Implement value type propagation on expansion

Currently, we only propagate types of sole, unquoted expansions (variable,
function call, or eval context), similar to NULL. To untypify the value,
simply quote it.

14 files changed, 265 insertions, 95 deletions

diff --git a/build2/functions-process-path.cxx b/build2/functions-process-path.cxx
index 7069c33..a1d6d7c 100644
--- a/build2/functions-process-path.cxx
+++ b/build2/functions-process-path.cxx
@@ -21,15 +21,5 @@ namespace build2
       {
         return move (p.effect.empty () ? p.recall : p.effect);
       };
-
-    //@@ TMP kludge
-    //
-    f["effect"] = [](names n)
-      {
-        auto p (value_traits<process_path>::convert (
-                  move (n[0]), n.size () > 1 ? &n[1] : nullptr));
-
-        return move (p.effect.empty () ? p.recall : p.effect);
-      };
   }
 }
diff --git a/build2/parser b/build2/parser
index 9b326b1..e1f6e77 100644
--- a/build2/parser
+++ b/build2/parser
@@ -110,8 +110,9 @@ namespace build2
                             value&& rhs,
                             token_type kind);
 
-    // Return the value as well as the indication of whether this is a non-
-    // empty eval context (i.e., '()' potentially with whitespace in between).
+    // Return the value (can be NULL/typed) as well as the indication of
+    // whether this is a non-empty eval context (i.e., '()' potentially with
+    // whitespace in between).
     //
     pair<value, bool>
     parse_eval (token&, token_type&);
@@ -187,26 +188,46 @@ namespace build2
       return ns;
     }
 
-    // As above but return the result as a value (always untyped) indicating
-    // if it is NULL. The only way for the value to be NULL is if it is the
-    // result of a sole, unquoted variable expansion, function call, or
-    // context evaluation.
+    // As above but return the result as a value, which can be typed and NULL.
     //
     value
-    parse_names_value (token& t, token_type& tt,
-                       const char* what = "name",
-                       const string* separators = &name_separators)
+    parse_value (token& t, token_type& tt,
+                 const char* what = "name",
+                 const string* separators = &name_separators)
     {
       names ns;
-      return parse_names (
-        t, tt, ns, false, what, separators, 0, nullptr, nullptr, nullptr)
-        ? value (move (ns))
-        : value (nullptr);
+      pair<bool, const value_type*> p (
+        parse_names (
+          t, tt, ns, false, what, separators, 0, nullptr, nullptr, nullptr));
+
+      value r (p.second); // Potentially typed NULL value.
+
+      // This should not fail since we are typing the result of reversal from
+      // the typed value.
+      //
+      if (p.first) // Not NULL.
+        r.assign (move (ns), nullptr);
+
+      return r;
     }
 
-    // Append names and return true if the parsed value is NOT NULL.
+    // Append names and return the indication if the parsed value is NOT NULL
+    // (first) and whether it is typed (second).
     //
-    bool
+    // You may have noticed that what we return here is essentially a value
+    // and doing it this way (i.e., reversing it to untyped names and
+    // returning its type so that it can potentially be "typed back") is kind
+    // of backwards. The reason we are doing it this way is because in many
+    // places we expect things untyped and if we were to always return a
+    // (potentially typed) value, then we would have to reverse it in all
+    // those places. Still it may make sense to look into redesigning the
+    // whole thing one day.
+    //
+    // Currently the only way for the result to be NULL or have a type is if
+    // it is the result of a sole, unquoted variable expansion, function call,
+    // or context evaluation.
+    //
+    pair<bool, const value_type*>
     parse_names (token&, token_type&,
                  names&,
                  bool chunk = false,
diff --git a/build2/parser.cxx b/build2/parser.cxx
index df2d168..5f9850d 100644
--- a/build2/parser.cxx
+++ b/build2/parser.cxx
@@ -626,7 +626,8 @@ namespace build2
 
                   if (lhs.extra != 0 && lhs.type != nullptr)
                     fail (at) << "typed prepend/append to target type/pattern-"
-                              << "specific variable " << var;
+                              << "specific variable " << var <<
+                      info << "use quoting to untypify the value";
                 }
               }
 
@@ -1138,6 +1139,7 @@ namespace build2
     // The rest is a value. Parse it as a variable value to get expansion,
     // attributes, etc. build2::import() will check the names, if required.
     //
+    location l (get_location (t));
     value rhs (parse_variable_value (t, tt));
 
     // While it may seem like supporting attributes is a good idea here,
@@ -1155,7 +1157,11 @@ namespace build2
       attributes_pop ();
 
     if (!rhs)
-      fail (t) << "null value in export";
+      fail (l) << "null value in export";
+
+    if (rhs.type != nullptr)
+      fail (l) << "typed value in export" <<
+        info << "use quoting to untypify the value";
 
     export_value_ = move (rhs).as<names> ();
 
@@ -1526,7 +1532,7 @@ namespace build2
     attributes_push (t, tt, true);
 
     return tt != type::newline && tt != type::eos
-      ? parse_names_value (t, tt)
+      ? parse_value (t, tt)
       : value (names ());
   }
 
@@ -1643,6 +1649,23 @@ namespace build2
         fail (l) << "unexpected value for attribute " << k << ": " << v;
     }
 
+    // If we have both attribute and value types, then they better match.
+    //
+    if (rhs.type != nullptr)
+    {
+      if (type != nullptr && type != rhs.type)
+      {
+        fail (l) << "conflicting attribute type " << type->name
+                 << " and value type " << rhs.type->name <<
+          info << "use quoting to untypify the value";
+      }
+
+      // Reduce this to the untyped value case for simplicity.
+      //
+      type = rhs.type;
+      untypify (rhs);
+    }
+
     // 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
@@ -1720,14 +1743,14 @@ namespace build2
   {
     // Parse the next value (if any) handling its attributes.
     //
-    auto parse_value = [&t, &tt, this] () -> value
+    auto parse = [&t, &tt, this] () -> value
     {
       // Parse value attributes if any. Note that it's ok not to have anything
       // after the attributes, as in, ($foo == [null]), or even ([null])
       //
       auto at (attributes_push (t, tt, true));
 
-      // Note that if names() gets called, it expects to see a name.
+      // Note that if parse_value() gets called, it expects to see a value.
       //
       value r (tt != type::rparen     &&
                tt != type::colon      &&
@@ -1737,7 +1760,7 @@ namespace build2
                tt != type::less_equal &&
                tt != type::greater    &&
                tt != type::greater_equal
-               ? parse_names_value (t, tt)
+               ? parse_value (t, tt)
                : value (names ()));
 
       if (pre_parse_)
@@ -1756,7 +1779,7 @@ namespace build2
       return v;
     };
 
-    value lhs (parse_value ());
+    value lhs (parse ());
 
     // We continue evaluating from left to right until we reach ')', storing
     // the result in lhs (think 'a == b == false').
@@ -1814,7 +1837,7 @@ namespace build2
           // @@ In C++ == and != have lower precedence than <, etc.
           //
           next (t, tt);
-          value rhs (parse_value ());
+          value rhs (parse ());
 
           if (pre_parse_)
             break;
@@ -2076,7 +2099,7 @@ namespace build2
   const string parser::name_separators (
     string (path::traits::directory_separators) + '%');
 
-  bool parser::
+  pair<bool, const value_type*> parser::
   parse_names (token& t, type& tt,
                names& ns,
                bool chunk,
@@ -2092,7 +2115,10 @@ namespace build2
 
     tracer trace ("parser::parse_names", &path_);
 
-    bool null (false);
+    // Returned value NULL/type (see below).
+    //
+    bool vnull (false);
+    const value_type* vtype (nullptr);
 
     // If pair is not 0, then it is an index + 1 of the first half of
     // the pair for which we are parsing the second halves, e.g.,
@@ -2315,11 +2341,11 @@ namespace build2
         names lv_storage;
         names_view lv;
 
-        // Check if we should set/propagate NULL. We only do this if this is
-        // the only expansion, that is, it is the first and the text token is
-        // not part of the name.
+        // Check if we should set/propagate value NULL/type. We only do this
+        // if this is the only expansion, that is, it is the first and the
+        // text token is not part of the name.
         //
-        auto set_null = [first, &tt] ()
+        auto set_value = [first, &tt] ()
         {
           return first &&
             tt != type::word &&
@@ -2423,17 +2449,15 @@ namespace build2
                                        args.second ? 1 : 0),
                                      loc);
 
-            // See if we should propagate the NULL indicator.
+            // See if we should propagate the value NULL/type.
             //
-            if (!result)
+            if (set_value ())
             {
-              if (set_null ())
-                null = true;
-
-              continue;
+              vnull = result.null;
+              vtype = result.type;
             }
 
-            if (result.empty ())
+            if (!result || result.empty ())
               continue;
 
             lv = reverse (result, lv_storage);
@@ -2448,17 +2472,15 @@ namespace build2
             //
             lookup l (lookup_variable (move (qual), move (name), loc));
 
-            if (!l)
+            // See if we should propagate the value NULL/type.
+            //
+            if (set_value ())
             {
-              // See if we should set the NULL indicator.
-              //
-              if (set_null ())
-                null = true;
-
-              continue;
+              vnull = !l;
+              vtype = l.defined () ? l->type : nullptr;
             }
 
-            if (l->empty ())
+            if (!l || l->empty ())
               continue;
 
             lv = reverse (*l, lv_storage);
@@ -2475,17 +2497,15 @@ namespace build2
           if (pre_parse_)
             continue; // As if empty result.
 
-          // See if we should propagate the NULL indicator.
+          // See if we should propagate the value NULL/type.
           //
-          if (!result)
+          if (set_value ())
           {
-            if (set_null ())
-              null = true;
-
-            continue;
+            vnull = result.null;
+            vtype = result.type;
           }
 
-          if (result.empty ())
+          if (!result || result.empty ())
             continue;
 
           lv = reverse (result, lv_storage);
@@ -2678,7 +2698,7 @@ namespace build2
         continue;
       }
 
-      // Note: remember to update set_null test if adding new recognized
+      // Note: remember to update set_value() test if adding new recognized
       // tokens.
 
       if (!first)
@@ -2714,7 +2734,7 @@ namespace build2
                        string ());
     }
 
-    return !null;
+    return make_pair (!vnull, vtype);
   }
 
   void parser::
@@ -2791,23 +2811,23 @@ namespace build2
   //
 
   // Here is the problem: we "overload" '(' and ')' to mean operation
-  // application rather than the eval context. At the same time we want
-  // to use names() to parse names, get variable expansion/function calls,
-  // quoting, etc. We just need to disable the eval context. The way this
-  // is done has two parts: Firstly, we parse names in chunks and detect
-  // and handle the opening paren. In other words, a buildspec like
-  // 'clean (./)' is "chunked" as 'clean', '(', etc. While this is fairly
-  // straightforward, there is one snag: concatenating eval contexts, as
-  // in 'clean(./)'. Normally, this will be treated as a single chunk and
-  // we don't want that. So here comes the trick (or hack, if you like):
-  // we will make every opening paren token "separated" (i.e., as if it
-  // was proceeded by a space). This will disable concatenating eval. In
-  // fact, we will even go a step further and only do this if we are in
-  // the original value mode. This will allow us to still use eval
-  // contexts in buildspec, provided that we quote it: '"cle(an)"'. Note
-  // also that function calls still work as usual: '$filter (clean test)'.
-  // To disable a function call and make it instead a var that is expanded
-  // into operation name(s), we can use quoting: '"$ops"(./)'.
+  // application rather than the eval context. At the same time we want to use
+  // parse_names() to parse names, get variable expansion/function calls,
+  // quoting, etc. We just need to disable the eval context. The way this is
+  // done has two parts: Firstly, we parse names in chunks and detect and
+  // handle the opening paren. In other words, a buildspec like 'clean (./)'
+  // is "chunked" as 'clean', '(', etc. While this is fairly straightforward,
+  // there is one snag: concatenating eval contexts, as in
+  // 'clean(./)'. Normally, this will be treated as a single chunk and we
+  // don't want that. So here comes the trick (or hack, if you like): we will
+  // make every opening paren token "separated" (i.e., as if it was proceeded
+  // by a space). This will disable concatenating eval. In fact, we will even
+  // go a step further and only do this if we are in the original value
+  // mode. This will allow us to still use eval contexts in buildspec,
+  // provided that we quote it: '"cle(an)"'. Note also that function calls
+  // still work as usual: '$filter (clean test)'.  To disable a function call
+  // and make it instead a var that is expanded into operation name(s), we can
+  // use quoting: '"$ops"(./)'.
   //
   static void
   paren_processor (token& t, const lexer& l)
@@ -2889,7 +2909,7 @@ namespace build2
       // opening paren, then they are operation/meta-operation names.
       // Otherwise they are targets.
       //
-      if (tt == type::lparen) // Peeked into by names().
+      if (tt == type::lparen) // Peeked into by parse_names().
       {
         if (ns.empty ())
           fail (t) << "operation name expected before '('";
diff --git a/build2/test/script/parser.cxx b/build2/test/script/parser.cxx
index 5730647..9afef75 100644
--- a/build2/test/script/parser.cxx
+++ b/build2/test/script/parser.cxx
@@ -1190,7 +1190,7 @@ namespace build2
         attributes_push (t, tt, true);
 
         return tt != type::newline && tt != type::semi
-          ? parse_names_value (t, tt, "variable value", nullptr)
+          ? parse_value (t, tt, "variable value", nullptr)
           : value (names ());
       }
 
diff --git a/build2/variable b/build2/variable
index 4fd802f..dd4f551 100644
--- a/build2/variable
+++ b/build2/variable
@@ -296,6 +296,11 @@ namespace build2
   void typify (value&, const variable&);
   void typify (value&, const value_type&, const variable*);
 
+  // Remove value type from the value reversing it to names. This is similar
+  // to reverse() below except that it modifies the value itself.
+  //
+  void untypify (value&);
+
   // Reverse the value back to names. The value should not be NULL and storage
   // should be empty.
   //
diff --git a/build2/variable.cxx b/build2/variable.cxx
index 39fdb5d..5a52557 100644
--- a/build2/variable.cxx
+++ b/build2/variable.cxx
@@ -336,6 +336,41 @@ namespace build2
     }
   }
 
+  void
+  untypify (value& v)
+  {
+    if (v.type == nullptr)
+      return;
+
+    if (v.null)
+    {
+      v.type = nullptr;
+      return;
+    }
+
+    names ns;
+    names_view nv (v.type->reverse (v, ns));
+
+    if (nv.empty () || nv.data () == ns.data ())
+    {
+      // If the data is in storage, then we are all set.
+      //
+      ns.resize (nv.size ()); // Just to be sure.
+    }
+    else
+    {
+      // If the data is somewhere in the value itself, then steal it.
+      //
+      auto b (const_cast<name*> (nv.data ()));
+      ns.assign (make_move_iterator (b),
+                 make_move_iterator (b + nv.size ()));
+    }
+
+    v = nullptr;                   // Free old data.
+    v.type = nullptr;              // Change type.
+    v.assign (move (ns), nullptr); // Assign new data.
+  }
+
   // Throw invalid_argument for an invalid simple value.
   //
   [[noreturn]] static void
@@ -736,6 +771,34 @@ namespace build2
   }
 
   void
+  process_path_assign (value& v, names&& ns, const variable* var)
+  {
+    using traits = value_traits<process_path>;
+
+    size_t n (ns.size ());
+
+    if (n <= 2)
+    {
+      try
+      {
+        traits::assign (
+          v,
+          (n == 0
+           ? process_path ()
+           : traits::convert (move (ns[0]), n == 2 ? &ns[1] : nullptr)));
+        return;
+      }
+      catch (const invalid_argument&) {} // Fall through.
+    }
+
+    diag_record dr (fail);
+    dr << "invalid process_path value '" << ns << "'";
+
+    if (var != nullptr)
+      dr << " in variable " << var->name;
+  }
+
+  void
   process_path_copy_ctor (value& l, const value& r, bool m)
   {
     const auto& rhs (r.as<process_path> ());
@@ -798,15 +861,15 @@ namespace build2
   {
     type_name,
     sizeof (process_path),
-    nullptr,                            // No base.
+    nullptr,                         // No base.
     &default_dtor<process_path>,
     &process_path_copy_ctor,
     &process_path_copy_assign,
-    &simple_assign<process_path, true>, // Allow empty values.
-    nullptr,                            // Append not supported.
-    nullptr,                            // Prepend not supported.
+    &process_path_assign,
+    nullptr,                         // Append not supported.
+    nullptr,                         // Prepend not supported.
     &process_path_reverse,
-    nullptr,                            // No cast (cast data_ directly).
+    nullptr,                         // No cast (cast data_ directly).
     &simple_compare<process_path>,
     &default_empty<process_path>
   };
diff --git a/tests/buildfile b/tests/buildfile
index f38b3ca..405df8f 100644
--- a/tests/buildfile
+++ b/tests/buildfile
@@ -2,6 +2,6 @@
 # copyright : Copyright (c) 2014-2016 Code Synthesis Ltd
 # license   : MIT; see accompanying LICENSE file
 
-d = directive/ function/ test/
+d = directive/ expansion/ function/ test/
 ./: $d
 include $d
diff --git a/tests/expansion/buildfile b/tests/expansion/buildfile
new file mode 100644
index 0000000..81df59e
--- /dev/null
+++ b/tests/expansion/buildfile
@@ -0,0 +1,7 @@
+# file      : tests/expansion/buildfile
+# copyright : Copyright (c) 2014-2016 Code Synthesis Ltd
+# license   : MIT; see accompanying LICENSE file
+
+./: test{type} file{common.test}
+
+test{*}: test = $effect($build.path)
diff --git a/tests/expansion/common.test b/tests/expansion/common.test
new file mode 100644
index 0000000..881c74e
--- /dev/null
+++ b/tests/expansion/common.test
@@ -0,0 +1,11 @@
+# file      : tests/expansion/assert.test
+# copyright : Copyright (c) 2014-2016 Code Synthesis Ltd
+# license   : MIT; see accompanying LICENSE file
+
++mkdir build
++cat <<EOI >>>build/bootstrap.build
+project = test
+amalgamation =
+EOI
+
+test.options += -q --buildfile - noop
diff --git a/tests/expansion/type.test b/tests/expansion/type.test
new file mode 100644
index 0000000..1aae5b6
--- /dev/null
+++ b/tests/expansion/type.test
@@ -0,0 +1,53 @@
+# file      : tests/expansion/type.test
+# copyright : Copyright (c) 2014-2016 Code Synthesis Ltd
+# license   : MIT; see accompanying LICENSE file
+
+# Test type propagation during expansion.
+
+.include common.test
+
+: var
+:
+$* <<EOI
+x = [bool] true
+y = \$x
+assert \(\$type\(\$y) == bool)
+EOI
+
+: eval
+:
+$* <<EOI
+y = \([bool] true)
+assert \(\$type\(\$y) == bool)
+EOI
+
+: func
+:
+$* <<EOI
+y = \$identity\([bool] true)
+assert \(\$type\(\$y) == bool)
+EOI
+
+: untypify
+:
+$* <<EOI
+x = [bool] true
+y = "\$x"
+assert \(\$type\(\$y) == "")
+EOI
+
+: type-conflict
+:
+$* <'print [bool] ([string] true)' 2>>EOE != 0
+<stdin>:1:7: error: conflicting attribute type bool and value type string
+  info: use quoting to untypify the value
+EOE
+
+: retypify
+:
+$* <'print [bool] "([string] true)"' >'true'
+
+: retypify-name
+: Test the "steal" case of untypify()
+:
+$* <'print [bool] "([name] true)"' >'true'
diff --git a/tests/test/script/builtin/touch.test b/tests/test/script/builtin/touch.test
index 4d2ff57..ef950ea 100644
--- a/tests/test/script/builtin/touch.test
+++ b/tests/test/script/builtin/touch.test
@@ -39,7 +39,7 @@ touch '' 2>"touch: invalid path ''" == 1
 :
 : Test touching an existing directory.
 :
-a = [path] $~;
+a = $~;
 a += "a";
 mkdir a;
 touch 2>"touch: '$a' exists and is not a file" a == 1
diff --git a/tests/variable/dir-path/buildfile b/tests/variable/dir-path/buildfile
index 5c99f07..c6fdb46 100644
--- a/tests/variable/dir-path/buildfile
+++ b/tests/variable/dir-path/buildfile
@@ -27,7 +27,7 @@ print [strings] -I $y
 print -I $y/baz
 print
 
-z = [strings] $x   # Re-typed.
+z = [strings] "$x"   # Re-typed.
 print $z
 print
 
@@ -37,9 +37,9 @@ r = [dir_path] /
 print $r/foo
 
 r += foo
-print [strings] $r
+print [strings] "$r"
 
 r += bar
-print [strings] $r
+print [strings] "$r"
 
 ./:
diff --git a/unit-tests/function/call.test b/unit-tests/function/call.test
index 003a828..2b91cc3 100644
--- a/unit-tests/function/call.test
+++ b/unit-tests/function/call.test
@@ -80,11 +80,11 @@ $* <'print $optional(abc)' >'false'
 
 : null-true
 :
-$* <'print $null([null])' >'true'
+$* <'print $nullable([null])' >'true'
 
 : null-false
 :
-$* <'print $null(nonull)' >'false'
+$* <'print $nullable(nonull)' >'false'
 
 : null-fail
 :
diff --git a/unit-tests/function/driver.cxx b/unit-tests/function/driver.cxx
index 6677c24..3dea374 100644
--- a/unit-tests/function/driver.cxx
+++ b/unit-tests/function/driver.cxx
@@ -33,7 +33,7 @@ namespace build2
     f["fail"]     = []()        {fail << "failed" << endf;};
     f["fail_arg"] = [](names a) {return convert<uint64_t> (move (a[0]));};
 
-    f["null"]     = [](names* a)          {return a == nullptr;};
+    f["nullable"] = [](names* a)          {return a == nullptr;};
     f["optional"] = [](optional<names> a) {return !a;};
 
     f["dummy0"] = []()         {return "abc";};