From 649d388ff422a9a049e2b50768db357a73ee59d5 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Fri, 2 Dec 2022 15:34:53 +0200 Subject: 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. --- libbuild2/file.cxx | 6 +- libbuild2/parser.cxx | 98 +++++++++++++++------- old-tests/variable/override/buildfile | 4 + old-tests/variable/override/p/buildfile | 2 + old-tests/variable/type-pattern-append/buildfile | 2 + tests/variable/override/testscript | 2 + .../target-type-pattern-specific/testscript | 19 +++++ 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 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 $* <>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 $* <>EOO foo/dir{~/b.+/}: x = 1 + foo/dir{bar}: + print $(foo/dir{bar}: x) EOI 1 -- cgit v1.1