Adjust variable block applicability in dependency chains

Before the block used to apply to the set of prerequisites before the last
`:`. This turned out to be counterintuitive and not very useful since
prerequisite-specific variables are a lot less common than target specific.
And it doesn't fit with ad hoc recipes.

The new rule is if the chain ends with `:`, then the block applies to the last
set of prerequisites. Otherwise, it applies to the last set of targets. For
example:

./: exe{test}: cxx{main}
{
  test = true        # Applies to the exe{test} target.
}

./: exe{test}: libue{test}:
{
  bin.whole = false  # Applies to the libue{test} prerequisite.
}

This is actually consistent with both non-chain and non-block cases.
Consider:

exe{test}: cxx{main}
{
  test = true
}

exe{test}: libue{test}:
{
  bin.whole = false
}

exe{test}: libue{test}: bin.whole = false

The only exception we now have in this overall approach of "if the
dependency declaration ends with a colon, then what follows is for a
prerequisite" is for the first semicolon:

exe{test}:
{
  test = true
}

exe{test}: test = true

But that's probably intuitive enough since there cannot be a prerequisite
without a target.

5 files changed, 228 insertions, 112 deletions

diff --git a/doc/manual.cli b/doc/manual.cli
index cf2ec3a..ba0aa63 100644
--- a/doc/manual.cli
+++ b/doc/manual.cli
@@ -3259,11 +3259,17 @@ h{config}: in{config}
 }
 \
 
-In case of a dependency chain, the block applies to the set of prerequisites
-(note: \i{not targets}) before last \c{:}. For example:
+In case of a dependency chain, if the chain ends with a colon (\c{:}), then
+the block applies to the last set of prerequisites. Otherwise, it applies to
+the last set of targets. For example:
 
 \
-./: exe{test}: libue{test}: cxx{test}
+./: exe{test}: cxx{main}
+{
+  test = true        # Applies to the exe{test} target.
+}
+
+./: exe{test}: libue{test}:
 {
   bin.whole = false  # Applies to the libue{test} prerequisite.
 }
diff --git a/libbuild2/parser.cxx b/libbuild2/parser.cxx
index 21b5794..b6004de 100644
--- a/libbuild2/parser.cxx
+++ b/libbuild2/parser.cxx
@@ -706,8 +706,18 @@ namespace build2
           // Note that we cannot just let parse_dependency() handle this case
           // because we can have (a mixture of) target type/patterns.
           //
-          // @@ This might change once we support ad hoc rules (where we may
-          // have prerequisites for a pattern; but perhaps this should be
+          // Also, it handles the exception to the rule that if a dependency
+          // declaration ends with a colon, then the variable assignment/block
+          // that follows is for the prerequisite. Compare:
+          //
+          // foo: x = y         foo: fox: x = y
+          // bar:               bar: baz:
+          // {                  {
+          //   x = y              x = y
+          // }                  }
+          //
+          // @@ This might change a bit once we support ad hoc rules (where we
+          // may have prerequisites for a pattern; but perhaps this should be
           // handled separately since the parse_dependency() is already too
           // complex and there will be no chains in this case).
           //
@@ -808,6 +818,9 @@ namespace build2
           const variable& var (parse_variable_name (move (pns), ploc));
           apply_variable_attributes (var);
 
+          // If variable visibility ends before, then it doesn't make sense
+          // to assign it in this context.
+          //
           if (var.visibility > variable_visibility::target)
           {
             fail (nloc) << "variable " << var << " has " << var.visibility
@@ -841,11 +854,10 @@ namespace build2
           else
             attributes_pop ();
 
-          bool r (parse_dependency (t, tt,
-                                    move (ns), nloc,
-                                    move (ans),
-                                    move (pns), ploc));
-          assert (r); // Block must have been claimed.
+          parse_dependency (t, tt,
+                            move (ns), nloc,
+                            move (ans),
+                            move (pns), ploc);
         }
 
         continue;
@@ -899,7 +911,7 @@ namespace build2
         const variable& var (parse_variable_name (move (ns), nloc));
         apply_variable_attributes (var);
 
-        if (var.visibility >= variable_visibility::target)
+        if (var.visibility > variable_visibility::scope)
         {
           diag_record dr (fail (nloc));
 
@@ -1021,7 +1033,7 @@ namespace build2
       const variable& var (parse_variable_name (move (ns), nloc));
       apply_variable_attributes (var);
 
-      if (prerequisite_ != nullptr                   &&
+      if (prerequisite_ == nullptr                   &&
           var.visibility > variable_visibility::target)
       {
         fail (t) << "variable " << var << " has " << var.visibility
@@ -1565,30 +1577,17 @@ namespace build2
     return tgs;
   }
 
-  bool parser::
+  void parser::
   parse_dependency (token& t, token_type& tt,
                     names&& tns, const location& tloc, // Target names.
                     adhoc_names&& ans,                 // Ad hoc target names.
-                    names&& pns, const location& ploc, // Prereq names.
-                    bool chain)
+                    names&& pns, const location& ploc) // Prereq names.
   {
     // Parse a dependency chain and/or a target/prerequisite-specific variable
-    // assignment/block and/or recipe block(s). Return true if the following
-    // block(s) (if any) have been "claimed", meaning they "belong" to
-    // targets/prerequisites before the last colon.
-    //
-    // enter: colon (anything else is not handled)
-    // leave: - first token on the next line           if returning true
-    //        - newline (presumably, must be verified) if returning false
-    //
-    // Note that top-level call (with chain == false) is expected to always
-    // return true.
-    //
-    // This dual-return "complication" is necessary to handle non-block cases
-    // like this:
+    // assignment/block and/or recipe block(s).
     //
-    // foo: bar
-    // {hxx ixx}: install = true
+    // enter: colon or newline (latter only in recursive calls)
+    // leave: - first token on the next line
     //
     tracer trace ("parser::parse_dependency", &path_);
 
@@ -1700,17 +1699,18 @@ namespace build2
     };
 
     // Do we have a dependency chain and/or prerequisite-specific variable
-    // assignment? If not, check for the target-specific variable block and/or
-    // recipe block(s) unless this is a chained call (in which case the block,
-    // if any, "belongs" to prerequisites).
+    // assignment/block? If not, check for the target-specific variable block
+    // and/or recipe block(s).
     //
     if (tt != type::colon)
     {
-      if (chain)
-        return false;
-
       next_after_newline (t, tt); // Must be a newline then.
 
+      // Note: watch out for non-block cases like this:
+      //
+      // foo: bar
+      // {hxx ixx}: install = true
+      //
       if (tt == type::percent       ||
           tt == type::multi_lcbrace ||
           (tt == type::lcbrace && peek () == type::newline))
@@ -1755,7 +1755,7 @@ namespace build2
         for_each_t (parse);
       }
 
-      return true; // Claimed or isn't any.
+      return;
     }
 
     // What should we do if there are no prerequisites (for example, because
@@ -1773,97 +1773,113 @@ namespace build2
 
     auto at (attributes_push (t, tt));
 
-    // @@ PAT: currently we pattern-expand prerequisite-specific vars.
+    // If we are here, then this can be one of three things:
     //
-    const location loc (get_location (t));
-    names ns (tt != type::newline && tt != type::eos
-              ? parse_names (t, tt, pattern_mode::expand)
-              : names ());
-
-    // Prerequisite-specific variable assignment.
+    // 1. A prerequisite-specific variable bloc:
     //
-    if (tt == type::assign || tt == type::prepend || tt == type::append)
+    //    foo: bar:
+    //    {
+    //      x = y
+    //    }
+    //
+    // 2. A prerequisite-specific variable asignment:
+    //
+    //    foo: bar: x = y
+    //
+    // 3. A further dependency chain :
+    //
+    //    foo: bar: baz ...
+    //
+    if (tt == type::newline || tt == type::eos)
     {
-      type at (tt);
-
-      const variable& var (parse_variable_name (move (ns), loc));
-      apply_variable_attributes (var);
+      attributes_pop (); // Must be none since can't be standalone.
 
-      // Parse the assignment for each prerequisites of each target.
+      // There must be a block.
       //
-      for_each_p ([this, &var, at] (token& t, token_type& tt)
-                {
-                  parse_variable (t, tt, var, at);
-                });
+      if (next_after_newline (t, tt) != type::lcbrace)
+        fail (t) << "expected '{' instead of " << t;
 
-      // Pretend that we have claimed the block to cause an error if there is
-      // one. Failed that, the following would result in a valid (target-
-      // specific) block:
-      //
-      // foo: bar: x = y
-      // {
-      //   ...
-      // }
+      if (next (t, tt) != type::newline)
+        fail (t) << "expected newline after '{'";
+
+      // Parse the block for each prerequisites of each target.
       //
-      next_after_newline (t, tt);
-      return true;
+      for_each_p ([this] (token& t, token_type& tt)
+                  {
+                    next (t, tt); // First token inside the block.
+
+                    parse_variable_block (t, tt);
+
+                    if (tt != type::rcbrace)
+                      fail (t) << "expected '}' instead of " << t;
+                  });
+
+      next (t, tt);                    // Presumably newline after '}'.
+      next_after_newline (t, tt, '}'); // Should be on its own line.
     }
-    //
-    // Dependency chain.
-    //
     else
     {
-      if (at.first)
-        fail (at.second) << "attributes before prerequisites";
-      else
-        attributes_pop ();
-
-      // Note that we could have "pre-resolved" these prerequisites to actual
-      // targets or, at least, made their directories absolute. We don't do it
-      // for ease of documentation: with the current semantics we can just say
-      // that the dependency chain is equivalent to specifying each dependency
-      // separately.
-      //
-      // Also note that supporting ad hoc target group specification in chains
-      // will be complicated. For example, what if prerequisites that have ad
-      // hoc targets don't end up being chained? Do we just silently drop
-      // them? Also, these are prerequsites first that happened to be reused
-      // as target names so perhaps it is the right thing not to support,
-      // conceptually.
+      // @@ PAT: currently we pattern-expand prerequisite-specific vars.
       //
-      if (parse_dependency (t, tt,
-                            names (pns), ploc,           // Note: can't move.
-                            {} /* ad hoc target name */,
-                            move (ns), loc,
-                            true /* chain */))
-        return true;
+      const location loc (get_location (t));
+      names ns (parse_names (t, tt, pattern_mode::expand));
 
-      // Claim the block (if any) for these prerequisites if it hasn't been
-      // claimed by the inner ones.
+      // Prerequisite-specific variable assignment.
       //
-      next_after_newline (t, tt); // Must be a newline.
-
-      if (tt == type::lcbrace && peek () == type::newline)
+      if (tt == type::assign || tt == type::prepend || tt == type::append)
       {
-        next (t, tt); // Newline.
+        type at (tt);
+
+        const variable& var (parse_variable_name (move (ns), loc));
+        apply_variable_attributes (var);
 
-        // Parse the block for each prerequisites of each target.
+        // Parse the assignment for each prerequisites of each target.
         //
-        for_each_p ([this] (token& t, token_type& tt)
+        for_each_p ([this, &var, at] (token& t, token_type& tt)
                     {
-                      next (t, tt); // First token inside the block.
-
-                      parse_variable_block (t, tt);
-
-                      if (tt != type::rcbrace)
-                        fail (t) << "expected '}' instead of " << t;
+                      parse_variable (t, tt, var, at);
                     });
 
-        next (t, tt);                    // Presumably newline after '}'.
-        next_after_newline (t, tt, '}'); // Should be on its own line.
+        next_after_newline (t, tt);
+
+        // Check we don't also have a variable block:
+        //
+        // foo: bar: x = y
+        // {
+        //   ...
+        // }
+        //
+        if (tt == type::lcbrace && peek () == type::newline)
+          fail (t) << "variable assignment block after variable assignment";
       }
+      //
+      // Dependency chain.
+      //
+      else
+      {
+        if (at.first)
+          fail (at.second) << "attributes before prerequisites";
+        else
+          attributes_pop ();
 
-      return true; // Claimed or isn't any.
+        // Note that we could have "pre-resolved" these prerequisites to
+        // actual targets or, at least, made their directories absolute. We
+        // don't do it for ease of documentation: with the current semantics
+        // we just say that the dependency chain is equivalent to specifying
+        // each dependency separately.
+        //
+        // Also note that supporting ad hoc target group specification in
+        // chains will be complicated. For example, what if prerequisites that
+        // have ad hoc targets don't end up being chained? Do we just silently
+        // drop them? Also, these are prerequsites first that happened to be
+        // reused as target names so perhaps it is the right thing not to
+        // support, conceptually.
+        //
+        parse_dependency (t, tt,
+                          move (pns), ploc,
+                          {} /* ad hoc target name */,
+                          move (ns), loc);
+      }
     }
   }
 
@@ -2590,7 +2606,7 @@ namespace build2
     {
       apply_variable_attributes (*var);
 
-      if (var->visibility >= variable_visibility::target)
+      if (var->visibility > variable_visibility::scope)
       {
         fail (vloc) << "variable " << *var << " has " << var->visibility
                     << " visibility but is assigned in import";
@@ -3323,7 +3339,7 @@ namespace build2
     const variable& var (parse_variable_name (move (vns), vloc));
     apply_variable_attributes (var);
 
-    if (var.visibility >= variable_visibility::target)
+    if (var.visibility > variable_visibility::scope)
     {
       fail (vloc) << "variable " << var << " has " << var.visibility
                   << " visibility but is assigned in for-loop";
diff --git a/libbuild2/parser.hxx b/libbuild2/parser.hxx
index 2db7ade..24a1ac1 100644
--- a/libbuild2/parser.hxx
+++ b/libbuild2/parser.hxx
@@ -152,12 +152,11 @@ namespace build2
     small_vector<reference_wrapper<target>, 1>
     enter_targets (names&&, const location&, adhoc_names&&, size_t);
 
-    bool
+    void
     parse_dependency (token&, token_type&,
                       names&&, const location&,
                       adhoc_names&&,
-                      names&&, const location&,
-                      bool = false);
+                      names&&, const location&);
 
     void
     parse_assert (token&, token_type&);
diff --git a/tests/dependency/chain/testscript b/tests/dependency/chain/testscript
index ac4a946..a24ace3 100644
--- a/tests/dependency/chain/testscript
+++ b/tests/dependency/chain/testscript
@@ -36,3 +36,52 @@ EOI
 
 %  .+/dir\{y/\}: .+:dir\{a/\} .+:dir\{b/\}%
 EOE
+
+: var-prereq
+:
+$* <<EOI 2>>/~%EOE%
+./: dir{x}: dir{a}: x = y
+dump dir{x}
+EOI
+<stdin>:2:1: dump:
+%  .+/dir\{x/\}: .+:dir\{a/\}:%
+  {
+    x = y
+  }
+EOE
+
+: var-prereq-block
+:
+$* <<EOI 2>>/~%EOE%
+./: dir{x}: dir{a}:
+{
+  x = y
+  z = x
+}
+dump dir{x}
+EOI
+<stdin>:6:1: dump:
+%  .+/dir\{x/\}: .+:dir\{a/\}:%
+  {
+    x = y
+    z = x
+  }
+EOE
+
+: var-target-block
+:
+$* <<EOI 2>>/~%EOE%
+./: dir{x}: dir{a}
+{
+  x = y
+  z = x
+}
+dump dir{x}
+EOI
+<stdin>:6:1: dump:
+%  .+/dir\{x/\}: .+:dir\{a/\}%
+  {
+    x = y
+    z = x
+  }
+EOE
diff --git a/tests/dependency/recipe/testscript b/tests/dependency/recipe/testscript
index bd33bcc..43ec514 100644
--- a/tests/dependency/recipe/testscript
+++ b/tests/dependency/recipe/testscript
@@ -139,6 +139,52 @@ EOI
   }}
 EOE
 
+: with-vars-depchain
+:
+$* <<EOI 2>>/~%EOE%
+./: alias{x}: alias{y}
+{
+  var = x
+}
+{{
+  echo
+}}
+dump alias{x}
+EOI
+<stdin>:8:1: dump:
+%  .+/alias\{x\}: .+/:alias\{y\}%
+  {
+    var = x
+  }
+  % [diag=echo] perform(update)
+  {{
+    echo
+  }}
+EOE
+
+: with-vars-replay-depchain
+:
+$* <<EOI 2>>/~%EOE%
+./: alias{x y}: alias{z}
+{
+  var = x
+}
+{{
+  echo
+}}
+dump alias{y}
+EOI
+<stdin>:8:1: dump:
+%  .+/alias\{y\}: .+/:alias\{z\}%
+  {
+    var = x
+  }
+  % [diag=echo] perform(update)
+  {{
+    echo
+  }}
+EOE
+
 : with-vars-header
 :
 $* <<EOI 2>>/~%EOE%