Fix/default config bug

.pre-commit-config.yaml

@@ -10,16 +10,11 @@ repos:
   - id: check-added-large-files
   - id: check-merge-conflict
 
-- repo: https://github.com/pre-commit/mirrors-yapf
-  rev: 'v0.32.0'
+- repo: https://github.com/google/yapf
+  rev: 'v0.43.0'
   hooks:
   - id: yapf
 
-- repo: https://github.com/pylint-dev/pylint
-  rev: 'v3.0.0a6'
-  hooks:
-  - id: pylint
-
 - repo: https://github.com/pre-commit/mirrors-clang-format
   rev: 'v16.0.3'
   hooks:

config_utilities/CMakeLists.txt

@@ -9,9 +9,7 @@ if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
   add_compile_options(-Wall -Wextra -Wpedantic)
 endif()
 
-
 list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
-
 include(OptionalPackage)
 
 option(BUILD_SHARED_LIBS "Build shared libs" ON)
@@ -21,7 +19,7 @@ option(CONFIG_UTILS_BUILD_TESTS "Build unit tests" ON)
 option(CONFIG_UTILS_BUILD_DEMOS "Build demo executables" ON)
 
 find_package(yaml-cpp REQUIRED)
-find_package(Boost REQUIRED COMPONENTS filesystem system)
+find_package(Boost CONFIG REQUIRED COMPONENTS filesystem system)
 find_optional(Eigen3 CONFIG_UTILS_ENABLE_EIGEN)
 find_optional_pkgcfg(libglog CONFIG_UTILS_ENABLE_GLOG)
 
@@ -50,7 +48,7 @@ add_library(
   src/visitor.cpp
   src/yaml_parser.cpp
   src/yaml_utils.cpp
-  )
+)
 target_link_libraries(
   ${PROJECT_NAME}
   PUBLIC yaml-cpp
@@ -86,4 +84,4 @@ if(CONFIG_UTILS_BUILD_TESTS)
   add_subdirectory(test)
 endif()
 
-include(HandleInstall)
+include(HandleInstall)

config_utilities/src/visitor.cpp

@@ -99,11 +99,13 @@ std::optional<YAML::Node> Visitor::visitVirtualConfig(bool is_set,
     }
   }
 
-  if (is_set && (visitor.mode == Visitor::Mode::kGet || visitor.mode == Visitor::Mode::kGetInfo)) {
+  if (visitor.mode == Visitor::Mode::kGet || visitor.mode == Visitor::Mode::kGetInfo) {
     // Also write the type param back to file.
     std::string error;
-    YAML::Node type_node =
-        YamlParser::toYaml(Settings::instance().factory.type_param_name, type, visitor.name_space, error);
+    YAML::Node type_node = YamlParser::toYaml(Settings::instance().factory.type_param_name,
+                                              is_set ? type : kUninitializedVirtualConfigType,
+                                              visitor.name_space,
+                                              error);
     mergeYamlNodes(visitor.data.data, type_node);
   }
 

config_utilities/test/tests/virtual_config.cpp

@@ -96,7 +96,7 @@ struct Derived2WithComplexParam : public Base2 {
   const std::shared_ptr<int> i_;
   inline static const auto registration_ =
       config::RegistrationWithConfig<Base2, Derived2WithComplexParam, Config, std::shared_ptr<int>>(
-        "Derived2WithComplexParam");
+          "Derived2WithComplexParam");
 };
 
 void declare_config(Derived2WithComplexParam::Config& config) {
@@ -108,13 +108,14 @@ struct Derived2WithMoveOnlyParam : public Base2 {
   struct Config {
     int i = 0;
   };
-  explicit Derived2WithMoveOnlyParam(const Config& config, std::unique_ptr<int> i) : config_(config), i_(std::move(i)) {}
+  explicit Derived2WithMoveOnlyParam(const Config& config, std::unique_ptr<int> i)
+      : config_(config), i_(std::move(i)) {}
   std::string name() const override { return "Derived2WithMoveOnlyParam"; }
   const Config config_;
   const std::unique_ptr<int> i_;
   inline static const auto registration_ =
       config::RegistrationWithConfig<Base2, Derived2WithMoveOnlyParam, Config, std::unique_ptr<int>>(
-        "Derived2WithMoveOnlyParam");
+          "Derived2WithMoveOnlyParam");
 };
 
 void declare_config(Derived2WithMoveOnlyParam::Config& config) {
@@ -174,6 +175,48 @@ void declare_config(ObjectWithOptionalConfigs::Config& config) {
   config::field(config.modules, "modules");
 }
 
+struct DefaultedOptional {
+  struct Config {
+    int foo = 3;
+  } const config;
+  explicit DefaultedOptional(const Config& config) : config(config) {}
+};
+
+void declare_config(DefaultedOptional::Config& config) {
+  name<DefaultedOptional::Config>();
+  field(config.foo, "foo");
+}
+
+struct ParentOfDefaultedOptional {
+  struct Config {
+    VirtualConfig<DefaultedOptional> child{DefaultedOptional::Config()};
+  } const config;
+
+  explicit ParentOfDefaultedOptional(const Config& config) : config(config), child(config.child.create()) {}
+  std::unique_ptr<DefaultedOptional> child;
+};
+
+void declare_config(ParentOfDefaultedOptional::Config& config) {
+  name<ParentOfDefaultedOptional::Config>();
+  field(config.child, "child");
+  config.child.setOptional();
+}
+
+struct GrandparentOfDefaultedOptional {
+  struct Config {
+    VirtualConfig<ParentOfDefaultedOptional> child;
+  } const config;
+
+  explicit GrandparentOfDefaultedOptional(const Config& config) : config(config), child(config.child.create()) {}
+  std::unique_ptr<ParentOfDefaultedOptional> child;
+};
+
+void declare_config(GrandparentOfDefaultedOptional::Config& config) {
+  name<ParentOfDefaultedOptional::Config>();
+  field(config.child, "child");
+  config.child.setOptional();
+}
+
 TEST(VirtualConfig, isSet) {
   Settings().restoreDefaults();
 
@@ -570,4 +613,57 @@ TEST(VirtualConfig, optionalNullCreation) {
   ASSERT_EQ(object, nullptr);
 }
 
+TEST(VirtualConfig, defaultedConfigCorrect) {
+  RegistrationGuard<DefaultedOptional, DefaultedOptional, DefaultedOptional::Config> guard("DefaultedOptional");
+  RegistrationGuard<ParentOfDefaultedOptional, ParentOfDefaultedOptional, ParentOfDefaultedOptional::Config>
+      parent_guard("ParentOfDefaultedOptional");
+  RegistrationGuard<GrandparentOfDefaultedOptional,
+                    GrandparentOfDefaultedOptional,
+                    GrandparentOfDefaultedOptional::Config>
+      grandparent_guard("GrandparentOfDefaultedOptional");
+
+  {  // default config does the right thing from YAML
+    const auto node = YAML::Load(R"""(
+type: GrandparentOfDefaultedOptional
+child:
+  type: ParentOfDefaultedOptional
+)""");
+    auto root = config::createFromYaml<GrandparentOfDefaultedOptional>(node);
+    ASSERT_TRUE(root);
+    ASSERT_TRUE(root->child);
+    EXPECT_TRUE(root->child->child);
+  }
+
+  {  // manually specifying the type does the right thing
+    const auto node = YAML::Load(R"""(
+type: GrandparentOfDefaultedOptional
+child:
+  type: ParentOfDefaultedOptional
+  child:
+    type: DefaultedOptional
+    foo: 5
+)""");
+    auto root = config::createFromYaml<GrandparentOfDefaultedOptional>(node);
+    ASSERT_TRUE(root);
+    ASSERT_TRUE(root->child);
+    ASSERT_TRUE(root->child->child);
+    EXPECT_EQ(root->child->child->config.foo, 5);
+  }
+
+  {  // overriding default does the right thing
+    const auto node = YAML::Load(R"""(
+type: GrandparentOfDefaultedOptional
+child:
+  type: ParentOfDefaultedOptional
+  child:
+    type: ''
+)""");
+    auto root_config = config::fromYaml<GrandparentOfDefaultedOptional::Config>(node);
+    auto root = config::createFromYaml<GrandparentOfDefaultedOptional>(node);
+    ASSERT_TRUE(root);
+    ASSERT_TRUE(root->child);
+    EXPECT_FALSE(root->child->child);
+  }
+}
+
 }  // namespace config::test