Test and fix override logic

8 files changed, 451 insertions, 44 deletions

diff --git a/build2/scope.cxx b/build2/scope.cxx
index ad251b5..3fd3c95 100644
--- a/build2/scope.cxx
+++ b/build2/scope.cxx
@@ -78,6 +78,10 @@ namespace build2
     size_t depth (0);
     const variable_map* vars (nullptr);
 
+    // Root scope of a project from which our initial value comes. See below.
+    //
+    const scope* proj (nullptr);
+
     // One special case is if the original is target-specific, which is the
     // most innermost. Or is it innermostest?
     //
@@ -88,8 +92,9 @@ namespace build2
 
       if (targetspec)
       {
-        vars = origl.vars;
         depth = origd;
+        vars = origl.vars;
+        proj = root_scope ();
       }
       else
         depth = 2; // For implied target-specific lookup.
@@ -97,51 +102,53 @@ namespace build2
 
     const scope* s;
 
-    // Return override value if it is present, applies, and ends with suffix.
+    // Return true if the override applies. Note that it expects vars and proj
+    // to be not NULL; if there is nothing "inner", then any override will
+    // still be "visible".
     //
-    auto find = [&vars, &s, this] (const variable* o, const char* sf = nullptr)
-      -> lookup
+    auto applies = [&vars, &s] (const variable* o, const scope* proj) -> bool
     {
-      if (sf != nullptr && o->name.rfind (sf) == string::npos)
-        return lookup ();
-
-      // Next see if it would apply. If there is nothing "inner", then any
-      // override will still be "visible".
-      //
-      if (vars != nullptr)
+      switch (o->visibility)
       {
-        switch (o->visibility)
-        {
-        case variable_visibility::scope:
-        {
-          // Does not apply if the innermost value is not in this scope.
-          //
-          if (vars != &s->vars)
-            return lookup ();
+      case variable_visibility::scope:
+      {
+        // Does not apply if the innermost value is not in this scope.
+        //
+        if (vars != &s->vars)
+          return false;
 
-          break;
-        }
-        case variable_visibility::project:
-        {
-          // Does not apply if in a different project.
-          //
-          if (root_scope () != s->root_scope ())
-            return lookup ();
+        break;
+      }
+      case variable_visibility::project:
+      {
+        // Does not apply if in a different project.
+        //
+        if (proj != s->root_scope ())
+          return false;
 
-          break;
-        }
-        case variable_visibility::normal:
-          break;
-        }
+        break;
+      }
+      case variable_visibility::normal:
+        break;
       }
 
+      return true;
+    };
+
+    // Return override value if it is present and optionally ends with suffix.
+    //
+    auto find = [&s] (const variable* o, const char* sf = nullptr) -> lookup
+    {
+      if (sf != nullptr && o->name.rfind (sf) == string::npos)
+        return lookup ();
+
       return lookup (s->vars.find (*o), &s->vars);
     };
 
     // Return true if a value is from this scope (either target type/pattern-
     // specific or ordinary).
     //
-    auto test = [&s] (const lookup& l) -> bool
+    auto belongs = [&s] (const lookup& l) -> bool
     {
       for (auto& p1: s->target_vars)
         for (auto& p2: p1.second)
@@ -163,10 +170,11 @@ namespace build2
       // the target type/patter-specific variables, which is "more inner" than
       // normal scope variables (see find_original()).
       //
-      if (vars == nullptr && origl.defined () && test (origl))
+      if (vars == nullptr && origl.defined () && belongs (origl))
       {
-        vars = origl.vars;
         depth = origd;
+        vars = origl.vars;
+        proj = s->root_scope ();
       }
 
       if (vars == nullptr)
@@ -178,6 +186,9 @@ namespace build2
            o != nullptr;
            o = o->override.get ())
       {
+        if (vars != 0 && !applies (o, proj))
+          continue;
+
         auto l (find (o));
 
         if (l.defined ())
@@ -215,19 +226,21 @@ namespace build2
 
     // Now find our "stem", that is the value to which we will be appending
     // suffixes and prepending prefixes. This is either the original or the
-    // __override, depending on which one is the innermost. We may also not
-    // have one at all.
+    // __override provided it applies. We may also not have either.
     //
     lookup stem (targetspec ? origl : lookup ());
 
     for (s = this; s != nullptr; s = s->parent_scope ())
     {
+      bool done (false);
+
       // First check if the original is from this scope.
       //
-      if (origl.defined () && test (origl))
+      if (origl.defined () && belongs (origl))
       {
         stem = origl;
-        break;
+        proj = s->root_scope ();
+        // Keep searching.
       }
 
       // Then look for an __override that applies.
@@ -236,13 +249,24 @@ namespace build2
            o != nullptr;
            o = o->override.get ())
       {
-        stem = find (o, ".__override");
+        // If we haven't yet found anything, then any override will still be
+        // "visible" even if it doesn't apply.
+        //
+        if (stem.defined () && !applies (o, root_scope ()))
+          continue;
 
-        if (stem.defined ())
+        auto l (find (o, ".__override"));
+
+        if (l.defined ())
+        {
+          stem = move (l);
+          proj = s->root_scope ();
+          done = true;
           break;
+        }
       }
 
-      if (stem.defined ())
+      if (done)
         break;
     }
 
@@ -283,18 +307,35 @@ namespace build2
            o != nullptr;
            o = o->override.get ())
       {
+        // First see if this override applies. This is actually tricky: what
+        // if the stem is a "visible" override from an outer project?
+        // Shouldn't its overrides apply? Sure sounds logical. So it seems
+        // we should use the project of the stem's scope and not the project
+        // of this scope.
+        //
+        if (proj != nullptr && !applies (o, proj))
+          continue;
+
         // Note that we keep override values as untyped names even if the
         // variable itself is typed. We also pass the original variable for
         // diagnostics.
         //
-        if (auto l = find (o, ".__prefix")) // No sense if NULL.
+        auto l (find (o, ".__prefix"));
+
+        if (l) // No sense to prepend/append if NULL.
         {
           cache.value.prepend (names (cast<names> (l)), var);
         }
-        else if (auto l = find (o, ".__suffix")) // No sense if NULL.
+        else if ((l = find (o, ".__suffix")))
         {
           cache.value.append (names (cast<names> (l)), var);
         }
+
+        // If we had no stem, use the scope of the first override that applies
+        // as the project.
+        //
+        if (proj == nullptr && l.defined ())
+          proj = s;
       }
     }
 
diff --git a/tests/variable/override/build/bootstrap.build b/tests/variable/override/build/bootstrap.build
new file mode 100644
index 0000000..1c2e239
--- /dev/null
+++ b/tests/variable/override/build/bootstrap.build
@@ -0,0 +1,2 @@
+project = override
+amalgamation = # Disabled.
diff --git a/tests/variable/override/buildfile b/tests/variable/override/buildfile
new file mode 100644
index 0000000..18c9831
--- /dev/null
+++ b/tests/variable/override/buildfile
@@ -0,0 +1,58 @@
+if ("$t" != "")
+{
+  [$t] v = [null]
+}
+
+print "/     :" $(/: v)
+
+if ($a == as)
+{
+  v = x
+}
+elif ($a == ap)
+{
+  v += s
+}
+elif ($a == pr)
+{
+  v =+ p
+}
+
+print ".     :" $v
+
+d/:
+{
+  if ($d_a == as)
+  {
+    v = x
+  }
+  elif ($d_a == ap)
+  {
+    v += s
+  }
+  elif ($d_a == pr)
+  {
+    v =+ p
+  }
+
+  print "d     :" $v
+
+
+  if ($d_t_a == as)
+  {
+    file{t}: v = x
+  }
+  elif ($d_t_a == ap)
+  {
+    file{t}: v += s
+  }
+  elif ($d_t_a == pr)
+  {
+    file{t}: v =+ p
+  }
+
+  print "d/t   :" $(file{t}: v)
+}
+
+include p/
+./:
diff --git a/tests/variable/override/cache b/tests/variable/override/cache
new file mode 100644
index 0000000..8378688
--- /dev/null
+++ b/tests/variable/override/cache
@@ -0,0 +1,13 @@
+x = [string] 0
+print $x
+
+x = [uint64] 1
+print $x
+
+y = 0
+print $y
+
+[uint64] y = [null]
+print $y
+
+./:
diff --git a/tests/variable/override/p/build/bootstrap.build b/tests/variable/override/p/build/bootstrap.build
new file mode 100644
index 0000000..723e2a3
--- /dev/null
+++ b/tests/variable/override/p/build/bootstrap.build
@@ -0,0 +1 @@
+project = override-p
diff --git a/tests/variable/override/p/buildfile b/tests/variable/override/p/buildfile
new file mode 100644
index 0000000..527b9ae
--- /dev/null
+++ b/tests/variable/override/p/buildfile
@@ -0,0 +1,49 @@
+if ($p_a == as)
+{
+  v = x
+}
+elif ($p_a == ap)
+{
+  v += s
+}
+elif ($p_a == pr)
+{
+  v =+ p
+}
+
+print "p     :" $v
+
+d/:
+{
+  if ($p_d_a == as)
+  {
+    v = x
+  }
+  elif ($p_d_a == ap)
+  {
+    v += s
+  }
+  elif ($p_d_a == pr)
+  {
+    v =+ p
+  }
+
+  print "p/d   :" $v
+
+  if ($p_d_t_a == as)
+  {
+    file{t}: v = x
+  }
+  elif ($p_d_t_a == ap)
+  {
+    file{t}: v += s
+  }
+  elif ($p_d_t_a == pr)
+  {
+    file{t}: v =+ p
+  }
+
+  print "p/d/t :" $(file{t}: v)
+}
+
+./:
diff --git a/tests/variable/override/simple b/tests/variable/override/simple
new file mode 100644
index 0000000..899daa2
--- /dev/null
+++ b/tests/variable/override/simple
@@ -0,0 +1,3 @@
+print $foo
+
+./:
diff --git a/tests/variable/override/test.sh b/tests/variable/override/test.sh
new file mode 100755
index 0000000..a195766
--- /dev/null
+++ b/tests/variable/override/test.sh
@@ -0,0 +1,240 @@
+#! /usr/bin/env bash
+
+verbose=n
+
+function error () { echo "$*" 1>&2; exit 1; }
+
+function fail ()
+{
+  if [ "$verbose" = "y" ]; then
+    b $*
+  else
+    b -q $* 2>/dev/null
+  fi
+
+  if [ $? -eq 0 ]; then
+    error "succeeded: b $*"
+  fi
+
+  return 0
+}
+
+function test ()
+{
+  # There is no way to get the exit code in process substitution
+  # so ruin the output.
+  #
+  diff -u - <(b -q $* || echo "<invalid output>")
+
+  if [ $? -ne 0 ]; then
+    error "failed: b $*"
+  fi
+}
+
+fail foo=bar[]             # error: unexpected [ in variable assignment 'foo=bar[]'
+fail foo=[string]bar       # error: typed override of variable foo
+fail "!foo=bar" "!foo=BAR" # error: multiple global overrides of variable foo
+fail "foo=bar" "foo=BAR"   # error: multiple project overrides of variable foo
+fail "%foo=bar" "%foo=BAR" # error: multiple project overrides of variable foo
+
+test --buildfile simple foo=bar ./ ./ <<< "bar"  # Multiple bootstraps of the same project.
+
+# Visibility.
+#
+test !v=X <<EOF
+/     : X
+.     : X
+d     : X
+d/t   : X
+p     : X
+p/d   : X
+p/d/t : X
+EOF
+
+test v=X <<EOF
+/     :
+.     : X
+d     : X
+d/t   : X
+p     : X
+p/d   : X
+p/d/t : X
+EOF
+
+test v=X p_a=assing <<EOF
+/     :
+.     : X
+d     : X
+d/t   : X
+p     : X
+p/d   : X
+p/d/t : X
+EOF
+
+test %v=X <<EOF
+/     :
+.     : X
+d     : X
+d/t   : X
+p     : X
+p/d   : X
+p/d/t : X
+EOF
+
+test %v=X p_a=as <<EOF
+/     :
+.     : X
+d     : X
+d/t   : X
+p     : x
+p/d   : x
+p/d/t : x
+EOF
+
+test %v+=S %v=+P a=as <<EOF
+/     :
+.     : P x S
+d     : P x S
+d/t   : P x S
+p     : P x S
+p/d   : P x S
+p/d/t : P x S
+EOF
+
+test %v+=S %v=+P a=as p_a=as <<EOF
+/     :
+.     : P x S
+d     : P x S
+d/t   : P x S
+p     : x
+p/d   : x
+p/d/t : x
+EOF
+
+# Append/Prepend in override.
+#
+test v+=S <<EOF
+/     :
+.     : S
+d     : S
+d/t   : S
+p     : S
+p/d   : S
+p/d/t : S
+EOF
+
+test v+=S a=as <<EOF
+/     :
+.     : x S
+d     : x S
+d/t   : x S
+p     : x S
+p/d   : x S
+p/d/t : x S
+EOF
+
+test %v=+P a=as p_a=as <<EOF
+/     :
+.     : P x
+d     : P x
+d/t   : P x
+p     : x
+p/d   : x
+p/d/t : x
+EOF
+
+test %v+=S v=+P a=as p_a=as <<EOF
+/     :
+.     : P x S
+d     : P x S
+d/t   : P x S
+p     : P x
+p/d   : P x
+p/d/t : P x
+EOF
+
+# Append/Prepend in both.
+#
+test v=X a=ap d_a=ap p_a=ap p_d_a=ap <<EOF
+/     :
+.     : X
+d     : X
+d/t   : X
+p     : X
+p/d   : X
+p/d/t : X
+EOF
+
+test v+=S v=+P a=as d_a=ap d_t_a=ap p_a=ap p_d_a=ap p_d_t_a=ap <<EOF
+/     :
+.     : P x S
+d     : P x s S
+d/t   : P x s s S
+p     : P x s S
+p/d   : P x s s S
+p/d/t : P x s s s S
+EOF
+
+# These ones are surprising. I guess the moral is we shouldn't do "blind"
+# cross-project append/prepend.
+#
+test %v=X a=as d_a=ap p_a=ap p_d_a=ap <<EOF
+/     :
+.     : X
+d     : X
+d/t   : X
+p     : x s
+p/d   : x s s
+p/d/t : x s s
+EOF
+
+test %v+=S a=as d_a=ap p_a=ap p_d_a=ap <<EOF
+/     :
+.     : x S
+d     : x s S
+d/t   : x s S
+p     : x s
+p/d   : x s s
+p/d/t : x s s
+EOF
+
+test %v+=S a=as d_a=ap p_a=ap p_d_a=ap ./ p/ <<EOF
+/     :
+.     : x S
+d     : x s S
+d/t   : x s S
+p     : x s S
+p/d   : x s s S
+p/d/t : x s s S
+EOF
+
+# Typed override.
+#
+test v+=S v=+P t=string <<EOF
+/     :
+.     : PS
+d     : PS
+d/t   : PS
+p     : PS
+p/d   : PS
+p/d/t : PS
+EOF
+
+test v+=S v=+P t=string a=as d_a=ap d_t_a=ap p_a=ap p_d_a=ap p_d_t_a=ap <<EOF
+/     :
+.     : PxS
+d     : PxsS
+d/t   : PxssS
+p     : PxsS
+p/d   : PxssS
+p/d/t : PxsssS
+EOF
+
+# Cache overwrite.
+#
+test --buildfile cache x+=01 y+=01 <<EOF
+001
+2
+0 01
+1
+EOF