Clean up module-related diagnostics

7 files changed, 64 insertions, 11 deletions

diff --git a/build2/cc/compile.cxx b/build2/cc/compile.cxx
index a7ad2fa..265b1a9 100644
--- a/build2/cc/compile.cxx
+++ b/build2/cc/compile.cxx
@@ -2451,7 +2451,7 @@ namespace build2
       if (!modules)
       {
         if (!mi.name.empty () || !mi.imports.empty ())
-          fail << "modules support not enabled or unavailable";
+          fail (relative (src)) << "modules support not enabled/available";
 
         return;
       }
@@ -2482,7 +2482,7 @@ namespace build2
 
       if (!mi.imports.empty ())
         md.mod_pos = search_modules (
-          act, t, lo, tt.bmi, move (mi.imports), cs);
+          act, t, lo, tt.bmi, src, move (mi.imports), cs);
 
       if (dd.expect (cs.string ()) != nullptr)
         updating = true;
@@ -2508,6 +2508,7 @@ namespace build2
                     file& t,
                     lorder lo,
                     const target_type& mtt,
+                    const file& src,
                     module_imports&& imports,
                     sha256& cs) const
     {
@@ -2769,7 +2770,7 @@ namespace build2
         // for bmi{} to have a different name).
         //
         if (p.is_a<bmi> ())
-          pt = &search (t, mtt, p.key ()); //@@ MOD: fuzzy...
+          pt = &search (t, mtt, p.key ()); // Same logic as in picking obj*{}.
         else if (p.is_a (mtt))
         {
           if (pt == nullptr)
@@ -2821,9 +2822,25 @@ namespace build2
         {
           if (pts[start + i] == nullptr)
           {
-            // @@ MOD: keep import location for diagnostics?
+            // It would have been nice to print the location of the import
+            // declaration. And we could save it during parsing at the expense
+            // of a few paths (that can be pooled). The question is what to do
+            // when we re-create this information from depdb? We could have
+            // saved the location information there but the relative paths
+            // (e.g., from the #line directives) could end up being wrong if
+            // the we re-run from a different working directory.
             //
-            fail << "unresolved import for module " << imports[i].name;
+            // It seems the only workable approach is to extract full location
+            // info during parse, not save it in depdb, when re-creating,
+            // fallback to just src path without any line/column information.
+            // This will probably cover the majority of case (most of the time
+            // it will be a misspelled module name, not a removal of module
+            // from buildfile).
+            //
+            // But at this stage this doesn't seem worth the trouble.
+            //
+            fail (relative (src)) << "unable to resolve module "
+                                  << imports[i].name;
           }
         }
       }
@@ -2858,7 +2875,8 @@ namespace build2
             {
               if (p.is_a (*x_mod)) // Got to be there.
               {
-                fail << "failed to correctly guess module name from " << p <<
+                fail (relative (src)) << "failed to correctly guess module "
+                                      << "name from " << p <<
                   info << "guessed: " << in <<
                   info << "actual:  " << mn <<
                   info << "consider adjusting module interface file names or" <<
diff --git a/build2/cc/compile.hxx b/build2/cc/compile.hxx
index d1b14bc..be4a2e2 100644
--- a/build2/cc/compile.hxx
+++ b/build2/cc/compile.hxx
@@ -113,7 +113,7 @@ namespace build2
 
       modules_positions
       search_modules (action, file&, lorder, const target_type&,
-                      module_imports&&, sha256&) const;
+                      const file&, module_imports&&, sha256&) const;
 
       void
       append_modules (cstrings&, strings&, const file&) const;
diff --git a/build2/diagnostics.hxx b/build2/diagnostics.hxx
index ce5f996..1e76099 100644
--- a/build2/diagnostics.hxx
+++ b/build2/diagnostics.hxx
@@ -224,7 +224,18 @@ namespace build2
                             const char* name,
                             const location& l,
                             uint16_t sverb)
-        : type_ (type), mod_ (mod), name_ (name), loc_ (l), sverb_ (sverb) {}
+        : type_ (type), mod_ (mod), name_ (name),
+          loc_ (l),
+          sverb_ (sverb) {}
+
+    location_prologue_base (const char* type,
+                            const char* mod,
+                            const char* name,
+                            path&& f,
+                            uint16_t sverb)
+        : type_ (type), mod_ (mod), name_ (name),
+          file_ (move (f)), loc_ (&file_),
+          sverb_ (sverb) {}
 
     void
     operator() (const diag_record& r) const;
@@ -233,6 +244,7 @@ namespace build2
     const char* type_;
     const char* mod_;
     const char* name_;
+    const path file_;
     const location loc_;
     const uint16_t sverb_;
   };
@@ -265,6 +277,15 @@ namespace build2
       return location_prologue (epilogue_, type_, mod_, name_, l, sverb_ ());
     }
 
+    // fail (relative (src)) << ...
+    //
+    location_prologue
+    operator() (path&& f) const
+    {
+      return location_prologue (
+        epilogue_, type_, mod_, name_, move (f), sverb_ ());
+    }
+
     template <typename L>
     location_prologue
     operator() (const L& l) const
diff --git a/build2/utility.cxx b/build2/utility.cxx
index 2b3f785..f2d9155 100644
--- a/build2/utility.cxx
+++ b/build2/utility.cxx
@@ -10,6 +10,7 @@
 #include <cstdlib>  // strtol()
 #include <iostream> // cerr
 
+#include <build2/target.hxx>
 #include <build2/variable.hxx>
 #include <build2/diagnostics.hxx>
 
@@ -125,6 +126,14 @@ namespace build2
   dir_path home;
   const dir_path* relative_base = &work;
 
+  path
+  relative (const path_target& t)
+  {
+    const path& p (t.path ());
+    assert (!p.empty ());
+    return relative (p);
+  }
+
   string
   diag_relative (const path& p, bool cur)
   {
diff --git a/build2/utility.hxx b/build2/utility.hxx
index 4ce3407..bb0eb8e 100644
--- a/build2/utility.hxx
+++ b/build2/utility.hxx
@@ -135,6 +135,11 @@ namespace build2
   basic_path<char, K>
   relative (const basic_path<char, K>&);
 
+  class path_target;
+
+  path
+  relative (const path_target&);
+
   // In addition to calling relative(), this function also uses shorter
   // notations such as '~/'. For directories the result includes the trailing
   // slash. If the path is the same as base, returns "./" if current is true
diff --git a/tests/cc/modules/testscript b/tests/cc/modules/testscript
index c259014..50dc9b2 100644
--- a/tests/cc/modules/testscript
+++ b/tests/cc/modules/testscript
@@ -160,7 +160,7 @@ $* test clean <<EOI
 :
 ln -s ../driver.cxx ./;
 $* test &*.d <'exe{test}: cxx{driver}' 2>>EOE != 0
-  error: unresolved import for module foo.core
+  driver.cxx: error: unable to resolve module foo.core
   EOE
 
 : misguessed
@@ -168,7 +168,7 @@ $* test &*.d <'exe{test}: cxx{driver}' 2>>EOE != 0
 ln -s ../core.mxx ./;
 cat <'import bar.core;' >=driver.cxx;
 $* test &*.d <'exe{test}: cxx{driver} mxx{core}' 2>>EOE != 0
-  error: failed to correctly guess module name from mxx{core}
+  driver.cxx: error: failed to correctly guess module name from mxx{core}
     info: guessed: bar.core
     info: actual:  foo.core
     info: consider adjusting module interface file names or
diff --git a/tests/cc/preprocessed/testscript b/tests/cc/preprocessed/testscript
index 8875f7a..b429853 100644
--- a/tests/cc/preprocessed/testscript
+++ b/tests/cc/preprocessed/testscript
@@ -98,7 +98,7 @@ $* &test* <<EOI 2>>EOE != 0
   cc.preprocessed = modules
   exe{test}: cxx{test}
   EOI
-  error: modules support not enabled or unavailable
+  test.cxx: error: modules support not enabled/available
   EOE
 
 : all