Add `gn edit` suggestions to `gn suggest` Change-Id: I9e0b7f76fa706460e7b5132df29abf906a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/25401 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: Matt Stark <msta@google.com>
diff --git a/docs/reference.md b/docs/reference.md index 7608924..c155188 100644 --- a/docs/reference.md +++ b/docs/reference.md
@@ -1517,7 +1517,8 @@ Will print a suggestion like: Request: path/to/target.cc wants to depend on foo/bar.h - Suggestion: add deps = [ "//foo:bar" ] to "//path/to:target" (defined in //path/to/BUILD.gn:1234) + Suggestion: Add deps = [ "//foo:bar" ] to //path/to:target (defined in //path/to/BUILD.gn:1234) + (`gn edit "add deps //foo:bar" //path/to:target`) ``` ## <a name="targets"></a>Target declarations
diff --git a/src/gn/command_suggest.cc b/src/gn/command_suggest.cc index b7cfc4d..f7595d8 100644 --- a/src/gn/command_suggest.cc +++ b/src/gn/command_suggest.cc
@@ -43,12 +43,39 @@ Will print a suggestion like: Request: path/to/target.cc wants to depend on foo/bar.h - Suggestion: add deps = [ "//foo:bar" ] to "//path/to:target" (defined in //path/to/BUILD.gn:1234) + Suggestion: Add deps = [ "//foo:bar" ] to //path/to:target (defined in //path/to/BUILD.gn:1234) + (`gn edit "add deps //foo:bar" //path/to:target`) )"; constexpr std::string_view kPrivateSuffix = "_Private"; namespace { + +struct EditCommand { + std::vector<std::string> command; + std::string target; + + std::string SubcommandString() const { + std::string result; + for (size_t i = 0; i < command.size(); ++i) { + if (i > 0) + result += " "; + if (command[i].contains(' ')) { + result += "\"" + command[i] + "\""; + } else { + result += command[i]; + } + } + return result; + } + + std::vector<std::string> Args() const { return {SubcommandString(), target}; } + + std::string ToString() const { + return "gn edit \"" + SubcommandString() + "\" " + target; + } +}; + // Determines whether a source file is in either the public or private API of a // target. std::optional<commands::ApiScope> DepKind(const Target* target, @@ -351,54 +378,51 @@ OutputString("\"", kLabelLike); }; + Label current_toolchain = default_toolchain; + auto OutputDefinition = [&](const Target* target) { OutputString(":", kLabelLike); OutputString(target->label().name(), kLabelLike); OutputString(" (defined at "); OutputString(target->user_friendly_location().Describe(false), kLabelLike); OutputString(")"); - }; - - Label current_toolchain = default_toolchain; - auto OutputTarget = [¤t_toolchain, - &OutputString](const Target* target) { - OutputString(target->label().GetUserVisibleName(current_toolchain), - kLabelLike); - }; - - auto OutputInsertionHint = [&](std::string_view key, - const std::vector<std::string>& candidates, - const Target* target) { - bool plural = candidates.size() != 1; - StartSuggestion(); - if (plural) { - OutputString("Add one of the following to "); - OutputString(key); - OutputString(" in "); - } else { - OutputString("Add "); - OutputString(key); - OutputString(" = [ "); - OutputQuoted(candidates.front()); - OutputString(" ] to "); - } - OutputDefinition(target); if (current_toolchain != default_toolchain) { OutputString(" for toolchain "); OutputString( target->label().GetToolchainLabel().GetUserVisibleName(false), kLabelLike); } - if (plural) { - OutputString(":\n"); - for (const auto& candidate : candidates) { - OutputString("* "); - OutputString(candidate); - OutputString("\n"); - } + }; + + auto OutputTarget = [¤t_toolchain, + &OutputString](const Target* target) { + OutputString(target->label().GetUserVisibleName(current_toolchain), + kLabelLike); + }; + + auto OutputEditCommand = [&](const EditCommand& edit, const Target* target) { + StartSuggestion(); + if (edit.command.size() >= 4 && edit.command[0] == "move") { + OutputString("Move "); + OutputQuoted(edit.command[3]); + OutputString(" from `"); + OutputString(edit.command[1]); + OutputString("` to `"); + OutputString(edit.command[2]); + OutputString("` in "); + OutputDefinition(target); + } else if (edit.command.size() >= 3 && edit.command[0] == "add") { + OutputString("Add "); + OutputString(edit.command[1]); + OutputString(" = [ "); + OutputQuoted(edit.command[2]); + OutputString(" ] to "); + OutputDefinition(target); } else { - OutputString("\n"); + CHECK(false) << "Not implemented: " << edit.command[0]; } + OutputString("\n"); + OutputString(" (`" + edit.ToString() + "`)\n"); }; auto ResolveSuggestion = [&](std::string_view value, @@ -486,8 +510,18 @@ OutputString(", but not in the toolchain "); OutputString(current_toolchain.GetUserVisibleName(false), kLabelLike); OutputString("\n"); - OutputInsertionHint("public", {std::string(included_name)}, - targets.front().first); + SourceFile file = + ResolveFilePath(build_settings, all_targets, included_name, includer); + const Target* target = targets.front().first; + std::string path = file.is_null() + ? std::string(included_name) + : RebasePath(file.value(), target->label().dir(), + build_settings->root_path_utf8()); + EditCommand edit{ + .command = {"add", "public", std::move(path)}, + .target = target->label().GetUserVisibleName(current_toolchain), + }; + OutputEditCommand(edit, target); return true; } @@ -503,8 +537,12 @@ StartSuggestion(); OutputString( "Create a source_set target for the common headers and sources and " - "have all of the above targets depend on that."); - OutputInsertionHint(dep_field, {"$NEW_SOURCE_SET"}, includer); + "have all of the above targets depend on that.\n"); + EditCommand edit{ + .command = {"add", dep_field, "$NEW_SOURCE_SET"}, + .target = includer->label().GetUserVisibleName(current_toolchain), + }; + OutputEditCommand(edit, includer); return true; } @@ -514,11 +552,30 @@ OutputQuoted(included_name); OutputString(" is in the private API of "); OutputTarget(included); - StartSuggestion(); - OutputString("Move "); - OutputQuoted(included_name); - OutputString(" from `sources` to `public` in "); - OutputDefinition(included); + OutputString("\n"); + SourceFile file = + ResolveFilePath(build_settings, all_targets, included_name, includer); + if (file.is_null()) { + // We tried to do `gn suggest out //:includer=//:included_Private` + std::vector<SourceFile> candidates; + for (const auto& source : included->sources()) { + if (source.GetType() == SourceFile::SOURCE_H) { + candidates.push_back(source); + } + } + if (candidates.size() == 1) { + file = candidates.front(); + } + } + std::string path = file.is_null() + ? "$HEADER" + : RebasePath(file.value(), included->label().dir(), + build_settings->root_path_utf8()); + EditCommand edit{ + .command = {"move", "sources", "public", std::move(path)}, + .target = included->label().GetUserVisibleName(current_toolchain), + }; + OutputEditCommand(edit, included); } // TODO: There are a bunch of optimizations we can perform here to make better @@ -618,7 +675,29 @@ bool rhs_abs = !rhs.starts_with(':'); return std::tie(lhs_abs, lhs) < std::tie(rhs_abs, rhs); }); - OutputInsertionHint(dep_field, labels, includer); + if (labels.size() == 1) { + OutputEditCommand( + EditCommand{ + .command = {"add", dep_field, labels.front()}, + .target = includer->label().GetUserVisibleName(current_toolchain), + }, + includer); + return; + } + + StartSuggestion(); + OutputString("Add one of the following to "); + OutputString(dep_field); + OutputString(" in "); + OutputDefinition(includer); + OutputString(":\n"); + for (const auto& l : labels) { + EditCommand edit{ + .command = {"add", dep_field, l}, + .target = includer->label().GetUserVisibleName(current_toolchain), + }; + OutputString("* " + l + " (`" + edit.ToString() + "`)\n"); + } }; if (included->visibility().CanSeeMe(includer->label())) {
diff --git a/src/gn/command_suggest_unittest.cc b/src/gn/command_suggest_unittest.cc index 7367309..5c9ae78 100644 --- a/src/gn/command_suggest_unittest.cc +++ b/src/gn/command_suggest_unittest.cc
@@ -329,14 +329,14 @@ auto includer = create_target("includer", Target::GROUP, [](Target*) {}); - auto run_suggest = [&](const Target& want) { + auto run_suggest = [&](std::string_view want) { std::string output; auto collect = [&](std::string_view s, TextDecoration, HtmlEscaping) { output.append(s); }; commands::OutputSuggestions(all_targets, setup_scope.build_settings(), - default_toolchain, "//:includer", - want.module_name(), collect); + default_toolchain, "//:includer", want, + collect); return output; }; @@ -349,9 +349,10 @@ }); // Prefer the real target over the group that exposes it. EXPECT_EQ( - "Suggestion: Add public_deps = [ \":visible\" ] to :includer (defined at " - "//BUILD.gn:1)\n", - run_suggest(*visible)); + "Suggestion: Add public_deps = [ \":visible\" ] to :includer (defined " + "at //BUILD.gn:1)\n" + " (`gn edit \"add public_deps :visible\" //:includer`)\n", + run_suggest(visible->module_name())); auto invisible = create_target("invisible", Target::SOURCE_SET, [&](Target* t) {}); @@ -360,8 +361,9 @@ "Suggestion: Carefully consider whether you want to change the " "visibility so that you can depend on it\n" "Suggestion: Add public_deps = [ \":invisible\" ] to :includer (defined " - "at //BUILD.gn:1)\n", - run_suggest(*invisible)); + "at //BUILD.gn:1)\n" + " (`gn edit \"add public_deps :invisible\" //:includer`)\n", + run_suggest(invisible->module_name())); auto exposer_invisible = create_target("exposer_invisible", Target::GROUP, [&](Target* t) { @@ -374,9 +376,10 @@ "visibility so that you can depend on one of them\n" "Suggestion: Add one of the following to public_deps in :includer " "(defined at //BUILD.gn:1):\n" - "* :exposer_invisible\n" - "* :invisible\n", - run_suggest(*invisible)); + "* :exposer_invisible (`gn edit \"add public_deps :exposer_invisible\" " + "//:includer`)\n" + "* :invisible (`gn edit \"add public_deps :invisible\" //:includer`)\n", + run_suggest(invisible->module_name())); auto exposer_visible = create_target("exposer_visible", Target::GROUP, [&](Target* t) { @@ -385,8 +388,9 @@ }); EXPECT_EQ( "Suggestion: Add public_deps = [ \":exposer_visible\" ] to :includer " - "(defined at //BUILD.gn:1)\n", - run_suggest(*invisible)); + "(defined at //BUILD.gn:1)\n" + " (`gn edit \"add public_deps :exposer_visible\" //:includer`)\n", + run_suggest(invisible->module_name())); auto exposer_visible2 = create_target("exposer_visible2", Target::GROUP, [&](Target* t) { @@ -399,9 +403,11 @@ "targets is visible to //:includer\n" "Suggestion: Add one of the following to public_deps in :includer " "(defined at //BUILD.gn:1):\n" - "* :exposer_visible\n" - "* :exposer_visible2\n", - run_suggest(*invisible)); + "* :exposer_visible (`gn edit \"add public_deps :exposer_visible\" " + "//:includer`)\n" + "* :exposer_visible2 (`gn edit \"add public_deps :exposer_visible2\" " + "//:includer`)\n", + run_suggest(invisible->module_name())); auto exposer_specific = create_target("exposer_specific", Target::GROUP, [&](Target* t) { @@ -410,8 +416,9 @@ }); EXPECT_EQ( "Suggestion: Add public_deps = [ \":exposer_specific\" ] to :includer " - "(defined at //BUILD.gn:1)\n", - run_suggest(*invisible)); + "(defined at //BUILD.gn:1)\n" + " (`gn edit \"add public_deps :exposer_specific\" //:includer`)\n", + run_suggest(invisible->module_name())); auto cyclic = create_target("cyclic", Target::SOURCE_SET, [&](Target* t) { t->public_deps().push_back(LabelTargetPair(includer.get())); @@ -426,8 +433,9 @@ "Suggestion: Find the part of the dependency chain where there is no " "#include and remove that dependency.\n" "Suggestion: Add public_deps = [ \":cyclic\" ] to :includer (defined at " - "//BUILD.gn:1)\n", - run_suggest(*cyclic)); + "//BUILD.gn:1)\n" + " (`gn edit \"add public_deps :cyclic\" //:includer`)\n", + run_suggest(cyclic->module_name())); auto cyclic_circular_includes = create_target( "cyclic_circular_includes", Target::STATIC_LIBRARY, [&](Target* t) { @@ -458,8 +466,26 @@ " # public_deps, and any link variables from :cyclic_circular_includes\n" "}\n" "Suggestion: Add public_deps = [ \":cyclic_circular_includes_sources\" ] " - "to " - ":includer " - "(defined at //BUILD.gn:1)\n", - run_suggest(*cyclic_circular_includes)); + "to :includer (defined at //BUILD.gn:1)\n" + " (`gn edit \"add public_deps :cyclic_circular_includes_sources\" " + "//:includer`)\n", + run_suggest(cyclic_circular_includes->module_name())); + + auto private_target = + create_target("private_target", Target::SOURCE_SET, [&](Target* t) { + t->set_all_headers_public(false); + t->sources().push_back(SourceFile("//private_target.h")); + t->visibility().SetPublic(); + }); + EXPECT_EQ( + "Warning: \"private_target_Private\" is in the private API of " + "//:private_target\n" + "Suggestion: Move \"private_target.h\" from `sources` to `public` " + "in :private_target (defined at //BUILD.gn:1)\n" + " (`gn edit \"move sources public private_target.h\" " + "//:private_target`)\n" + "Suggestion: Add public_deps = [ \":private_target\" ] to :includer " + "(defined at //BUILD.gn:1)\n" + " (`gn edit \"add public_deps :private_target\" //:includer`)\n", + run_suggest("private_target_Private")); }