Implement `gn check --fix` Change-Id: I5155d94bcae10a8f0b7243c7de9826ad6a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/25481 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 6463f38..c55cef6 100644 --- a/docs/reference.md +++ b/docs/reference.md
@@ -366,7 +366,7 @@ given arguments set (which may affect the values of other arguments). ``` -### <a name="cmd_check"></a>**gn check <out_dir> [<label_pattern>] [\--force] [\--check-generated]** [Back to Top](#gn-reference) +### <a name="cmd_check"></a>**gn check <out_dir> [<label_pattern>] [\--force] [\--check-generated] [\--fix]** [Back to Top](#gn-reference) ``` GN's include header checker validates that the includes for C-like source @@ -393,6 +393,9 @@ Check system style includes (using <angle brackets>) in addition to "double quote" includes. + --fix + Automatically apply suggestions to resolve header dependency errors. + --default-toolchain Normally wildcard targets are matched in all toolchains. This switch makes wildcard labels with no explicit toolchain reference
diff --git a/src/gn/command_check.cc b/src/gn/command_check.cc index eba8c70..45331bf 100644 --- a/src/gn/command_check.cc +++ b/src/gn/command_check.cc
@@ -4,6 +4,8 @@ #include <stddef.h> +#include <tuple> + #include "base/command_line.h" #include "base/strings/stringprintf.h" #include "gn/commands.h" @@ -55,7 +57,7 @@ const char kCheck[] = "check"; const char kCheck_HelpShort[] = "check: Check header dependencies."; const char kCheck_Help[] = - R"(gn check <out_dir> [<label_pattern>] [--force] [--check-generated] + R"(gn check <out_dir> [<label_pattern>] [--force] [--check-generated] [--fix] GN's include header checker validates that the includes for C-like source files match the build dependency graph. @@ -79,6 +81,9 @@ Check system style includes (using <angle brackets>) in addition to "double quote" includes. + --fix + Automatically apply suggestions to resolve header dependency errors. + )" DEFAULT_TOOLCHAIN_SWITCH_HELP R"( --force @@ -241,10 +246,11 @@ bool check_generated = cmdline->HasSwitch("check-generated"); bool check_system = setup->check_system_includes() || cmdline->HasSwitch("check-system"); + bool fix = cmdline->HasSwitch("fix"); if (!CheckPublicHeaders(&setup->build_settings(), all_targets, targets_to_check, force, check_generated, - check_system)) + check_system, fix, setup)) return 1; if (!base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kQuiet)) { @@ -266,21 +272,83 @@ const std::vector<const Target*>& to_check, bool force_check, bool check_generated, - bool check_system) { + bool check_system, + bool apply, + Setup* setup, + OutputStringFunc output_fn) { ScopedTrace trace(TraceItem::TRACE_CHECK_HEADERS, "Check headers"); + auto OutputString = [&](std::string_view str, + TextDecoration dec = DECORATION_NONE, + HtmlEscaping esc = DEFAULT_ESCAPING) { + if (output_fn) { + output_fn(str, dec, esc); + } else { + ::OutputString(str, dec, esc); + } + }; + scoped_refptr<HeaderChecker> header_checker(new HeaderChecker( build_settings, all_targets, check_generated, check_system)); - std::vector<Err> header_errors; - header_checker->Run(to_check, force_check, &header_errors); - for (size_t i = 0; i < header_errors.size(); i++) { - if (i > 0) + std::vector<HeaderChecker::Violation> violations; + header_checker->Run(to_check, force_check, &violations); + + Label default_toolchain = setup ? setup->loader()->default_toolchain_label() + : Label(SourceDir("//toolchain/"), "default"); + + bool remaining_violations = false; + bool needs_separator = false; + for (auto& violation : violations) { + if (needs_separator) { OutputString("___________________\n", DECORATION_YELLOW); - if (!header_errors[i].PrintToStdout()) + needs_separator = false; + } + bool fixed = false; + std::vector<std::tuple<std::string, TextDecoration, HtmlEscaping>> buf; + if (!violation.source_file.is_null() && + !violation.included_file.is_null()) { + SuggestResult exit_code = OutputSuggestions( + all_targets, build_settings, default_toolchain, + violation.source_file.value(), violation.included_file.value(), + [&](std::string_view str, TextDecoration dec, HtmlEscaping esc) { + buf.emplace_back(str, dec, esc); + }, + apply, setup); + fixed = apply && (exit_code == SuggestResult::kSuccess); + } + + auto& err = violation.error; + if (fixed) { + // Strip the help text. We shouldn't be overly verbose if we've already + // fixed the problem, just provide the context of the #include we were + // trying to fix. + err = Err(err.location(), err.message()); + } else { + remaining_violations = true; + } + + bool printed = true; + if (output_fn) { + OutputString(err.to_string()); + } else { + printed = err.PrintToStdout(); + } + + if (printed) { + for (const auto& [str, dec, esc] : buf) { + OutputString(str, dec, esc); + } + needs_separator = true; + } else if (apply) { + // If we're applying suggestions and only configured to print a single + // error, we can't yet stop because we might be able to apply more. + } else { break; + } } - return header_errors.empty(); + + return !remaining_violations; } } // namespace commands
diff --git a/src/gn/command_suggest_unittest.cc b/src/gn/command_suggest_unittest.cc index 4921207..57a166f 100644 --- a/src/gn/command_suggest_unittest.cc +++ b/src/gn/command_suggest_unittest.cc
@@ -38,8 +38,15 @@ files.try_emplace(SourceFile("//.gn"), "buildconfig = \"//BUILDCONFIG.gn\"\n"); - files.try_emplace(SourceFile("//BUILDCONFIG.gn"), - "set_default_toolchain(\"//toolchain:default\")\n"); + files.try_emplace(SourceFile("//BUILDCONFIG.gn"), R"( +set_default_toolchain("//toolchain:default") +set_defaults("executable") { + include_dirs = [ "//" ] +} +set_defaults("source_set") { + include_dirs = [ "//" ] +} +)"); files.try_emplace(SourceFile("//toolchain/BUILD.gn"), R"( toolchain("default") { tool("cxx") { @@ -599,4 +606,50 @@ } )"; EXPECT_EQ(expected_build_gn, project.Read(SourceFile("//BUILD.gn"))); +} + +TEST_F(SuggestTest, CheckAppliesSuggestions) { + TestProject project({ + {SourceFile("//BUILD.gn"), R"( +group("all") { + deps = [ + "//included", + "//includer", + ] +} +)"}, + {SourceFile("//includer/BUILD.gn"), R"(executable("includer") { + sources = [ "includer.cc" ] +} +)"}, + {SourceFile("//included/BUILD.gn"), R"(source_set("included") { + sources = [ "included.h" ] +} +)"}, + {SourceFile("//includer/includer.cc"), + "#include \"included/included.h\""}, + {SourceFile("//included/included.h"), ""}, + }); + + std::string output; + auto collect = [&](std::string_view s, TextDecoration, HtmlEscaping) { + output.append(s); + }; + + EXPECT_TRUE(commands::CheckPublicHeaders( + &project.setup.build_settings(), project.targets(), project.targets(), + false, false, false, true, &project.setup, collect)); + EXPECT_EQ( + "ERROR at //includer/includer.cc:1:11: Include not allowed.\n" + "#include \"included/included.h\"\n" + " ^\n" + "[APPLIED] Suggestion: Add deps = [ \"//included:included\" ] to " + ":includer (defined at //includer/BUILD.gn:1)\n", + output); + std::string expected_build_gn = R"(executable("includer") { + sources = [ "includer.cc" ] + deps = [ "//included" ] +} +)"; + EXPECT_EQ(expected_build_gn, project.Read(SourceFile("//includer/BUILD.gn"))); } \ No newline at end of file
diff --git a/src/gn/commands.h b/src/gn/commands.h index a04a88e..0cc9c8d 100644 --- a/src/gn/commands.h +++ b/src/gn/commands.h
@@ -346,7 +346,10 @@ const std::vector<const Target*>& to_check, bool force_check, bool check_generated, - bool check_system); + bool check_system, + bool apply = false, + Setup* setup = nullptr, + OutputStringFunc output_fn = nullptr); // Filters the given list of targets by the given pattern list. void FilterTargetsByPatterns(const std::vector<const Target*>& input,
diff --git a/src/gn/header_checker.cc b/src/gn/header_checker.cc index 3e3a9f1..f7c7632 100644 --- a/src/gn/header_checker.cc +++ b/src/gn/header_checker.cc
@@ -178,7 +178,7 @@ bool HeaderChecker::Run(const std::vector<const Target*>& to_check, bool force_check, - std::vector<Err>* errors) { + std::vector<Violation>* violations) { FileMap files_to_check; for (auto* check : to_check) { // This function will get called with all target types, but check only @@ -227,9 +227,10 @@ RunCheckOverFiles(files_to_check, force_check, &pool); - if (errors_.empty()) + if (violations_.empty()) return true; - *errors = errors_; + if (violations) + *violations = violations_; return false; } @@ -284,10 +285,12 @@ void HeaderChecker::DoWork(const TargetVector& targets, const SourceFile& file) { - std::vector<Err> errors; - if (!CheckFile(targets, file, &errors)) { + std::vector<Violation> violations; + if (!CheckFile(targets, file, &violations)) { std::lock_guard<std::mutex> lock(errors_lock_); - errors_.insert(errors_.end(), errors.begin(), errors.end()); + violations_.insert(violations_.end(), + std::make_move_iterator(violations.begin()), + std::make_move_iterator(violations.end())); } if (!task_count_.Decrement()) { @@ -488,7 +491,7 @@ bool HeaderChecker::CheckFile(const TargetVector& targets, const SourceFile& file, - std::vector<Err>* errors) const { + std::vector<Violation>* violations) const { ScopedTrace trace(TraceItem::TRACE_CHECK_HEADER, file.value()); // Sometimes you have generated source files included as sources in another @@ -508,11 +511,13 @@ for (const TargetInfo& from_target_info : targets) { const Target* from_target = from_target_info.target; - errors->emplace_back( - from_target->defined_from(), "Source file not found.", - "The target:\n " + from_target->label().GetUserVisibleName(false) + - "\nhas a source file:\n " + file.value() + - "\nwhich was not found."); + violations->emplace_back( + Err(from_target->defined_from(), "Source file not found.", + "The target:\n " + + from_target->label().GetUserVisibleName(false) + + "\nhas a source file:\n " + file.value() + + "\nwhich was not found."), + file, SourceFile()); } return false; } @@ -532,7 +537,7 @@ if (includes.empty()) return true; - size_t error_count_before = errors->size(); + size_t violations_count_before = violations->size(); for (const TargetInfo& from_target_info : targets) { const Target* from_target = from_target_info.target; @@ -553,15 +558,20 @@ SourceFile included_file = SourceFileForInclude(inc, include_dirs, input_file, &err); if (!included_file.is_null()) { + std::vector<Err> include_errors; CheckInclude(from_target_cache, from_target_info.is_public && file.GetType() == SourceFile::SOURCE_H, - input_file, included_file, inc.location, errors); + input_file, included_file, inc.location, &include_errors); + for (auto& e : include_errors) { + violations->emplace_back(std::move(e), file, + std::move(included_file)); + } } } } - return errors->size() == error_count_before; + return violations->size() == violations_count_before; } // If the file exists:
diff --git a/src/gn/header_checker.h b/src/gn/header_checker.h index 60f485d..0fee4b6 100644 --- a/src/gn/header_checker.h +++ b/src/gn/header_checker.h
@@ -24,10 +24,10 @@ #include "gn/err.h" #include "gn/hash_table_base.h" #include "gn/source_dir.h" +#include "gn/source_file.h" class BuildSettings; class InputFile; -class SourceFile; class Target; class WorkerPool; @@ -54,6 +54,19 @@ }; using Chain = std::vector<ChainLink>; + // Represents a header dependency violation found during checking. + struct Violation { + // The diagnostic error describing the violation. + Err error; + + // The source file that contained the invalid #include directive. + SourceFile source_file; + + // The header file that was included without appropriate build dependency. + // May be null if we were unable to find the header file. + SourceFile included_file; + }; + // check_generated, if true, will also check generated // files. Something that can only be done after running a build that // has generated them. @@ -65,14 +78,14 @@ // Runs the check. The targets in to_check will be checked. // // This assumes that the current thread already has a message loop. On - // error, fills the given vector with the errors and returns false. Returns - // true on success. + // error, fills the given vector with the violations and returns false. + // Returns true on success. // // force_check, if true, will override targets opting out of header checking // with "check_includes = false" and will check them anyway. bool Run(const std::vector<const Target*>& to_check, bool force_check, - std::vector<Err>* errors); + std::vector<Violation>* violations); private: friend class base::RefCountedThreadSafe<HeaderChecker>; @@ -270,7 +283,7 @@ // error messages. bool CheckFile(const TargetVector& targets, const SourceFile& file, - std::vector<Err>* errors) const; + std::vector<Violation>* violations) const; // Checks that the given file in the given target can include the // given include file. If disallowed, adds the error or errors to @@ -341,7 +354,7 @@ mutable std::mutex errors_lock_; - std::vector<Err> errors_; + std::vector<Violation> violations_; mutable std::array<DependencyCacheShard, kNumShards> dependency_cache_;
diff --git a/src/gn/setup.cc b/src/gn/setup.cc index c0f4c39..47b0282 100644 --- a/src/gn/setup.cc +++ b/src/gn/setup.cc
@@ -598,8 +598,11 @@ to_check = all_targets; } + bool fix = cmdline.HasSwitch("fix"); + if (!commands::CheckPublicHeaders(&build_settings_, all_targets, to_check, - false, false, check_system_includes_)) { + false, false, check_system_includes_, fix, + this)) { return false; } }