Fail if scope or target qualification in variable expansion is unknown

There are three options here: we can "fall through" to an outer scope (there
is always the global scope backstop; this is the old semantics, sort of), we
can return NULL straight away, or we can fail. It feels like in most cases
unknown scope or target is a mistake and doing anything other than failing is
just making things harder to debug.

7 files changed, 103 insertions, 30 deletions

diff --git a/libbuild2/file.cxx b/libbuild2/file.cxx
index b93a20a..03dea5f 100644
--- a/libbuild2/file.cxx
+++ b/libbuild2/file.cxx
@@ -523,10 +523,14 @@ namespace build2
   pair<scope&, scope*>
   switch_scope (scope& root, const dir_path& out_base, bool proj)
   {
+    context& ctx (root.ctx);
+
+    assert (ctx.phase == run_phase::load);
+
     // First, enter the scope into the map and see if it is in any project. If
     // it is not, then there is nothing else to do.
     //
-    auto i (root.ctx.scopes.rw (root).insert_out (out_base));
+    auto i (ctx.scopes.rw (root).insert_out (out_base));
     scope& base (*i->second.front ());
 
     scope* rs (nullptr);
diff --git a/libbuild2/parser.cxx b/libbuild2/parser.cxx
index 29fe23f..13a3b12 100644
--- a/libbuild2/parser.cxx
+++ b/libbuild2/parser.cxx
@@ -59,27 +59,7 @@ namespace build2
     enter_scope (parser& p, dir_path&& d)
         : p_ (&p), r_ (p.root_), s_ (p.scope_), b_ (p.pbase_)
     {
-      // Try hard not to call normalize(). Most of the time we will go just
-      // one level deeper.
-      //
-      bool n (true);
-
-      if (d.relative ())
-      {
-        // Relative scopes are opened relative to out, not src.
-        //
-        if (d.simple () && !d.current () && !d.parent ())
-        {
-          d = dir_path (p.scope_->out_path ()) /= d.string ();
-          n = false;
-        }
-        else
-          d = p.scope_->out_path () / d;
-      }
-
-      if (n)
-        d.normalize ();
-
+      complete_normalize (*p.scope_, d);
       e_ = p.switch_scope (d);
     }
 
@@ -123,6 +103,31 @@ namespace build2
     enter_scope (const enter_scope&) = delete;
     enter_scope& operator= (const enter_scope&) = delete;
 
+    static void
+    complete_normalize (scope& s, dir_path& d)
+    {
+      // Try hard not to call normalize(). Most of the time we will go just
+      // one level deeper.
+      //
+      bool n (true);
+
+      if (d.relative ())
+      {
+        // Relative scopes are opened relative to out, not src.
+        //
+        if (d.simple () && !d.current () && !d.parent ())
+        {
+          d = dir_path (s.out_path ()) /= d.string ();
+          n = false;
+        }
+        else
+          d = s.out_path () / d;
+      }
+
+      if (n)
+        d.normalize ();
+    }
+
   private:
     parser* p_;
     scope* r_;
@@ -8318,6 +8323,9 @@ namespace build2
   lookup parser::
   lookup_variable (names&& qual, string&& name, const location& loc)
   {
+    // Note that this function can be called during execute (for example, from
+    // scripts). In particular, this means we cannot use enter_{scope,target}.
+
     if (pre_parse_)
       return lookup ();
 
@@ -8329,9 +8337,6 @@ namespace build2
 
     // If we are qualified, it can be a scope or a target.
     //
-    enter_scope sg;
-    enter_target tg;
-
     if (qual.empty ())
     {
       s = scope_;
@@ -8340,13 +8345,29 @@ namespace build2
     }
     else
     {
+      // What should we do if we cannot find the qualification (scope or
+      // target)? We can "fall through" to an outer scope (there is always the
+      // global scope backstop), we can return NULL straight away, or we can
+      // fail. It feels like in most cases unknown scope or target is a
+      // mistake and doing anything other than failing is just making things
+      // harder to debug.
+      //
       switch (qual.front ().pair)
       {
       case '/':
         {
           assert (qual.front ().directory ());
-          sg = enter_scope (*this, move (qual.front ().dir));
-          s = scope_;
+
+          dir_path& d (qual.front ().dir);
+          enter_scope::complete_normalize (*scope_, d);
+
+          s = &ctx->scopes.find_out (d);
+
+          if (s->out_path () != d)
+            fail (loc) << "unknown scope " << d << " in scope-qualified "
+                       << "variable " << name << " expansion" <<
+              info << "did you forget to include the corresponding buildfile?";
+
           break;
         }
       default:
@@ -8356,8 +8377,24 @@ namespace build2
           if (n.pair)
             o = move (qual.back ());
 
-          tg = enter_target (*this, move (n), move (o), true, loc, trace);
-          t = target_;
+          t = enter_target::find_target (*this, n, o, loc, trace);
+
+          if (t == nullptr || !operator>= (t->decl, target_decl::implied)) // VC14
+          {
+            diag_record dr (fail (loc));
+
+            dr << "unknown target " << n;
+
+            if (n.pair && !o.dir.empty ())
+              dr << '@' << o.dir;
+
+            dr << " in target-qualified variable " << name << " expansion";
+          }
+
+          // Use the target's var_pool for good measure.
+          //
+          s = &t->base_scope ();
+
           break;
         }
       }
@@ -8365,10 +8402,13 @@ namespace build2
 
     // Lookup.
     //
-    if (const variable* pvar = scope_->var_pool ().find (name))
+    if (const variable* pvar =
+          (s != nullptr ? s : scope_)->var_pool ().find (name))
     {
       auto& var (*pvar);
 
+      // Note: the order of the following blocks is important.
+
       if (p != nullptr)
       {
         // The lookup depth is a bit of a hack but should be harmless since
diff --git a/old-tests/variable/override/buildfile b/old-tests/variable/override/buildfile
index 87dc273..c0330cb 100644
--- a/old-tests/variable/override/buildfile
+++ b/old-tests/variable/override/buildfile
@@ -3,6 +3,8 @@ if ($p.t != [null])
   [$p.t] p.v = [null]
 }
 
+/:
+
 print "/     :" $(/: p.v)
 
 if ($p.a == as)
@@ -22,6 +24,8 @@ print ".     :" $p.v
 
 d/
 {
+  file{t}:
+
   if ($p.d_a == as)
   {
     p.v = x
diff --git a/old-tests/variable/override/p/buildfile b/old-tests/variable/override/p/buildfile
index 166d869..8f4df28 100644
--- a/old-tests/variable/override/p/buildfile
+++ b/old-tests/variable/override/p/buildfile
@@ -15,6 +15,8 @@ print "p     :" $p.v
 
 d/
 {
+  file{t}:
+
   if ($p.p_d_a == as)
   {
     p.v = x
diff --git a/old-tests/variable/type-pattern-append/buildfile b/old-tests/variable/type-pattern-append/buildfile
index 348f70f..3077c32 100644
--- a/old-tests/variable/type-pattern-append/buildfile
+++ b/old-tests/variable/type-pattern-append/buildfile
@@ -1,3 +1,5 @@
+./ sub/:
+
 # Typed append/prepend.
 #
 #dir{a*}: x += [bool] true
diff --git a/tests/variable/override/testscript b/tests/variable/override/testscript
index 9ee4643..7b973c0 100644
--- a/tests/variable/override/testscript
+++ b/tests/variable/override/testscript
@@ -63,6 +63,8 @@
   p.x = 0
   file{*}: p.x += a
 
+  file{foo}:
+
   print $(file{foo}:p.x)
 
   p.x = 1                 # Should invalidate both caches.
diff --git a/tests/variable/target-type-pattern-specific/testscript b/tests/variable/target-type-pattern-specific/testscript
index 016380b..9c600ca 100644
--- a/tests/variable/target-type-pattern-specific/testscript
+++ b/tests/variable/target-type-pattern-specific/testscript
@@ -12,6 +12,9 @@ x = x
 y = y
 dir{*}: x  = X
 dir{*}: y += Y
+
+./:
+
 print $(./: x)
 print $(./: y)
 EOI
@@ -26,6 +29,7 @@ dir{*}: x = y
 
 x = z
 dir{*-foo}: x = $x # 'z'
+bar-foo/:
 print $(bar-foo/: x)
 
 x = G
@@ -59,6 +63,7 @@ print $(file{x-foz}: x)
 *: x1 = X1
 {*}: x2 = X2
 target{*}: x3 = X3
+file{x}:
 print $(file{x}: x1)
 print $(file{x}: x2)
 print $(file{x}: x3)
@@ -89,6 +94,9 @@ dir{*}:
   y += Y
   z  = $x # Note: from scope.
 }
+
+./:
+
 print $(./: x)
 print $(./: y)
 print $(./: z)
@@ -108,6 +116,9 @@ file{f*} file{b*}:
   x  = X
   y += Y
 }
+
+file{foo bar}:
+
 print $(file{foo}: x)
 print $(file{bar}: y)
 EOI
@@ -123,6 +134,8 @@ EOO
   $* <<EOI >>EOO
   file{~/'.+\.txt'/i}: x = 1
 
+  file{foo.txt foo.TXT}:
+
   print $(file{foo.txt}: x)
   print $(file{foo.TXT}: x)
   EOI
@@ -140,6 +153,8 @@ EOO
   txt{~/'.+\.tx'/e}:  x = 2
   txt{~/'.+\.txt'/e}: x = 3
 
+  txt{foo.x foo.tx foo.txt foo.bar...}:
+
   print $(txt{foo.x}: x)
   print $(txt{foo.tx}: x)
   print $(txt{foo.txt}: x)
@@ -157,6 +172,8 @@ EOO
   x = 0
   file{~/'(.+)-\1'/}: x = 1
 
+  file{foo-foo foo-bar}:
+
   print $(file{foo-foo}: x)
   print $(file{foo-bar}: x)
   EOI
@@ -169,6 +186,8 @@ EOO
   $* <<EOI >>EOO
   foo/dir{~/b.+/}: x = 1
 
+  foo/dir{bar}:
+
   print $(foo/dir{bar}: x)
   EOI
   1