Return stable imported target name from import_direct()

5 files changed, 75 insertions, 30 deletions

diff --git a/libbuild2/file.cxx b/libbuild2/file.cxx
index 682f89d..4cf6d82 100644
--- a/libbuild2/file.cxx
+++ b/libbuild2/file.cxx
@@ -2679,11 +2679,10 @@ namespace build2
     //
     if (metadata)
     {
-      pair<const target*, import_kind> r (
+      import_result<target> r (
         import_direct (base, move (tgt), ph2, opt, metadata, loc));
 
-      return make_pair (r.first != nullptr ? r.first->as_name () : names {},
-                        r.second);
+      return make_pair (move (r.name), r.kind);
     }
 
     pair<name, optional<dir_path>> r (
@@ -2725,7 +2724,13 @@ namespace build2
                                           nullopt           /* metadata */,
                                           false             /* existing */,
                                           loc))
+            {
+              // Note that here r.first was still project-qualified and we
+              // have no choice but to call as_name(). This shouldn't cause
+              // any problems since the import() call assigns the extension.
+              //
               ns = t->as_name ();
+            }
             else
               ns.clear (); // NULL
           }
@@ -2765,6 +2770,10 @@ namespace build2
 
     // Target type-specific search.
     //
+    // Note that if this function returns a target, it should have the
+    // extension assigned (like the find/insert_target() functions) so that
+    // as_name() returns a stable name.
+    //
     const target_key& tk (pk.tk);
     const target_type& tt (*tk.type);
 
@@ -2862,7 +2871,7 @@ namespace build2
     dr << endf;
   }
 
-  pair<const target*, import_kind>
+  import_result<target>
   import_direct (bool& new_value,
                  scope& base,
                  name tgt,
@@ -2872,7 +2881,7 @@ namespace build2
                  const location& loc,
                  const char* what)
   {
-    // This is like normal import() except we return the target rather than
+    // This is like normal import() except we return the target in addition to
     // its name.
     //
     tracer trace ("import_direct");
@@ -2888,7 +2897,7 @@ namespace build2
     //
     auto meta (metadata ? optional<string> (tgt.value) : nullopt);
 
-    names ns;
+    names ns, rns;
     import_kind k;
     const target* pt (nullptr);
 
@@ -2911,7 +2920,7 @@ namespace build2
       if (r.first.empty ())
       {
         assert (opt);
-        return make_pair (pt, k); // NULL
+        return import_result<target> {nullptr, {}, k}; // NULL
       }
       else if (r.first.qualified ())
       {
@@ -2931,22 +2940,34 @@ namespace build2
         }
 
         if (pt == nullptr)
-          return make_pair (pt, k); // NULL
+          return import_result<target> {nullptr, {}, k}; // NULL
+
+        // Note that here r.first was still project-qualified and we have no
+        // choice but to call as_name() (below). This shouldn't cause any
+        // problems since the import() call assigns the extension.
 
-        // Otherwise fall through.
+        // Fall through.
       }
       else
+      {
+        // It's a bit fuzzy in which cases we end up here. So for now we keep
+        // the original if it's absolute and call as_name() otherwise.
+        //
+        if (r.first.absolute ())
+          rns.push_back (r.first);
+
         ns.push_back (move (r.first)); // And fall through.
+      }
     }
     else
     {
       k = r.first.absolute () ? import_kind::adhoc : import_kind::normal;
-      ns = import_load (base.ctx, move (r), metadata, loc).first;
+      rns = ns = import_load (base.ctx, move (r), metadata, loc).first;
     }
 
     if (pt == nullptr)
     {
-      // Similar logic to perform's search().
+      // Similar logic to perform's search(). Note: modifies ns.
       //
       target_key tk (base.find_target_key (ns, loc));
       pt = ctx.targets.find (tk, trace);
@@ -2954,6 +2975,9 @@ namespace build2
         fail (loc) << "unknown imported target " << tk;
     }
 
+    if (rns.empty ())
+      rns = pt->as_name ();
+
     target& t (pt->rw ()); // Load phase.
 
     // Note that if metadata is requested via any of the import*() functions,
@@ -3025,18 +3049,18 @@ namespace build2
         fail (loc) << "no metadata for imported target " << t;
     }
 
-    return make_pair (pt, k);
+    return import_result<target> {pt, move (rns), k};
   }
 
   ostream&
-  operator<< (ostream& o, const pair<const exe*, import_kind>& p)
+  operator<< (ostream& o, const import_result<exe>& r)
   {
-    assert (p.first != nullptr);
+    assert (r.target != nullptr);
 
-    if (p.second == import_kind::normal)
-      o << *p.first;
+    if (r.kind == import_kind::normal)
+      o << *r.target;
     else
-      o << p.first->process_path ();
+      o << r.target->process_path ();
 
     return o;
   }
diff --git a/libbuild2/file.hxx b/libbuild2/file.hxx
index 1c8e57e..b47d8dc 100644
--- a/libbuild2/file.hxx
+++ b/libbuild2/file.hxx
@@ -360,9 +360,19 @@ namespace build2
   // The what argument specifies what triggered the import (for example,
   // "module load") and is used in diagnostics.
   //
-  // This function also returns the kind of import that was performed.
+  // This function also returns the stable exported target name (see
+  // target::as_name() for details) as well as the kind of import that was
+  // performed.
   //
-  pair<const target*, import_kind>
+  template <typename T>
+  struct import_result
+  {
+    const T*    target;
+    names       name;
+    import_kind kind;
+  };
+
+  import_result<target>
   import_direct (scope& base,
                  name,
                  bool phase2,
@@ -376,7 +386,7 @@ namespace build2
   // details. Note that a phase 2 fallback/default logic is not considered new
   // (though this can be easily adjusted based on import kind).
   //
-  LIBBUILD2_SYMEXPORT pair<const target*, import_kind>
+  LIBBUILD2_SYMEXPORT import_result<target>
   import_direct (bool& new_value,
                  scope& base,
                  name,
@@ -388,13 +398,13 @@ namespace build2
 
 
   template <typename T>
-  pair<const T*, import_kind>
+  import_result<T>
   import_direct (scope&,
                  name, bool, bool, bool,
                  const location&, const char* = "import");
 
   template <typename T>
-  pair<const T*, import_kind>
+  import_result<T>
   import_direct (bool&,
                  scope&,
                  name,
@@ -406,7 +416,7 @@ namespace build2
   // build system modules to print the configuration report.
   //
   LIBBUILD2_SYMEXPORT ostream&
-  operator<< (ostream&, const pair<const exe*, import_kind>&);
+  operator<< (ostream&, const import_result<exe>&);
 
   // As import phase 2 but only imports as an already existing target. But
   // unlike it, this function can be called during the load and execute
diff --git a/libbuild2/file.ixx b/libbuild2/file.ixx
index bd138a0..dbd892d 100644
--- a/libbuild2/file.ixx
+++ b/libbuild2/file.ixx
@@ -42,7 +42,7 @@ namespace build2
     return *import (ctx, pk, false, nullopt, false, location ());
   }
 
-  inline pair<const target*, import_kind>
+  inline import_result<target>
   import_direct (scope& base,
                  name tgt,
                  bool ph2, bool opt, bool md,
@@ -53,19 +53,21 @@ namespace build2
   }
 
   template <typename T>
-  inline pair<const T*, import_kind>
+  inline import_result<T>
   import_direct (scope& base,
                  name tgt,
                  bool ph2, bool opt, bool md,
                  const location& loc, const char* w)
   {
     auto r (import_direct (base, move (tgt), ph2, opt, md, loc, w));
-    return make_pair (r.first != nullptr ? &r.first->as<const T> () : nullptr,
-                      r.second);
+    return import_result<T> {
+      r.target != nullptr ? &r.target->as<const T> () : nullptr,
+      move (r.name),
+      r.kind};
   }
 
   template <typename T>
-  inline pair<const T*, import_kind>
+  inline import_result<T>
   import_direct (bool& nv,
                  scope& base,
                  name tgt,
@@ -73,8 +75,10 @@ namespace build2
                  const location& loc, const char* w)
   {
     auto r (import_direct (nv, base, move (tgt), ph2, opt, md, loc, w));
-    return make_pair (r.first != nullptr ? &r.first->as<const T> () : nullptr,
-                      r.second);
+    return import_result<T> {
+      r.target != nullptr ? &r.target->as<const T> () : nullptr,
+      move (r.name),
+      r.kind};
   }
 
   inline const target*
diff --git a/libbuild2/target-key.hxx b/libbuild2/target-key.hxx
index f5351b8..cd1ba83 100644
--- a/libbuild2/target-key.hxx
+++ b/libbuild2/target-key.hxx
@@ -33,6 +33,8 @@ namespace build2
 
     // Append/return the target name or a pair of names if out-qualified.
     //
+    // See also target::as_name() for the returned name stability guarantees.
+    //
     void
     as_name (names&) const;
 
diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx
index 1c10621..c2df3e7 100644
--- a/libbuild2/target.hxx
+++ b/libbuild2/target.hxx
@@ -315,6 +315,11 @@ namespace build2
     target_key
     key_locked () const;
 
+    // Note that the returned name is guaranteed to be "stable" (e.g., for
+    // hashing) only if the target has the extension assigned. This happens,
+    // for example, when a path is derived for a path-based target (which
+    // normally happens when such a target is matched for update).
+    //
     names
     as_name () const;