diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h --- a/clang-tools-extra/clangd/Config.h +++ b/clang-tools-extra/clangd/Config.h @@ -86,6 +86,7 @@ ExternalIndexSpec External; } Index; + enum UnusedIncludesPolicy { Strict, None }; /// Controls warnings and errors when parsing code. struct { bool SuppressAll = false; @@ -97,6 +98,8 @@ std::string Checks; llvm::StringMap CheckOptions; } ClangTidy; + + UnusedIncludesPolicy UnusedIncludes = None; } Diagnostics; /// Style of the codebase. diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp --- a/clang-tools-extra/clangd/ConfigCompile.cpp +++ b/clang-tools-extra/clangd/ConfigCompile.cpp @@ -414,6 +414,16 @@ C.Diagnostics.Suppress.insert(N); }); + if (F.UnusedIncludes) + if (auto Val = compileEnum( + "UnusedIncludes", **F.UnusedIncludes) + .map("Strict", Config::UnusedIncludesPolicy::Strict) + .map("None", Config::UnusedIncludesPolicy::None) + .value()) + Out.Apply.push_back([Val](const Params &, Config &C) { + C.Diagnostics.UnusedIncludes = *Val; + }); + compile(std::move(F.ClangTidy)); } diff --git a/clang-tools-extra/clangd/ConfigFragment.h b/clang-tools-extra/clangd/ConfigFragment.h --- a/clang-tools-extra/clangd/ConfigFragment.h +++ b/clang-tools-extra/clangd/ConfigFragment.h @@ -210,6 +210,22 @@ /// This often has other advantages, such as skipping some analysis. std::vector> Suppress; + /// Controls how clangd will treat and warn users on suboptimal include + /// usage. + /// + /// When set to Strict, clangd will warn about all headers that are not used + /// (no symbols referenced in the main file come from that header) in the + /// main file but are directly included from it. + /// + /// FIXME(kirillbobyrev): Removing that header might break the code if it + /// transitively includes used headers and these headers are not included + /// directly. + /// + /// Valid values are: + /// - Strict + /// - None + llvm::Optional> UnusedIncludes; + /// Controls how clang-tidy will run over the code base. /// /// The settings are merged with any settings found in .clang-tidy diff --git a/clang-tools-extra/clangd/ConfigYAML.cpp b/clang-tools-extra/clangd/ConfigYAML.cpp --- a/clang-tools-extra/clangd/ConfigYAML.cpp +++ b/clang-tools-extra/clangd/ConfigYAML.cpp @@ -118,6 +118,9 @@ if (auto Values = scalarValues(N)) F.Suppress = std::move(*Values); }); + Dict.handle("UnusedIncludes", [&](Node &N) { + F.UnusedIncludes = scalarValue(N, "UnusedIncludes"); + }); Dict.handle("ClangTidy", [&](Node &N) { parse(F.ClangTidy, N); }); Dict.parse(N); } diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h --- a/clang-tools-extra/clangd/Diagnostics.h +++ b/clang-tools-extra/clangd/Diagnostics.h @@ -101,6 +101,7 @@ Unknown, Clang, ClangTidy, + Clangd, ClangdConfig, } Source = Unknown; /// Elaborate on the problem, usually pointing to a related piece of code. diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -485,6 +485,9 @@ case Diag::ClangTidy: Main.source = "clang-tidy"; break; + case Diag::Clangd: + Main.source = "clangd"; + break; case Diag::ClangdConfig: Main.source = "clangd-config"; break; diff --git a/clang-tools-extra/clangd/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h --- a/clang-tools-extra/clangd/IncludeCleaner.h +++ b/clang-tools-extra/clangd/IncludeCleaner.h @@ -63,6 +63,9 @@ std::vector computeUnusedIncludes(ParsedAST &AST); +std::vector issueUnusedIncludesDiagnostics(ParsedAST &AST, + llvm::StringRef Code); + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp --- a/clang-tools-extra/clangd/IncludeCleaner.cpp +++ b/clang-tools-extra/clangd/IncludeCleaner.cpp @@ -7,9 +7,13 @@ //===----------------------------------------------------------------------===// #include "IncludeCleaner.h" +#include "Config.h" +#include "Protocol.h" +#include "SourceCode.h" #include "support/Logger.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/Basic/SourceLocation.h" +#include "llvm/Support/FormatVariadic.h" namespace clang { namespace clangd { @@ -142,6 +146,20 @@ } }; +// Returns the range starting at '#' and ending at EOL. Escaped newlines are not +// handled. +clangd::Range getDiagnosticRange(llvm::StringRef Code, unsigned HashOffset) { + clangd::Range Result; + Result.end = Result.start = offsetToPosition(Code, HashOffset); + + // Span the warning until the EOL or EOF. + Result.end.character += + lspLength(Code.drop_front(HashOffset).take_until([](char C) { + return C == '\n' || C == '\r'; + })); + return Result; +} + } // namespace ReferencedLocations findReferencedLocations(ParsedAST &AST) { @@ -215,5 +233,38 @@ return getUnused(AST.getIncludeStructure(), ReferencedFiles); } +std::vector issueUnusedIncludesDiagnostics(ParsedAST &AST, + llvm::StringRef Code) { + const Config &Cfg = Config::current(); + if (Cfg.Diagnostics.UnusedIncludes != Config::UnusedIncludesPolicy::Strict || + Cfg.Diagnostics.SuppressAll || + Cfg.Diagnostics.Suppress.contains("unused-includes")) + return {}; + std::vector Result; + std::string FileName = + AST.getSourceManager() + .getFileEntryForID(AST.getSourceManager().getMainFileID()) + ->getName() + .str(); + for (const auto *Inc : computeUnusedIncludes(AST)) { + Diag D; + D.Message = llvm::formatv("included header {0} is not used", Inc->Written); + D.Name = "unused-includes"; + D.Source = Diag::DiagSource::Clangd; + D.File = FileName; + D.Severity = DiagnosticsEngine::Warning; + D.Tags.push_back(Unnecessary); + D.Range = getDiagnosticRange(Code, Inc->HashOffset); + D.Fixes.emplace_back(); + D.Fixes.back().Message = "remove #include directive"; + D.Fixes.back().Edits.emplace_back(); + D.Fixes.back().Edits.back().range.start.line = Inc->HashLine; + D.Fixes.back().Edits.back().range.end.line = Inc->HashLine + 1; + D.InsideMainFile = true; + Result.push_back(std::move(D)); + } + return Result; +} + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -18,6 +18,7 @@ #include "FeatureModule.h" #include "Headers.h" #include "HeuristicResolver.h" +#include "IncludeCleaner.h" #include "IncludeFixer.h" #include "Preamble.h" #include "SourceCode.h" @@ -515,10 +516,18 @@ Diags->insert(Diags->end(), D.begin(), D.end()); } } - return ParsedAST(Inputs.Version, std::move(Preamble), std::move(Clang), + ParsedAST Result(Inputs.Version, std::move(Preamble), std::move(Clang), std::move(Action), std::move(Tokens), std::move(Macros), std::move(Marks), std::move(ParsedDecls), std::move(Diags), std::move(Includes), std::move(CanonIncludes)); + if (Result.Diags) { + auto UnusedHeadersDiags = + issueUnusedIncludesDiagnostics(Result, Inputs.Contents); + Result.Diags->insert(Result.Diags->end(), + make_move_iterator(UnusedHeadersDiags.begin()), + make_move_iterator(UnusedHeadersDiags.end())); + } + return Result; } ParsedAST::ParsedAST(ParsedAST &&Other) = default; diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp --- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp +++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp @@ -244,6 +244,25 @@ } } +TEST_F(ConfigCompileTests, DiagnosticsIncludeCleaner) { + // Defaults to None. + EXPECT_TRUE(compileAndApply()); + EXPECT_EQ(Conf.Diagnostics.UnusedIncludes, + Config::UnusedIncludesPolicy::None); + + Frag = {}; + Frag.Diagnostics.UnusedIncludes.emplace("None"); + EXPECT_TRUE(compileAndApply()); + EXPECT_EQ(Conf.Diagnostics.UnusedIncludes, + Config::UnusedIncludesPolicy::None); + + Frag = {}; + Frag.Diagnostics.UnusedIncludes.emplace("Strict"); + EXPECT_TRUE(compileAndApply()); + EXPECT_EQ(Conf.Diagnostics.UnusedIncludes, + Config::UnusedIncludesPolicy::Strict); +} + TEST_F(ConfigCompileTests, DiagnosticSuppression) { Frag.Diagnostics.Suppress.emplace_back("bugprone-use-after-move"); Frag.Diagnostics.Suppress.emplace_back("unreachable-code"); diff --git a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp --- a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp +++ b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp @@ -67,6 +67,7 @@ CheckOptions: IgnoreMacros: true example-check.ExampleOption: 0 + UnusedIncludes: Strict )yaml"; auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback()); EXPECT_THAT(Diags.Diagnostics, IsEmpty()); @@ -83,6 +84,8 @@ EXPECT_THAT(Results[3].Diagnostics.ClangTidy.CheckOptions, ElementsAre(PairVal("IgnoreMacros", "true"), PairVal("example-check.ExampleOption", "0"))); + EXPECT_TRUE(Results[3].Diagnostics.UnusedIncludes); + EXPECT_EQ("Strict", *Results[3].Diagnostics.UnusedIncludes.getValue()); } TEST(ParseYAML, Locations) { diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -1479,6 +1479,44 @@ AllOf(Diag(Test.range("deprecated"), "'bar' is deprecated"), WithTag(DiagnosticTag::Deprecated)))); } + +TEST(DiagnosticsTest, IncludeCleaner) { + Annotations Test(R"cpp( +$fix[[ $diag[[#include "unused.h"]] +]] #include "used.h" + + void foo() { + used(); + } + )cpp"); + TestTU TU; + TU.Code = Test.code().str(); + TU.AdditionalFiles["unused.h"] = R"cpp( + void unused() {} + )cpp"; + TU.AdditionalFiles["used.h"] = R"cpp( + void used() {} + )cpp"; + // Off by default. + EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty()); + Config Cfg; + Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Strict; + WithContextValue WithCfg(Config::Key, std::move(Cfg)); + EXPECT_THAT( + *TU.build().getDiagnostics(), + UnorderedElementsAre(AllOf( + Diag(Test.range("diag"), "included header \"unused.h\" is not used"), + WithTag(DiagnosticTag::Unnecessary), DiagSource(Diag::Clangd), + WithFix(Fix(Test.range("fix"), "", "remove #include directive"))))); + Cfg.Diagnostics.SuppressAll = true; + WithContextValue SuppressAllWithCfg(Config::Key, std::move(Cfg)); + EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty()); + Cfg.Diagnostics.SuppressAll = false; + Cfg.Diagnostics.Suppress = {"unused-includes"}; + WithContextValue SuppressFilterWithCfg(Config::Key, std::move(Cfg)); + EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty()); +} + } // namespace } // namespace clangd } // namespace clang