From 6a1c5cc2c05e7c013a44cddbf030b7d50f1daf12 Mon Sep 17 00:00:00 2001
From: Klaus Aehlig <klaus.aehlig@huawei.com>
Date: Tue, 22 Oct 2024 16:48:03 +0200
Subject: expressions: enforce strict arguments for "join" and "join_cmd"

... as described in the documentation. It was never intended to have
a single string being an argument for those.
---
 CHANGELOG.md                                       |  4 ++++
 .../build_engine/expression/evaluator.cpp          | 25 +++++++++++++---------
 .../build_engine/expression/expression.test.cpp    | 15 ++++++++-----
 3 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d4f194c3..dace2933 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -33,6 +33,10 @@ local build root on upgrade.
 - The built-in rule `"generic"` now properly enforces that the
   obtained outputs form a well-formed artifact stage; a conflicting
   arrangement of artifacts was possilbe beforehand.
+- The built-in expression functions `"join"` and `"join_cmd"`
+  now properly enforce that the argument is a list of strings.
+  So far, they used to accept a single string, treating it as a
+  singleton list.
 - A bug was fixed that cased `just serve` to fail with an internal
   error when building against ignore-special roots.
 - `just` now accurately reports internal errors that occurred on
diff --git a/src/buildtool/build_engine/expression/evaluator.cpp b/src/buildtool/build_engine/expression/evaluator.cpp
index 3e129539..dfae8499 100644
--- a/src/buildtool/build_engine/expression/evaluator.cpp
+++ b/src/buildtool/build_engine/expression/evaluator.cpp
@@ -347,14 +347,16 @@ auto ShellQuote(std::string arg) -> std::string {
     return fmt::format("'{}'", arg);
 }
 
-template <bool kDoQuote = false>
+template <bool kDoQuote = false, bool kAllowString = false>
 auto Join(ExpressionPtr const& expr, std::string const& sep) -> ExpressionPtr {
-    if (expr->IsString()) {
-        auto string = expr->String();
-        if constexpr (kDoQuote) {
-            string = ShellQuote(std::move(string));
+    if constexpr (kAllowString) {
+        if (expr->IsString()) {
+            auto string = expr->String();
+            if constexpr (kDoQuote) {
+                string = ShellQuote(std::move(string));
+            }
+            return ExpressionPtr{std::move(string)};
         }
-        return ExpressionPtr{std::move(string)};
     }
     if (expr->IsList()) {
         auto const& list = expr->List();
@@ -370,8 +372,10 @@ auto Join(ExpressionPtr const& expr, std::string const& sep) -> ExpressionPtr {
         });
         return ExpressionPtr{ss.str()};
     }
-    throw Evaluator::EvaluationError{fmt::format(
-        "Join expects string or list but got: {}.", expr->ToString())};
+    throw Evaluator::EvaluationError{
+        fmt::format("Join expects a list of strings{}, but got: {}.",
+                    kAllowString ? " or a single string" : "",
+                    expr->ToString())};
 }
 
 template <bool kDisjoint = false>
@@ -787,7 +791,7 @@ auto JoinCmdExpr(SubExprEvaluator&& eval,
                  ExpressionPtr const& expr,
                  Configuration const& env) -> ExpressionPtr {
     auto const& list = eval(expr->Get("$1", list_t{}), env);
-    return Join</*kDoQuote=*/true>(list, " ");
+    return Join</*kDoQuote=*/true, /*kAllowString=*/false>(list, " ");
 }
 
 auto JsonEncodeExpr(SubExprEvaluator&& eval,
@@ -1120,7 +1124,8 @@ auto ConcatTargetNameExpr(SubExprEvaluator&& eval,
                           Configuration const& env) -> ExpressionPtr {
     auto p1 = eval(expr->Get("$1", ""s), env);
     auto p2 = eval(expr->Get("$2", ""s), env);
-    return ConcatTargetName(p1, Join(p2, ""));
+    return ConcatTargetName(
+        p1, Join</*kDoQuote=*/false, /*kAllowString=*/true>(p2, ""));
 }
 
 auto ContextExpr(SubExprEvaluator&& eval,
diff --git a/test/buildtool/build_engine/expression/expression.test.cpp b/test/buildtool/build_engine/expression/expression.test.cpp
index aaca1c71..3a34702d 100644
--- a/test/buildtool/build_engine/expression/expression.test.cpp
+++ b/test/buildtool/build_engine/expression/expression.test.cpp
@@ -1042,14 +1042,11 @@ TEST_CASE("Expression Evaluation", "[expression]") {  // NOLINT
         REQUIRE(multi->IsString());
         CHECK(multi == Expression::FromJson(R"("foo;bar;baz")"_json));
 
+        // only list of strings are allowed
         expr = Replace(expr, "$1", foo);
         REQUIRE(expr);
-        auto string = expr.Evaluate(env, fcts);
-        REQUIRE(string);
-        REQUIRE(string->IsString());
-        CHECK(string == Expression::FromJson(R"("foo")"_json));
+        CHECK_FALSE(expr.Evaluate(env, fcts));
 
-        // only list of strings or string is allowed
         expr = Replace(expr, "$1", list_t{foo, ExpressionPtr{number_t{}}});
         REQUIRE(expr);
         CHECK_FALSE(expr.Evaluate(env, fcts));
@@ -1070,6 +1067,14 @@ TEST_CASE("Expression Evaluation", "[expression]") {  // NOLINT
         REQUIRE(result->IsString());
         CHECK(result ==
               Expression::FromJson(R"("'foo' 'bar'\\''s' 'baz'")"_json));
+
+        expr = Expression::FromJson(R"(
+          {"type": "join_cmd"
+          , "$1": "not a list"
+          }
+        )"_json);
+        REQUIRE(expr);
+        CHECK_FALSE(expr.Evaluate(env, fcts));
     }
 
     SECTION("escape_chars expression") {
-- 
cgit v1.2.3