Fix variable append logic in script

8 files changed, 105 insertions, 91 deletions

diff --git a/libbuild2/build/script/parser+for.test.testscript b/libbuild2/build/script/parser+for.test.testscript
index f4ebf3c..877f958 100644
--- a/libbuild2/build/script/parser+for.test.testscript
+++ b/libbuild2/build/script/parser+for.test.testscript
@@ -87,9 +87,7 @@
       cmd $x
     end
     EOI
-    error: type mismatch in variable x
-      info: value type is dir_path
-      info: variable type is string
+    buildfile:11:1: error: conflicting variable x type string and value type dir_path
     EOE
 
     : defined-var
diff --git a/libbuild2/build/script/parser.cxx b/libbuild2/build/script/parser.cxx
index cb0dbbb..8759cdf 100644
--- a/libbuild2/build/script/parser.cxx
+++ b/libbuild2/build/script/parser.cxx
@@ -1305,19 +1305,25 @@ namespace build2
         // Note that we rely on "small function object" optimization for the
         // exec_*() lambdas.
         //
-        auto exec_assign = [this] (const variable& var,
-                                   value&& val,
-                                   type kind,
-                                   const location& l)
+        auto exec_set = [this] (const variable& var,
+                                token& t, build2::script::token_type& tt,
+                                const location&)
         {
+          next (t, tt);
+          type kind (tt); // Assignment kind.
+
+          mode (lexer_mode::variable_line);
+          value rhs (parse_variable_line (t, tt));
+
+          assert (tt == type::newline);
+
+          // Assign.
+          //
           value& lhs (kind == type::assign
                       ? environment_->assign (var)
                       : environment_->append (var));
 
-          if (kind == type::assign)
-            lhs = move (val);
-          else
-            append_value (&var, lhs, move (val), l);
+          apply_value_attributes (&var, lhs, move (rhs), kind);
         };
 
         auto exec_cond = [this] (token& t, build2::script::token_type& tt,
@@ -1333,9 +1339,22 @@ namespace build2
           return runner_->run_cond (*environment_, ce, ii, li, ll);
         };
 
+        auto exec_for = [this] (const variable& var,
+                                value&& val,
+                                const location& l)
+        {
+          value& lhs (environment_->assign (var));
+
+          // To match the function semantics also pass the value's type
+          // attribute, restoring it from RHS. Note that the value can't be
+          // NULL.
+          //
+          apply_value (&var, lhs, move (val), type::assign, l, val.type);
+        };
+
         build2::script::parser::exec_lines (
           begin, end,
-          exec_assign, exec_cmd, exec_cond,
+          exec_set, exec_cmd, exec_cond, exec_for,
           nullptr /* iteration_index */,
           environment_->exec_line,
           &environment_->var_pool);
diff --git a/libbuild2/parser.cxx b/libbuild2/parser.cxx
index 5dbbfac..1cffb85 100644
--- a/libbuild2/parser.cxx
+++ b/libbuild2/parser.cxx
@@ -5001,6 +5001,18 @@ namespace build2
         fail (l) << "unexpected value in attribute " << a;
     }
 
+    apply_value (var, v, move (rhs), kind, l, type, null);
+  }
+
+  void parser::
+  apply_value (const variable* var,
+               value& v,
+               value&& rhs,
+               type kind,
+               const location& l,
+               const value_type* type,
+               bool null)
+  {
     // When do we set the type and when do we keep the original? This gets
     // tricky for append/prepend where both values contribute. The guiding
     // rule here is that if the user specified the type, then they reasonable
diff --git a/libbuild2/parser.hxx b/libbuild2/parser.hxx
index 0d1e9e2..02d4f74 100644
--- a/libbuild2/parser.hxx
+++ b/libbuild2/parser.hxx
@@ -304,6 +304,18 @@ namespace build2
                             value&& rhs,
                             token_type assign_kind);
 
+    void
+    apply_value (const variable*, // Optional.
+                 value& lhs,
+                 value&& rhs,
+                 token_type assign_kind,
+                 const location&,
+                 //
+                 // Attributes:
+                 //
+                 const value_type* = nullptr,
+                 bool null = false);
+
     // Return the value pack (values can be NULL/typed). Note that for an
     // empty eval context ('()' potentially with whitespaces in between) the
     // result is an empty pack, not a pack of one empty.
diff --git a/libbuild2/script/parser.cxx b/libbuild2/script/parser.cxx
index aa186ff..536821b 100644
--- a/libbuild2/script/parser.cxx
+++ b/libbuild2/script/parser.cxx
@@ -2035,37 +2035,6 @@ namespace build2
       build2::parser::apply_value_attributes (var, lhs, move (rhs), kind);
     }
 
-    void parser::
-    append_value (const variable* var,
-                  value& v,
-                  value&& rhs,
-                  const location& l)
-    {
-      if (rhs) // Don't append/prepent NULL.
-      {
-        // Perform the type conversion (see
-        // build2::parser::apply_value_attributes() for the approach
-        // reasoning).
-        //
-        if (rhs.type != nullptr)
-        {
-          if (!v)
-            v.type = rhs.type;
-          else if (v.type == nullptr)
-            typify (v, *rhs.type, var);
-          else if (v.type != rhs.type)
-            fail (l) << "conflicting original value type " << v.type->name
-                     << " and append value type " << rhs.type->name;
-
-          // Reduce this to the untyped value case.
-          //
-          untypify (rhs);
-        }
-
-        v.append (move (rhs).as<names> (), var);
-      }
-    }
-
     line_type parser::
     pre_parse_line_start (token& t, token_type& tt, lexer_mode stm)
     {
@@ -2122,9 +2091,10 @@ namespace build2
 
     bool parser::
     exec_lines (lines::const_iterator i, lines::const_iterator e,
-                const function<exec_assign_function>& exec_assign,
+                const function<exec_set_function>& exec_set,
                 const function<exec_cmd_function>& exec_cmd,
                 const function<exec_cond_function>& exec_cond,
+                const function<exec_for_function>& exec_for,
                 const iteration_index* ii, size_t& li,
                 variable_pool* var_pool)
     {
@@ -2233,25 +2203,7 @@ namespace build2
                 var = &var_pool->insert (t.value);
               }
 
-              next (t, tt);
-              type kind (tt); // Assignment kind.
-
-              assert (kind == type::assign || kind == type::append);
-
-              // Parse the value with the potential attributes.
-              //
-              // Note that we don't really need to change the mode since we
-              // are replaying the tokens.
-              //
-              value val;
-              apply_value_attributes (var,
-                                      val,
-                                      parse_variable_line (t, tt),
-                                      kind);
-
-              assert (tt == type::newline);
-
-              exec_assign (*var, move (val), kind, ll);
+              exec_set (*var, t, tt, ll);
 
               replay_stop ();
               break;
@@ -2312,7 +2264,7 @@ namespace build2
                 lines::const_iterator j (fcend (i, false, false));
 
                 if (!exec_lines (i + 1, j,
-                                 exec_assign, exec_cmd, exec_cond,
+                                 exec_set, exec_cmd, exec_cond, exec_for,
                                  ii, li,
                                  var_pool))
                   return false;
@@ -2364,7 +2316,7 @@ namespace build2
                     we = fcend (i, true, false);
 
                   if (!exec_lines (i + 1, we,
-                                   exec_assign, exec_cmd, exec_cond,
+                                   exec_set, exec_cmd, exec_cond, exec_for,
                                    &wi, li,
                                    var_pool))
                     return false;
@@ -2471,7 +2423,7 @@ namespace build2
                   if (etype != nullptr)
                     typify (v, *etype, var);
 
-                  exec_assign (*var, move (v), type::assign, ll);
+                  exec_for (*var, move (v), ll);
 
                   // Find the construct end, if it is not found yet.
                   //
@@ -2479,7 +2431,7 @@ namespace build2
                     fe = fcend (i, true, false);
 
                   if (!exec_lines (i + 1, fe,
-                                   exec_assign, exec_cmd, exec_cond,
+                                   exec_set, exec_cmd, exec_cond, exec_for,
                                    &fi, li,
                                    var_pool))
                     return false;
diff --git a/libbuild2/script/parser.hxx b/libbuild2/script/parser.hxx
index 0f276e1..3a4c46f 100644
--- a/libbuild2/script/parser.hxx
+++ b/libbuild2/script/parser.hxx
@@ -42,11 +42,6 @@ namespace build2
 
       using build2::parser::apply_value_attributes;
 
-      // The variable is optional and is only used for diagnostics.
-      //
-      void
-      append_value (const variable*, value& lhs, value&& rhs, const location&);
-
       // Return true if a command line element needs to be re-lexed.
       //
       // Specifically, it needs to be re-lexed if it contains any of the
@@ -167,10 +162,9 @@ namespace build2
       // builtin). For unsuccessful termination the failed exception is
       // thrown.
       //
-      using exec_assign_function = void (const variable&,
-                                         value&&,
-                                         token_type kind,
-                                         const location&);
+      using exec_set_function = void (const variable&,
+                                      token&, token_type&,
+                                      const location&);
 
       using exec_cmd_function = void (token&, token_type&,
                                       const iteration_index*, size_t li,
@@ -181,6 +175,10 @@ namespace build2
                                         const iteration_index*, size_t li,
                                         const location&);
 
+      using exec_for_function = void (const variable&,
+                                      value&&,
+                                      const location&);
+
       // If a parser implementation doesn't pre-enter variables into a pool
       // during the pre-parsing phase, then they are entered during the
       // execution phase and so the variable pool must be provided. Note that
@@ -188,9 +186,10 @@ namespace build2
       //
       bool
       exec_lines (lines::const_iterator b, lines::const_iterator e,
-                  const function<exec_assign_function>&,
+                  const function<exec_set_function>&,
                   const function<exec_cmd_function>&,
                   const function<exec_cond_function>&,
+                  const function<exec_for_function>&,
                   const iteration_index*, size_t& li,
                   variable_pool* = nullptr);
 
diff --git a/libbuild2/test/script/parser+for.test.testscript b/libbuild2/test/script/parser+for.test.testscript
index 92fc714..70c1c89 100644
--- a/libbuild2/test/script/parser+for.test.testscript
+++ b/libbuild2/test/script/parser+for.test.testscript
@@ -87,9 +87,7 @@
       cmd $x
     end
     EOI
-    error: type mismatch in variable x
-      info: value type is dir_path
-      info: variable type is string
+    testscript:1:1: error: conflicting variable x type string and value type dir_path
     EOE
 
     : scope-var
diff --git a/libbuild2/test/script/parser.cxx b/libbuild2/test/script/parser.cxx
index 0ba5a79..f302aee 100644
--- a/libbuild2/test/script/parser.cxx
+++ b/libbuild2/test/script/parser.cxx
@@ -1549,19 +1549,27 @@ namespace build2
         // Note that we rely on "small function object" optimization for the
         // exec_*() lambdas.
         //
-        auto exec_assign = [this] (const variable& var,
-                                   value&& val,
-                                   type kind,
-                                   const location& l)
+        auto exec_set = [this] (const variable& var,
+                                token& t, build2::script::token_type& tt,
+                                const location&)
         {
+          next (t, tt);
+          type kind (tt); // Assignment kind.
+
+          // We cannot reuse the value mode (see above for details).
+          //
+          mode (lexer_mode::variable_line);
+          value rhs (parse_variable_line (t, tt));
+
+          assert (tt == type::newline);
+
+          // Assign.
+          //
           value& lhs (kind == type::assign
                       ? scope_->assign (var)
                       : scope_->append (var));
 
-          if (kind == type::assign)
-            lhs = move (val);
-          else
-            append_value (&var, lhs, move (val), l);
+          apply_value_attributes (&var, lhs, move (rhs), kind);
 
           if (script_->test_command_var (var.name))
             scope_->reset_special ();
@@ -1601,6 +1609,22 @@ namespace build2
           return runner_->run_cond (*scope_, ce, ii, li, ll);
         };
 
+        auto exec_for = [this] (const variable& var,
+                                value&& val,
+                                const location& l)
+        {
+          value& lhs (scope_->assign (var));
+
+          // To match the function semantics also pass the value's type
+          // attribute, restoring it from RHS. Note that the value can't be
+          // NULL.
+          //
+          apply_value (&var, lhs, move (val), type::assign, l, val.type);
+
+          if (script_->test_command_var (var.name))
+            scope_->reset_special ();
+        };
+
         size_t li (1);
 
         if (test* t = dynamic_cast<test*> (scope_))
@@ -1608,7 +1632,7 @@ namespace build2
           ct = command_type::test;
 
           exec_lines (t->tests_.begin (), t->tests_.end (),
-                      exec_assign, exec_cmd, exec_cond,
+                      exec_set, exec_cmd, exec_cond, exec_for,
                       nullptr /* iteration_index */, li);
         }
         else if (group* g = dynamic_cast<group*> (scope_))
@@ -1617,7 +1641,7 @@ namespace build2
 
           bool exec_scope (
             exec_lines (g->setup_.begin (), g->setup_.end (),
-                        exec_assign, exec_cmd, exec_cond,
+                        exec_set, exec_cmd, exec_cond, exec_for,
                         nullptr /* iteration_index */, li));
 
           if (exec_scope)
@@ -1788,7 +1812,7 @@ namespace build2
           ct = command_type::teardown;
 
           exec_lines (g->tdown_.begin (), g->tdown_.end (),
-                      exec_assign, exec_cmd, exec_cond,
+                      exec_set, exec_cmd, exec_cond, exec_for,
                       nullptr /* iteration_index */, li);
         }
         else