Detect and diagnose undefined dependency configuration variables

3 files changed, 202 insertions, 63 deletions

diff --git a/bpkg/package-skeleton.cxx b/bpkg/package-skeleton.cxx
index 6539077..72192ff 100644
--- a/bpkg/package-skeleton.cxx
+++ b/bpkg/package-skeleton.cxx
@@ -40,6 +40,90 @@ namespace bpkg
 
   void
   build2_init (const common_options&);
+}
+
+namespace bpkg
+{
+  // Check whether the specified configuration variable override has a
+  // project variables (i.e., its name starts with config.<project>).
+  //
+  // Note that some user-specified variables may have qualifications
+  // (global, scope, etc) but there is no reason to expect any project
+  // configuration variables to use such qualifications (since they can
+  // only apply to one project). So we ignore all qualified variables.
+  //
+  static inline bool
+  project_override (const string& v, const string& p)
+  {
+    size_t n (p.size ());
+    return v.compare (0, n, p) == 0 && strchr (".=+ \t", v[n]) != nullptr;
+  }
+
+  // Check whether the specified configuration variable name is a project
+  // variables (i.e., its name starts with config.<project>).
+  //
+  static inline bool
+  project_variable (const string& v, const string& p)
+  {
+    size_t n (p.size ());
+    return v.compare (0, n, p) == 0 && (v[n] == '.' || v[n] == '\0');
+  }
+
+  // Customized buildfile parser that is used to detect and diagnose
+  // references to undefined dependency configuration variables.
+  //
+  class buildfile_parser: public build2::parser
+  {
+  public:
+    buildfile_parser (build2::context& ctx,
+                      const strings& dvps,
+                      optional<size_t> dvps_pending = {})
+        : parser (ctx),
+          dependency_var_prefixes_ (dvps),
+          dependency_var_prefixes_pending_ (dvps_pending) {}
+
+  protected:
+    virtual build2::lookup
+    lookup_variable (build2::name&& qual,
+                     string&& name,
+                     const build2::location& loc) override
+    {
+      using namespace build2;
+      using build2::fail;
+      using build2::info;
+
+      // To avoid making copies of the name, pre-check if it is from one
+      // of the dependencies.
+      //
+      optional<string> dep;
+      if (!pre_parse_ && qual.empty ())
+      {
+        auto b (dependency_var_prefixes_.begin ());
+        auto e (dependency_var_prefixes_pending_
+                ? b + *dependency_var_prefixes_pending_
+                : dependency_var_prefixes_.end ());
+
+        if (find_if (b, e,
+                     [&name] (const string& p)
+                     {
+                       return project_variable (name, p);
+                     }) != e)
+          dep = name;
+      }
+
+      lookup l (parser::lookup_variable (move (qual), move (name), loc));
+
+      if (dep && !l.defined ())
+        fail (loc) << "undefined dependency configuration variable " << *dep <<
+          info << "was " << *dep << " set in earlier prefer or require clause?";
+
+      return l;
+    }
+
+  private:
+    const strings& dependency_var_prefixes_;
+    optional<size_t> dependency_var_prefixes_pending_;
+  };
 
   // Note: cannot be package_skeleton member function due to iterator return
   // (build2 stuff is only forward-declared in the header).
@@ -77,6 +161,8 @@ namespace bpkg
         dependency_reflect_ (move (v.dependency_reflect_)),
         dependency_reflect_index_ (v.dependency_reflect_index_),
         dependency_reflect_pending_ (v.dependency_reflect_pending_),
+        dependency_var_prefixes_ (move (v.dependency_var_prefixes_)),
+        dependency_var_prefixes_pending_ (v.dependency_var_prefixes_pending_),
         prefer_accept_ (v.prefer_accept_)
   {
     v.db_ = nullptr;
@@ -110,6 +196,8 @@ namespace bpkg
       dependency_reflect_ = move (v.dependency_reflect_);
       dependency_reflect_index_ = v.dependency_reflect_index_;
       dependency_reflect_pending_ = v.dependency_reflect_pending_;
+      dependency_var_prefixes_ = move (v.dependency_var_prefixes_);
+      dependency_var_prefixes_pending_ = v.dependency_var_prefixes_pending_;
       prefer_accept_ = v.prefer_accept_;
 
       v.db_ = nullptr;
@@ -141,6 +229,8 @@ namespace bpkg
         dependency_reflect_ (v.dependency_reflect_),
         dependency_reflect_index_ (v.dependency_reflect_index_),
         dependency_reflect_pending_ (v.dependency_reflect_pending_),
+        dependency_var_prefixes_ (v.dependency_var_prefixes_),
+        dependency_var_prefixes_pending_ (v.dependency_var_prefixes_pending_),
         prefer_accept_ (v.prefer_accept_)
   {
     // The idea here is to create an "unloaded" copy but with enough state
@@ -174,6 +264,9 @@ namespace bpkg
     dependency_reflect_index_ = 0;
     dependency_reflect_pending_ = 0;
 
+    dependency_var_prefixes_.clear ();
+    dependency_var_prefixes_pending_ = 0;
+
     prefer_accept_ = nullopt;
   }
 
@@ -395,13 +488,9 @@ namespace bpkg
       //    package. And, could be helpful to warn that configuration variable
       //    does not exist.
       //
-      const string& p (var_prefix_);
-      size_t n (p.size ());
-
       for (const variable& var: rs.ctx.var_pool)
       {
-        if (var.name.compare (0, n, p) != 0 || (var.name[n] != '.' &&
-                                                var.name[n] != '\0'))
+        if (!project_variable (var.name, var_prefix_))
           continue;
 
         using config::variable_origin;
@@ -571,7 +660,7 @@ namespace bpkg
             dr << build2::info (build2::location (mf,
                                                   nv.value_line,
                                                   nv.value_column))
-               << "in depends value defined here";
+               << "in depends manifest value defined here";
             break;
           }
         }
@@ -616,21 +705,23 @@ namespace bpkg
       uint64_t il (1);
 
       auto df = build2::make_diag_frame (
-        [&cond, &rs, depends_index] (const build2::diag_record& dr)
+        [this, &cond, &rs, depends_index] (const build2::diag_record& dr)
         {
           dr << info << "enable condition: (" << cond << ")";
 
           // For external packages we have the manifest so print the location
           // of the depends value in questions.
           //
-          if (!rs.out_eq_src ())
+          if (rs.out_eq_src ())
+            dr << info << "in depends manifest value of package " << key.name;
+          else
             depends_location (dr,
                               rs.src_path () / manifest_file,
                               depends_index);
         });
 
       lexer l (is, in, il /* start line */);
-      parser p (rs.ctx);
+      buildfile_parser p (rs.ctx, dependency_var_prefixes_);
       value v (p.parse_eval (l, rs, rs, parser::pattern_mode::expand));
 
       try
@@ -715,6 +806,7 @@ namespace bpkg
       // Drop the state from the previous evaluation of prefer/accept if it's
       // from the wrong position.
       //
+      optional<size_t> dependency_var_prefixes_pending;
       if (prefer_accept_)
       {
         if (*prefer_accept_ != indexes)
@@ -723,12 +815,16 @@ namespace bpkg
           prefer_accept_ = nullopt;
         }
         else
+        {
           // This could theoretically happen if we make a copy of the skeleton
           // after evaluate_prefer_accept() and then attempt to continue with
           // the call on the copy to evaluate_reflect() passing the same
           // position. But it doesn't appear our usage should trigger this.
           //
           assert (ctx_ != nullptr);
+
+          dependency_var_prefixes_pending = dependency_var_prefixes_pending_;
+        }
       }
 
       scope& rs (load ());
@@ -740,7 +836,7 @@ namespace bpkg
       uint64_t il (1);
 
       auto df = build2::make_diag_frame (
-        [&refl, &rs, depends_index] (const build2::diag_record& dr)
+        [this, &refl, &rs, depends_index] (const build2::diag_record& dr)
         {
           // Probably safe to assume a one-line fragment contains a variable
           // assignment.
@@ -754,7 +850,9 @@ namespace bpkg
           // For external packages we have the manifest so print the location
           // of the depends value in questions.
           //
-          if (!rs.out_eq_src ())
+          if (rs.out_eq_src ())
+            dr << info << "in depends manifest value of package " << key.name;
+          else
             depends_location (dr,
                               rs.src_path () / manifest_file,
                               depends_index);
@@ -822,7 +920,9 @@ namespace bpkg
       process (true);
 
       lexer l (is, in, il /* start line */);
-      parser p (rs.ctx);
+      buildfile_parser p (rs.ctx,
+                          dependency_var_prefixes_,
+                          dependency_var_prefixes_pending);
       p.parse_buildfile (l, &rs, rs);
 
       process (false);
@@ -1040,9 +1140,12 @@ namespace bpkg
       //
       // Additionally, for all origins we need to typify the variables.
       //
-      // All of this is done by load().
+      // All of this is done by load(), including removing and returning the
+      // dependency variable prefixes (config.<project>) which we later add
+      // to dependency_var_prefixes_.
       //
-      scope& rs (load (cfgs));
+      strings dvps;
+      scope& rs (load (cfgs, &dvps, true /* defaults */));
 
       // Evaluate the prefer clause.
       //
@@ -1054,7 +1157,7 @@ namespace bpkg
         uint64_t il (1);
 
         auto df = build2::make_diag_frame (
-          [&prefer, &rs, depends_index] (const build2::diag_record& dr)
+          [this, &prefer, &rs, depends_index] (const build2::diag_record& dr)
           {
             dr << info << "prefer clause:\n"
                << trim_right (string (prefer));
@@ -1062,22 +1165,25 @@ namespace bpkg
             // For external packages we have the manifest so print the
             // location of the depends value in questions.
             //
-            if (!rs.out_eq_src ())
+            if (rs.out_eq_src ())
+              dr << info << "in depends manifest value of package " << key.name;
+            else
               depends_location (dr,
                                 rs.src_path () / manifest_file,
                                 depends_index);
           });
 
         lexer l (is, in, il /* start line */);
-        parser p (rs.ctx);
+        buildfile_parser p (rs.ctx, dependency_var_prefixes_);
         p.parse_buildfile (l, &rs, rs);
 
         // Check if the dependent set any stray configuration variables.
         //
-        for (package_configuration& cfg: cfgs)
+        for (size_t i (0); i != cfgs.size (); ++i)
         {
-          string ns ("config." + cfg.package.name.variable ());
+          package_configuration& cfg (cfgs[i]);
 
+          const string& ns (dvps[i]); // Parallel.
           for (auto p (rs.vars.lookup_namespace (ns));
                p.first != p.second;
                ++p.first)
@@ -1112,21 +1218,23 @@ namespace bpkg
         uint64_t il (1);
 
         auto df = build2::make_diag_frame (
-          [&accept, &rs, depends_index] (const build2::diag_record& dr)
+          [this, &accept, &rs, depends_index] (const build2::diag_record& dr)
           {
             dr << info << "accept condition: (" << accept << ")";
 
-            // For external packages we have the manifest so print the location
-            // of the depends value in questions.
+            // For external packages we have the manifest so print the
+            // location of the depends value in questions.
             //
-            if (!rs.out_eq_src ())
+            if (rs.out_eq_src ())
+              dr << info << "in depends manifest value of package " << key.name;
+            else
               depends_location (dr,
                                 rs.src_path () / manifest_file,
                                 depends_index);
           });
 
         lexer l (is, in, il /* start line */);
-        parser p (rs.ctx);
+        buildfile_parser p (rs.ctx, dependency_var_prefixes_);
         value v (p.parse_eval (l, rs, rs, parser::pattern_mode::expand));
 
         try
@@ -1151,10 +1259,11 @@ namespace bpkg
         dependency_reflect_index_ = depends_index;
         dependency_reflect_pending_ = dependency_reflect_.size ();
 
-        for (package_configuration& cfg: cfgs)
+        for (size_t i (0); i != cfgs.size (); ++i)
         {
-          string ns ("config." + cfg.package.name.variable ());
+          package_configuration& cfg (cfgs[i]);
 
+          const string& ns (dvps[i]);
           for (auto p (rs.vars.lookup_namespace (ns));
                p.first != p.second;
                ++p.first)
@@ -1262,6 +1371,15 @@ namespace bpkg
           }
         }
 
+        // Note that because we add it here, the following reflect clause will
+        // not be able to expand undefined values. We handle this by keeping a
+        // pending position.
+        //
+        dependency_var_prefixes_pending_ = dependency_var_prefixes_.size ();
+        dependency_var_prefixes_.insert (dependency_var_prefixes_.end (),
+                                         make_move_iterator (dvps.begin ()),
+                                         make_move_iterator (dvps.end ()));
+
         // Note: do not drop the build system state yet since it should be
         // reused by the following reflect clause, if any.
         //
@@ -1315,7 +1433,8 @@ namespace bpkg
       // information as well as overrides (see up_negotiate_configuration()
       // for details).
       //
-      scope& rs (load (cfgs, false /* defaults */));
+      strings dvps;
+      scope& rs (load (cfgs, &dvps, false /* defaults */));
 
       // Evaluate the require clause.
       //
@@ -1327,7 +1446,7 @@ namespace bpkg
         uint64_t il (1);
 
         auto df = build2::make_diag_frame (
-          [&require, &rs, depends_index] (const build2::diag_record& dr)
+          [this, &require, &rs, depends_index] (const build2::diag_record& dr)
           {
             dr << info << "require clause:\n"
                << trim_right (string (require));
@@ -1335,30 +1454,34 @@ namespace bpkg
             // For external packages we have the manifest so print the
             // location of the depends value in questions.
             //
-            if (!rs.out_eq_src ())
+            if (rs.out_eq_src ())
+              dr << info << "in depends manifest value of package " << key.name;
+            else
               depends_location (dr,
                                 rs.src_path () / manifest_file,
                                 depends_index);
           });
 
         lexer l (is, in, il /* start line */);
-        parser p (rs.ctx);
+        buildfile_parser p (rs.ctx, dependency_var_prefixes_);
         p.parse_buildfile (l, &rs, rs);
 
         // Check for stray variables and enforce all the require restrictions
         // (bool, set to true, etc).
         //
-        for (package_configuration& cfg: cfgs)
+        for (size_t i (0); i != cfgs.size (); ++i)
         {
-          string ns ("config." + cfg.package.name.variable ());
+          package_configuration& cfg (cfgs[i]);
 
-          // Note that because we didn't set any default (or other dependent)
-          // values, all the values we see are set by this dependent.
-          //
+          const string& ns (dvps[i]); // Parallel.
           for (auto p (rs.vars.lookup_namespace (ns));
                p.first != p.second;
                ++p.first)
           {
+            // Note that because we didn't set any default (or other
+            // dependent) values, all the values we see are set by this
+            // dependent.
+            //
             const variable& var (p.first->first);
 
             // This can be one of the overrides (__override, __prefix, etc),
@@ -1401,10 +1524,11 @@ namespace bpkg
       // First determine if acceptable.
       //
       bool r (true);
-      for (package_configuration& cfg: cfgs)
+      for (size_t i (0); i != cfgs.size (); ++i)
       {
-        string ns ("config." + cfg.package.name.variable ());
+        package_configuration& cfg (cfgs[i]);
 
+        const string& ns (dvps[i]);
         for (auto p (rs.vars.lookup_namespace (ns));
              p.first != p.second;
              ++p.first)
@@ -1465,10 +1589,11 @@ namespace bpkg
         dependency_reflect_index_ = depends_index;
         dependency_reflect_pending_ = dependency_reflect_.size ();
 
-        for (package_configuration& cfg: cfgs)
+        for (size_t i (0); i != cfgs.size (); ++i)
         {
-          string ns ("config." + cfg.package.name.variable ());
+          package_configuration& cfg (cfgs[i]);
 
+          const string& ns (dvps[i]);
           for (auto p (rs.vars.lookup_namespace (ns));
                p.first != p.second;
                ++p.first)
@@ -1517,6 +1642,10 @@ namespace bpkg
             }
           }
         }
+
+        dependency_var_prefixes_.insert (dependency_var_prefixes_.end (),
+                                         make_move_iterator (dvps.begin ()),
+                                         make_move_iterator (dvps.end ()));
       }
 
       // Drop the build system state since it needs reloading (while it may
@@ -1545,7 +1674,7 @@ namespace bpkg
             find_if (config_vars_.begin (), config_vars_.end (),
                      [this] (const string& v)
                      {
-                       return project_override (v);
+                       return project_override (v, var_prefix_);
                      }) == config_vars_.end ());
   }
 
@@ -1572,7 +1701,7 @@ namespace bpkg
     //
     for (const string& v: config_vars_)
     {
-      if (project_override (v))
+      if (project_override (v, var_prefix_))
         print (v) << " (user configuration)";
     }
 
@@ -1639,7 +1768,7 @@ namespace bpkg
         //
         for (const string& v: config_vars_)
         {
-          if (project_override (v))
+          if (project_override (v, var_prefix_))
           {
             string n (var_name (v));
 
@@ -1870,7 +1999,7 @@ namespace bpkg
   }
 
   build2::scope& package_skeleton::
-  load (const dependency_configurations& cfgs, bool defaults)
+  load (const dependency_configurations& cfgs, strings* dvps, bool defaults)
   {
     if (ctx_ != nullptr)
     {
@@ -1893,6 +2022,7 @@ namespace bpkg
       // If we have any dependency configurations, then here we need to add
       // dependency configuration variables with the override origin to the
       // command line overrides (see evaluate_prefer_accept() for details).
+      // While at it, handle dependency variable prefixes.
       //
       strings dependency_vars;
       for (const package_configuration& cfg: cfgs)
@@ -1902,6 +2032,16 @@ namespace bpkg
           if (v.origin == variable_origin::override_)
             dependency_vars.push_back (v.serialize_cmdline ());
         }
+
+        string p ("config." + cfg.package.name.variable ());
+
+        auto i (find (dependency_var_prefixes_.begin (),
+                      dependency_var_prefixes_.end (),
+                      p));
+        if (i != dependency_var_prefixes_.end ())
+          dependency_var_prefixes_.erase (i);
+
+        dvps->push_back (move (p));
       }
 
       // If there aren't any, then we can reuse already merged cmd_vars (they
diff --git a/bpkg/package-skeleton.hxx b/bpkg/package-skeleton.hxx
index aa9b2a9..ad0fc42 100644
--- a/bpkg/package-skeleton.hxx
+++ b/bpkg/package-skeleton.hxx
@@ -201,10 +201,15 @@ namespace bpkg
     // If dependency configurations are specified, then typify the variables
     // and set their values. If defaults is false, then only typify the
     // variables and set overrides without setting the default/buildfile
-    // values. Note that buildfile values have value::extra set to 2.
+    // values. Note that buildfile values have value::extra set to 2. While
+    // at it, also remove from dependency_var_prefixes_ and add to
+    // dependency_var_prefixes variable prefixes (config.<project>) for
+    // the passed dependencies.
     //
     build2::scope&
-    load (const dependency_configurations& = {}, bool defaults = true);
+    load (const dependency_configurations& = {},
+          strings* dependency_var_prefixes = nullptr,
+          bool defaults = true);
 
     // Merge command line variable overrides into one list (normally to be
     // passed to bootstrap()).
@@ -217,23 +222,6 @@ namespace bpkg
                     const strings& dependency_vars = {},
                     bool cache = false);
 
-    // Check whether the specified configuration variable override has a
-    // project variables (i.e., its name start with config.<project>).
-    //
-    // Note that some user-specified variables may have qualifications
-    // (global, scope, etc) but there is no reason to expect any project
-    // configuration variables to use such qualifications (since they can
-    // only apply to one project). So we ignore all qualified variables.
-    //
-    bool
-    project_override (const string& v) const
-    {
-      const string& p (var_prefix_);
-      size_t n (p.size ());
-
-      return v.compare (0, n, p) == 0 && strchr (".=+ \t", v[n]) != nullptr;
-    }
-
     // Implementation details (public for bootstrap()).
     //
   public:
@@ -299,6 +287,17 @@ namespace bpkg
     size_t                  dependency_reflect_index_ = 0;
     size_t                  dependency_reflect_pending_ = 0;
 
+    // List of variable prefixes (config.<project>) of all known dependencies.
+    //
+    // This information is used to detect and diagnose references to undefined
+    // dependency configuration variables (for example, those that were not
+    // set and therefore not reflected). The pending index is used to ignore
+    // the entries added by the last evaluate_prefer_accept() in the following
+    // reflect clause (see prefer_accept_ below for details).
+    //
+    strings dependency_var_prefixes_;
+    size_t  dependency_var_prefixes_pending_;
+
     // Position of the last successfully evaluated prefer/accept clauses.
     //
     // This information is used to make all (as opposed to only those set by
diff --git a/tests/pkg-build.testscript b/tests/pkg-build.testscript
index 8d3945f..d5f767b 100644
--- a/tests/pkg-build.testscript
+++ b/tests/pkg-build.testscript
@@ -4320,7 +4320,7 @@ test.options += --no-progress
           $* fax/ 2>>~%EOE% != 0;
             <depends-enable-clause>:1: error: invalid bool value: multiple names
               info: enable condition: (config.fax.libbiz = true)
-            %  fax.manifest:10:10: info: in depends value defined here%
+            %  fax.manifest:10:10: info: in depends manifest value defined here%
             info: while satisfying fax/1.0.0#3
             EOE