Deduplicate deps/public_deps and sources/public in gn edit When running `gn check --fix`, if you added a #include to both the .cc and the .h file, you will get the following suggestions: * "add deps //dep" * "add public_deps //dep" This ensures that it will only get added to public_deps. Additionally, change gn suggest to recommend adding to public_deps instead of moving from deps to public_deps. Change-Id: I59074724bb5ed61a3499f1a81d2f3b356a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/26380 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: Matt Stark <msta@google.com>
diff --git a/src/gn/command_suggest.cc b/src/gn/command_suggest.cc index 5009b9a..613e79b 100644 --- a/src/gn/command_suggest.cc +++ b/src/gn/command_suggest.cc
@@ -805,17 +805,8 @@ std::string label_str = label.dir() == includer->label().dir() ? ":" + label.name() : label.GetUserVisibleName(current_toolchain); - bool is_move = - dep_field == "public_deps" && - std::ranges::any_of(includer->private_deps(), [&](const auto& dep) { - return dep.ptr == target || dep.label == target->label(); - }); EditCommand edit{ - .command = - is_move ? std::vector<std::string>{"move", "deps", "public_deps", - label_str} - : std::vector<std::string>{"add", std::string(dep_field), - label_str}, + .command = {"add", std::string(dep_field), label_str}, .target = includer->label().GetUserVisibleName(current_toolchain), }; candidate_deps.push_back({target, std::move(label_str), std::move(edit)}); @@ -836,19 +827,9 @@ SetAmbiguous(); StartSuggestion(); - auto is_move = [](const auto& c) { return c.edit.command[0] == "move"; }; - if (std::ranges::all_of(candidate_deps, is_move)) { - OutputString( - "Move one of the following from `deps` to `public_deps` in "); - } else if (std::ranges::any_of(candidate_deps, is_move)) { - OutputString("Add or move one of the following to "); - OutputString(dep_field); - OutputString(" in "); - } else { - OutputString("Add one of the following to "); - OutputString(dep_field); - OutputString(" in "); - } + OutputString("Add one of the following to "); + OutputString(dep_field); + OutputString(" in "); OutputDefinition(includer); OutputString(":\n"); for (const auto& c : candidate_deps) {
diff --git a/src/gn/command_suggest_unittest.cc b/src/gn/command_suggest_unittest.cc index cf8c876..68fce78 100644 --- a/src/gn/command_suggest_unittest.cc +++ b/src/gn/command_suggest_unittest.cc
@@ -438,9 +438,9 @@ includer->private_deps().push_back(LabelTargetPair(visible.get())); EXPECT_EQ( - "Suggestion: Move \":visible\" from `deps` to `public_deps` in :includer " + "Suggestion: Add public_deps = [ \":visible\" ] to :includer " "(defined at //BUILD.gn:1)\n" - " (`gn edit \"move deps public_deps :visible\" //:includer`)\n", + " (`gn edit \"add public_deps :visible\" //:includer`)\n", run_suggest(visible->module_name())); includer->private_deps().clear(); @@ -708,8 +708,8 @@ "private dependencies.\n" "#include \"included/included.h\"\n" " ^\n" - "[APPLIED] Suggestion: Move \"//included:included\" from `deps` to " - "`public_deps` in :includer (defined at //includer/BUILD.gn:1)\n", + "[APPLIED] Suggestion: Add public_deps = [ \"//included:included\" ] to " + ":includer (defined at //includer/BUILD.gn:1)\n", output); std::string expected_build_gn = R"(source_set("includer") { check_includes_strict = true
diff --git a/src/gn/edit_command_unittest.cc b/src/gn/edit_command_unittest.cc index 21a2472..0f2f4ea 100644 --- a/src/gn/edit_command_unittest.cc +++ b/src/gn/edit_command_unittest.cc
@@ -131,9 +131,10 @@ } TEST_F(EditCommandTest, AddSubcommand) { - EXPECT_SUCCESS(DoEdit("add deps //add1 //add2 //add3 :dep2", + EXPECT_SUCCESS(DoEdit("add deps //add1 //add2 //add3 :dep2 //in_public_deps", R"( executable("foo") { + public_deps = [ "//in_public_deps" ] deps = [ "//dep1" ] deps += [ "//:dep2" ] if (is_linux) { @@ -143,6 +144,7 @@ )"), Edited(R"( executable("foo") { + public_deps = [ "//in_public_deps" ] deps = [ "//add1", "//add2", @@ -204,15 +206,17 @@ } )")); - EXPECT_SUCCESS(DoEdit("add deps //base", + EXPECT_SUCCESS(DoEdit("add sources bar.cc foo.h", R"( executable("foo") { - deps = other_deps + public = [ "foo.h" ] + sources = other_sources } )"), Edited(R"( executable("foo") { - deps = [ "//base" ] + other_deps + public = [ "foo.h" ] + sources = [ "bar.cc" ] + other_sources } )")); @@ -220,7 +224,10 @@ R"( executable("foo") { sources = [ "foo.cc" ] - deps = [ "//dep" ] + deps = [ + "//base", + "//dep", + ] } )"), Edited(R"( @@ -231,10 +238,13 @@ } )")); - EXPECT_SUCCESS(DoEdit("add deps //base", + EXPECT_SUCCESS(DoEdit("add public foo.h", R"( executable("foo") { - sources = [ "foo.cc" ] + sources = [ + "foo.cc", + "foo.h", + ] if (is_linux) { deps = [ "//dep" ] } @@ -243,9 +253,9 @@ Edited(R"( executable("foo") { sources = [ "foo.cc" ] - deps = [ "//base" ] + public = [ "foo.h" ] if (is_linux) { - deps += [ "//dep" ] + deps = [ "//dep" ] } } )"));
diff --git a/src/gn/edit_subcommands.cc b/src/gn/edit_subcommands.cc index 9f6dd23..7232289 100644 --- a/src/gn/edit_subcommands.cc +++ b/src/gn/edit_subcommands.cc
@@ -165,15 +165,50 @@ return done; } +// Returns whether |target| contains |value| in |attribute|. +// Assignments using `-=` are filtered out. +bool AttributeContainsValue(const EditTarget& target, + std::string_view attribute, + const Value& value) { + for (const auto& assignment : target.assignments(attribute)) { + if (const auto* op = assignment.node()->AsBinaryOp(); + op && op->op().type() == Token::MINUS_EQUALS) { + continue; + } + if (!FindListElementInAssignment(target, assignment, value).empty()) { + return true; + } + } + return false; +} + void AddToTarget(BuildFile& build_file, const EditTarget& target, const std::string& attribute, - const std::vector<Value>& values) { + const std::vector<Value>& values, + EditState& state) { auto assignments = target.assignments(attribute); - std::vector<Value> to_add = values; - - // Iterate over a copy of values since we're mutating it. + std::vector<Value> to_add; + to_add.reserve(values.size()); for (const auto& value : values) { + // Add deps when in public_deps -> no-op + // Add public_deps when in deps -> remove from deps + // Same for sources / public + if (attribute == "deps" && + AttributeContainsValue(target, "public_deps", value)) { + continue; + } else if (attribute == "sources" && + AttributeContainsValue(target, "public", value)) { + continue; + } else if (attribute == "public_deps") { + RemoveFromTarget(target, "deps", value, state, + /*warn_if_missing=*/false); + } else if (attribute == "public") { + RemoveFromTarget(target, "sources", value, state, + /*warn_if_missing=*/false); + } + + bool already_present_unconditionally = false; for (auto& assignment : assignments) { auto matches = FindListElementInAssignment(target, assignment, value); for (const auto& match : matches) { @@ -186,10 +221,18 @@ } else { // If it's added unconditionally, we don't need to worry about // adding it anymore. - std::erase(to_add, value); + already_present_unconditionally = true; } } } + + if (!already_present_unconditionally) { + to_add.push_back(value); + } + } + + if (to_add.empty()) { + return; } if (const auto* first = FirstUnconditionalAssignment(assignments); first) { @@ -257,7 +300,7 @@ [attribute = std::move(attribute), values = std::move(values)]( BuildFile& build_file, const EditTarget& target, EditState& state) -> Err { - AddToTarget(build_file, target, attribute, values); + AddToTarget(build_file, target, attribute, values, state); return Ok(); }); } @@ -270,23 +313,6 @@ }); } -// Returns whether |target| contains |value| in |attribute|. -// Assignments using `-=` are filtered out. -bool AttributeContainsValue(const EditTarget& target, - std::string_view attribute, - const Value& value) { - for (const auto& assignment : target.assignments(attribute)) { - if (const auto* op = assignment.node()->AsBinaryOp(); - op && op->op().type() == Token::MINUS_EQUALS) { - continue; - } - if (!FindListElementInAssignment(target, assignment, value).empty()) { - return true; - } - } - return false; -} - EditCommand MoveCommand(std::string from_attribute, std::string to_attribute, std::vector<Value> values) { @@ -305,7 +331,7 @@ } } if (!moved_values.empty()) { - AddToTarget(build_file, target, to_attribute, moved_values); + AddToTarget(build_file, target, to_attribute, moved_values, state); } return Ok(); });