Eliminate duplicated code in ParseEntityName{FromJson,FromExpression}

This patch introduces a templated ParseEntityName which can accept a
json or ExpressionPtr. Internally, performs a proper dispatch on these
cases

- isString
- isList
  - size == 2
  - size >= 3

A test is added for checking the proper handling of an empty list

3 files changed, 194 insertions, 139 deletions

diff --git a/src/buildtool/build_engine/base_maps/entity_name.hpp b/src/buildtool/build_engine/base_maps/entity_name.hpp
index 4f1e2189..e198b97f 100644
--- a/src/buildtool/build_engine/base_maps/entity_name.hpp
+++ b/src/buildtool/build_engine/base_maps/entity_name.hpp
@@ -14,36 +14,97 @@
 
 namespace BuildMaps::Base {
 
-[[nodiscard]] inline auto ParseEntityNameFromJson(
-    nlohmann::json const& json,
+static inline auto IsString(const nlohmann::json& x) noexcept -> bool {
+    return x.is_string();
+}
+
+static inline auto IsString(const ExpressionPtr& x) noexcept -> bool {
+    return x->IsString();
+}
+
+// for compatibility with ExpressionPtr
+static inline auto GetList(const nlohmann::json& x) noexcept
+    -> nlohmann::json const& {
+    return x;
+}
+
+static inline auto GetList(const ExpressionPtr& x) noexcept
+    -> Expression::list_t const& {
+    return x->Value<Expression::list_t>()->get();
+}
+
+static inline auto IsList(const nlohmann::json& x) noexcept -> bool {
+    return x.is_array();
+}
+
+static inline auto IsList(const ExpressionPtr& x) noexcept -> bool {
+    return x->IsList();
+}
+
+template <typename T>
+auto Size(const T& x) noexcept -> std::size_t {
+    return x.size();
+}
+
+static inline auto GetString(const nlohmann::json& x) -> std::string {
+    return x.template get<std::string>();
+}
+
+static inline auto GetString(const ExpressionPtr& x) -> const std::string& {
+    return x->String();
+}
+
+static inline auto ToString(const nlohmann::json& x) -> std::string {
+    return x.dump();
+}
+
+static inline auto ToString(const ExpressionPtr& x) noexcept -> std::string {
+    return x->ToString();
+}
+
+static inline auto IsNone(const nlohmann::json& x) noexcept -> bool {
+    return x.is_null();
+}
+
+static inline auto IsNone(const ExpressionPtr& x) noexcept -> bool {
+    return x->IsNone();
+}
+
+template <typename T>
+// IsList(list) == true and Size(list) == 2
+[[nodiscard]] inline auto ParseEntityName_2(T const& list,
+                                            EntityName const& current) noexcept
+    -> std::optional<EntityName> {
+    try {
+        if (IsString(list[0]) and IsString(list[1])) {
+            return EntityName{current.GetNamedTarget().repository,
+                              GetString(list[0]),
+                              GetString(list[1])};
+        }
+
+    } catch (...) {
+    }
+    return std::nullopt;
+}
+
+template <typename T>
+// IsList(list) == true and Size(list) >= 3
+// list[0] == kFileLocationMarker
+[[nodiscard]] inline auto ParseEntityNameFile(
+    T const& list,
     EntityName const& current,
     std::optional<std::function<void(std::string const&)>> logger =
         std::nullopt) noexcept -> std::optional<EntityName> {
     try {
-        if (json.is_string()) {
+        if (IsString(list[2])) {
+            const auto& name = GetString(list[2]);
             const auto& x = current.GetNamedTarget();
-            return EntityName{
-                x.repository, x.module, json.template get<std::string>()};
-        }
-        if (json.is_array() and json.size() == 2 and json[0].is_string() and
-            json[1].is_string()) {
-            return EntityName{current.GetNamedTarget().repository,
-                              json[0].template get<std::string>(),
-                              json[1].template get<std::string>()};
-        }
-        if (json.is_array() and json.size() == 3 and json[0].is_string() and
-            json[0].template get<std::string>() ==
-                EntityName::kFileLocationMarker and
-            json[2].is_string()) {
-            auto name = json[2].template get<std::string>();
-            if (json[1].is_null()) {
-                const auto& x = current.GetNamedTarget();
+            if (IsNone(list[1])) {
                 return EntityName{
                     x.repository, x.module, name, ReferenceType::kFile};
             }
-            if (json[1].is_string()) {
-                auto middle = json[1].template get<std::string>();
-                const auto& x = current.GetNamedTarget();
+            if (IsString(list[1])) {
+                const auto& middle = GetString(list[1]);
                 if (middle == "." or middle == x.module) {
                     return EntityName{
                         x.repository, x.module, name, ReferenceType::kFile};
@@ -52,19 +113,28 @@ namespace BuildMaps::Base {
             if (logger) {
                 (*logger)(
                     fmt::format("Invalid module name {} for file reference",
-                                json[1].dump()));
+                                ToString(list[1])));
             }
         }
-        else if (json.is_array() and json.size() == 3 and
-                 json[0].is_string() and
-                 json[0].template get<std::string>() ==
-                     EntityName::kRelativeLocationMarker and
-                 json[1].is_string() and json[2].is_string()) {
-            auto relmodule = json[1].template get<std::string>();
-            auto name = json[2].template get<std::string>();
+    } catch (...) {
+    }
+    return std::nullopt;
+}
+template <typename T>
+// IsList(list) == true and Size(list) >= 3
+// list[0] == kRelativeLocationMarker
+[[nodiscard]] inline auto ParseEntityNameRelative(
+    T const& list,
+    EntityName const& current,
+    std::optional<std::function<void(std::string const&)>> logger =
+        std::nullopt) noexcept -> std::optional<EntityName> {
+    try {
+        if (IsString(list[1]) and IsString(list[2])) {
+            const auto& relmodule = GetString(list[1]);
+            const auto& name = GetString(list[2]);
 
             std::filesystem::path m{current.GetNamedTarget().module};
-            auto module = (m / relmodule).lexically_normal().string();
+            const auto& module = (m / relmodule).lexically_normal().string();
             if (module.compare(0, 3, "../") != 0) {
                 return EntityName{
                     current.GetNamedTarget().repository, module, name};
@@ -75,24 +145,24 @@ namespace BuildMaps::Base {
                     relmodule));
             }
         }
-        else if (json.is_array() and json.size() == 3 and
-                 json[0].is_string() and
-                 json[0].template get<std::string>() ==
-                     EntityName::kAnonymousMarker) {
-            if (logger) {
-                (*logger)(fmt::format(
-                    "Parsing anonymous target from JSON is not supported."));
-            }
-        }
-        else if (json.is_array() and json.size() == 4 and
-                 json[0].is_string() and
-                 json[0].template get<std::string>() ==
-                     EntityName::kLocationMarker and
-                 json[1].is_string() and json[2].is_string() and
-                 json[3].is_string()) {
-            auto local_repo_name = json[1].template get<std::string>();
-            auto module = json[2].template get<std::string>();
-            auto target = json[3].template get<std::string>();
+    } catch (...) {
+    }
+    return std::nullopt;
+}
+
+template <typename T>
+// IsList(list) == true and Size(list) >= 3
+// list[0] == EntityName::kLocationMarker
+[[nodiscard]] inline auto ParseEntityNameLocation(
+    T const& list,
+    EntityName const& current,
+    std::optional<std::function<void(std::string const&)>> logger =
+        std::nullopt) noexcept -> std::optional<EntityName> {
+    try {
+        if (IsString(list[1]) and IsString(list[2]) and IsString(list[3])) {
+            const auto& local_repo_name = GetString(list[1]);
+            const auto& module = GetString(list[2]);
+            const auto& target = GetString(list[3]);
             auto const* repo_name = RepositoryConfig::Instance().GlobalName(
                 current.GetNamedTarget().repository, local_repo_name);
             if (repo_name != nullptr) {
@@ -103,114 +173,92 @@ namespace BuildMaps::Base {
                                       local_repo_name));
             }
         }
-        else if (logger) {
-            (*logger)(fmt::format("Syntactically invalid entity name: {}.",
-                                  json.dump()));
+
+    } catch (...) {
+    }
+    return std::nullopt;
+}
+
+template <typename T>
+// IsList(list) == true and Size(list) >= 3
+[[nodiscard]] inline auto ParseEntityName_3(
+    T const& list,
+    EntityName const& current,
+    std::optional<std::function<void(std::string const&)>> logger =
+        std::nullopt) noexcept -> std::optional<EntityName> {
+    try {
+        // the first entry of the list must be a string
+        if (IsString(list[0])) {
+            const auto& s0 = GetString(list[0]);
+            if (s0 == EntityName::kFileLocationMarker) {
+                return ParseEntityNameFile(list, current, logger);
+            }
+            if (s0 == EntityName::kRelativeLocationMarker) {
+                return ParseEntityNameRelative(list, current, logger);
+            }
+            if (s0 == EntityName::kLocationMarker) {
+                return ParseEntityNameLocation(list, current, logger);
+            }
+            if (s0 == EntityName::kAnonymousMarker and logger) {
+                (*logger)(fmt::format(
+                    "Parsing anonymous target is not "
+                    "supported. Identifiers of anonymous targets should be "
+                    "obtained as FIELD value of anonymous fields"));
+            }
         }
     } catch (...) {
     }
     return std::nullopt;
 }
 
-[[nodiscard]] inline auto ParseEntityNameFromExpression(
-    ExpressionPtr const& expr,
+template <typename T>
+[[nodiscard]] inline auto ParseEntityName(
+    T const& source,
     EntityName const& current,
     std::optional<std::function<void(std::string const&)>> logger =
         std::nullopt) noexcept -> std::optional<EntityName> {
     try {
-        if (expr) {
-            if (expr->IsString()) {
-                const auto& x = current.GetNamedTarget();
-                return EntityName{
-                    x.repository, x.module, expr->Value<std::string>()->get()};
+        std::optional<EntityName> res = std::nullopt;
+        if (IsString(source)) {
+            const auto& x = current.GetNamedTarget();
+            return EntityName{x.repository, x.module, GetString(source)};
+        }
+        if (IsList(source)) {
+            auto const& list = GetList(source);
+            const auto list_size = Size(list);
+            if (list_size == 2) {
+                res = ParseEntityName_2(list, current);
             }
-            if (expr->IsList()) {
-                auto const& list = expr->Value<Expression::list_t>()->get();
-                if (list.size() == 2 and list[0]->IsString() and
-                    list[1]->IsString()) {
-                    return EntityName{
-                        NamedTarget{current.GetNamedTarget().repository,
-                                    list[0]->Value<std::string>()->get(),
-                                    list[1]->Value<std::string>()->get()}};
-                }
-                if (list.size() == 3 and list[0]->IsString() and
-                    list[0]->String() == EntityName::kFileLocationMarker and
-                    list[2]->IsString()) {
-                    auto name = list[2]->Value<std::string>()->get();
-                    if (list[1]->IsNone()) {
-                        const auto& x = current.GetNamedTarget();
-                        return EntityName{
-                            x.repository, x.module, name, ReferenceType::kFile};
-                    }
-                    if (list[1]->IsString() and
-                        (list[1]->String() == "." or
-                         list[1]->String() ==
-                             current.GetNamedTarget().module)) {
-                        const auto& x = current.GetNamedTarget();
-                        return EntityName{
-                            x.repository, x.module, name, ReferenceType::kFile};
-                    }
-                    if (logger) {
-                        (*logger)(fmt::format(
-                            "Invalid module name {} for file reference",
-                            list[1]->ToString()));
-                    }
-                }
-                else if (list.size() == 3 and list[0]->IsString() and
-                         list[0]->String() ==
-                             EntityName::kRelativeLocationMarker and
-                         list[1]->IsString() and list[2]->IsString()) {
-                    std::filesystem::path m{current.GetNamedTarget().module};
-                    auto module =
-                        (m / (list[1]->String())).lexically_normal().string();
-                    if (module.compare(0, 3, "../") != 0) {
-                        return EntityName{
-                            NamedTarget{current.GetNamedTarget().repository,
-                                        module,
-                                        list[2]->String()}};
-                    }
-                    if (logger) {
-                        (*logger)(fmt::format(
-                            "Relative module name {} is outside of workspace",
-                            list[1]->String()));
-                    }
-                }
-                else if (list.size() == 3 and list[0]->IsString() and
-                         list[0]->String() ==
-                             EntityName::kRelativeLocationMarker and
-                         list[1]->IsMap() and list[2]->IsNode()) {
-                    return EntityName{AnonymousTarget{list[1], list[2]}};
-                }
-                else if (list.size() == 4 and list[0]->IsString() and
-                         list[0]->String() == EntityName::kLocationMarker and
-                         list[1]->IsString() and list[2]->IsString() and
-                         list[3]->IsString()) {
-                    auto const* repo_name =
-                        RepositoryConfig::Instance().GlobalName(
-                            current.GetNamedTarget().repository,
-                            list[1]->String());
-                    if (repo_name != nullptr) {
-                        return EntityName{
-                            *repo_name, list[2]->String(), list[3]->String()};
-                    }
-                    if (logger) {
-                        (*logger)(
-                            fmt::format("Cannot resolve repository name {}",
-                                        list[1]->String()));
-                    }
-                }
-                else if (logger) {
-                    (*logger)(
-                        fmt::format("Syntactically invalid entity name: {}.",
-                                    expr->ToString()));
-                }
+            else if (list_size >= 3) {
+                res = ParseEntityName_3(list, current, logger);
             }
         }
+        if (logger and (res == std::nullopt)) {
+            (*logger)(fmt::format("Syntactically invalid entity name: {}.",
+                                  ToString(source)));
+        }
+        return res;
     } catch (...) {
     }
     return std::nullopt;
 }
 
+[[nodiscard]] inline auto ParseEntityNameFromJson(
+    nlohmann::json const& json,
+    EntityName const& current,
+    std::optional<std::function<void(std::string const&)>> logger =
+        std::nullopt) noexcept -> std::optional<EntityName> {
+    return ParseEntityName(json, current, std::move(logger));
+}
+
+[[nodiscard]] inline auto ParseEntityNameFromExpression(
+    ExpressionPtr const& expr,
+    EntityName const& current,
+    std::optional<std::function<void(std::string const&)>> logger =
+        std::nullopt) noexcept -> std::optional<EntityName> {
+    return ParseEntityName(expr, current, std::move(logger));
+}
+
 }  // namespace BuildMaps::Base
 
 #endif
diff --git a/test/buildtool/build_engine/base_maps/data_rule/RULES b/test/buildtool/build_engine/base_maps/data_rule/RULES
index ac6c540a..f14248b2 100644
--- a/test/buildtool/build_engine/base_maps/data_rule/RULES
+++ b/test/buildtool/build_engine/base_maps/data_rule/RULES
@@ -84,6 +84,8 @@
   { "implicit": {"target": [["module_without_name"]]}
   , "expression": {"type": "RESULT"}
   }
+, "test_malformed_implicit_entity_name_2":
+  {"implicit": {"target": [[]]}, "expression": {"type": "RESULT"}}
 , "test_malformed_config_vars":
   {"config_vars": "not_a_list", "expression": {"type": "RESULT"}}
 , "test_malformed_config_transitions":
diff --git a/test/buildtool/build_engine/base_maps/rule_map.test.cpp b/test/buildtool/build_engine/base_maps/rule_map.test.cpp
index c0e64675..8667c7a9 100644
--- a/test/buildtool/build_engine/base_maps/rule_map.test.cpp
+++ b/test/buildtool/build_engine/base_maps/rule_map.test.cpp
@@ -323,6 +323,11 @@ TEST_CASE("Malformed rule description", "[rule_map]") {
                          [](auto /*values*/) {
                              CHECK(false);  // should never be called
                          }));
+        CHECK_FALSE(
+            ReadUserRule({"", ".", "test_malformed_implicit_entity_name_2"},
+                         [](auto /*values*/) {
+                             CHECK(false);  // should never be called
+                         }));
     }
 
     SECTION("Malformed config_vars") {