Improve heuristic to disambiguate recommended dependency to add.

If it still is ambiguous after applying visibility rules, we
disambiguate by label.

Change-Id: I1a36609a24091cc4780e166532f211f26a6a6964
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26420
Commit-Queue: Matt Stark <msta@google.com>
Reviewed-by: Takuto Ikuta <tikuta@google.com>

src/gn/command_suggest.cc

diff --git a/src/gn/command_suggest.cc b/src/gn/command_suggest.cc
index 6a13507..c6ff2f8 100644
--- a/src/gn/command_suggest.cc
+++ b/src/gn/command_suggest.cc
@@ -918,6 +918,50 @@
     visible_candidates = nonpublic_candidates;
   }
 
+  if (visible_candidates.size() > 1) {
+    auto ClosenessHeuristic =
+        [&](const Target* candidate) -> std::pair<int, size_t> {
+      // Consider the following example:
+      // * We attempted to include a file from //absl:strings.
+      // * //absl:strings is private, but is part of a group //absl:absl.
+      // * //angle:absl re-exports //absl:absl. It should ideally only be
+      //   visible to //angle/..., but may not be.
+      //
+      // Then:
+      // * If we include it from //angle:foo or //angle/subdir:foo, we should
+      //   get //angle:absl.
+      // * If we include it from outside, we should get the canonical one
+      //   (//absl:absl).
+      std::string_view cand_dir = candidate->label().dir().value();
+      std::string_view includer_dir = includer->label().dir().value();
+      std::string_view included_dir = included->label().dir().value();
+      // SourceDir always ends with "/", so string starts_with is ok.
+      if (includer_dir.starts_with(cand_dir)) {
+        // Prefer the more specific option.
+        return {2, cand_dir.size()};
+      } else if (included_dir.starts_with(cand_dir)) {
+        // If no option is more specific, we should prefer the "canonical" one.
+        // We assume it to be canonical if it's defined in the tree of the
+        // original target. (Eg. //absl re-exporting //absl/subdir:foo)
+        //
+        // We intentionally make no decision on whether //absl:exporter or
+        // //absl/subdir:exporter would be "more canonical" - we can change this
+        // later if we'd like, but for now we'd treat this as ambiguous.
+        return {1, 0};
+      }
+      return {0, 0};
+    };
+
+    std::ranges::stable_sort(
+        visible_candidates, [&](const Target* lhs, const Target* rhs) {
+          return ClosenessHeuristic(lhs) > ClosenessHeuristic(rhs);
+        });
+    if (ClosenessHeuristic(visible_candidates[0]) !=
+        ClosenessHeuristic(visible_candidates[1])) {
+      visible_candidates.resize(1);
+    }
+  }
+
   if (visible_candidates.size() == 1) {
     OutputDepSuggestion(visible_candidates);
   } else if (visible_candidates.size() > 1) {

src/gn/command_suggest_unittest.cc

diff --git a/src/gn/command_suggest_unittest.cc b/src/gn/command_suggest_unittest.cc
index 3f2833a..6cee00c 100644
--- a/src/gn/command_suggest_unittest.cc
+++ b/src/gn/command_suggest_unittest.cc
@@ -385,11 +385,10 @@
   };
 
   auto create_target = [&](std::string_view name, Target::OutputType type,
-                           auto fn) {
+                           auto fn, SourceDir dir = SourceDir("//")) {
     auto target = std::make_unique<Target>(
         setup_scope.settings(),
-        Label(SourceDir("//"), name, default_toolchain.dir(),
-              default_toolchain.name()));
+        Label(dir, name, default_toolchain.dir(), default_toolchain.name()));
     target->set_output_type(type);
     target->SetToolchain(setup_scope.toolchain());
     target->set_user_friendly_location(dummy_loc);
@@ -399,7 +398,7 @@
       target->set_module_type(module_type);
       target->set_module_name(std::string(name));
       target->public_headers().push_back(
-          SourceFile("//" + std::string(name) + ".h"));
+          SourceFile(dir.value() + std::string(name) + ".h"));
     }
     fn(target.get());
     Err err;
@@ -410,18 +409,21 @@
 
   auto includer = create_target("includer", Target::GROUP, [](Target*) {});
 
-  auto run_suggest = [&](std::string_view want) {
+  auto run_suggest_for = [&](std::string_view from, std::string_view want) {
     std::string output;
     auto collect = [&](std::string_view s, TextDecoration, HtmlEscaping) {
       output.append(s);
     };
     commands::TargetResolutionCache cache;
     commands::OutputSuggestions(all_targets, setup_scope.build_settings(),
-                                default_toolchain, "//:includer", want, collect,
-                                cache);
+                                default_toolchain, from, want, collect, cache);
     return output;
   };
 
+  auto run_suggest = [&](std::string_view want) {
+    return run_suggest_for("//:includer", want);
+  };
+
   auto visible = create_target("visible", Target::SOURCE_SET,
                                [&](Target* t) { t->visibility().SetPublic(); });
   auto visible_group =
@@ -578,6 +580,34 @@
       "(defined at //BUILD.gn:1)\n"
       "  (`gn edit \"add public_deps :private_target\" //:includer`)\n",
       run_suggest("private_target_Private"));
+
+  auto pkg_target = create_target(
+      "pkg_target", Target::SOURCE_SET, [](Target* t) {}, SourceDir("//pkg/"));
+  auto pkg_includer = create_target(
+      "pkg_includer", Target::GROUP, [](Target*) {}, SourceDir("//pkg/sub/"));
+  auto other_exposer = create_target(
+      "other_exposer", Target::GROUP,
+      [&](Target* t) {
+        t->public_deps().push_back(LabelTargetPair(pkg_target.get()));
+        t->visibility().SetPublic();
+      },
+      SourceDir("//other/"));
+  auto parent_exposer = create_target(
+      "parent_exposer", Target::GROUP,
+      [&](Target* t) {
+        t->public_deps().push_back(LabelTargetPair(pkg_target.get()));
+        t->visibility().SetPublic();
+      },
+      SourceDir("//pkg/"));
+
+  // Disambiguate by package closeness: parent_exposer is in //pkg/, which is an
+  // ancestor of //pkg/sub/, so it is preferred over other_exposer in //other/.
+  EXPECT_EQ(
+      "Suggestion: Add public_deps = [ \"//pkg:parent_exposer\" ] to "
+      ":pkg_includer (defined at //BUILD.gn:1)\n"
+      "  (`gn edit \"add public_deps //pkg:parent_exposer\" "
+      "//pkg/sub:pkg_includer`)\n",
+      run_suggest_for("//pkg/sub:pkg_includer", pkg_target->module_name()));
 }
 
 TEST_F(SuggestTest, ApplyValidSuggestion) {