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 UnusedHeadersPolicy { Strict, None }; /// Controls warnings and errors when parsing code. struct { bool SuppressAll = false; @@ -97,6 +98,10 @@ std::string Checks; llvm::StringMap CheckOptions; } ClangTidy; + + struct { + UnusedHeadersPolicy UnusedHeaders = None; + } IncludeCleaner; } 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,7 @@ C.Diagnostics.Suppress.insert(N); }); + compile(std::move(F.IncludeCleaner)); compile(std::move(F.ClangTidy)); } @@ -458,6 +459,18 @@ CurSpec += Str; } + void compile(Fragment::DiagnosticsBlock::IncludeCleanerBlock &&F) { + if (F.UnusedHeaders) + if (auto Val = compileEnum("IncludeCleaner", + **F.UnusedHeaders) + .map("Strict", Config::UnusedHeadersPolicy::Strict) + .map("None", Config::UnusedHeadersPolicy::None) + .value()) + Out.Apply.push_back([Val](const Params &, Config &C) { + C.Diagnostics.IncludeCleaner.UnusedHeaders = *Val; + }); + } + void compile(Fragment::DiagnosticsBlock::ClangTidyBlock &&F) { std::string Checks; for (auto &CheckGlob : F.Add) 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. + struct IncludeCleanerBlock { + /// When set to Strict, IncludeCleaner 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. 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> UnusedHeaders; + }; + IncludeCleanerBlock IncludeCleaner; + /// 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,10 +118,19 @@ if (auto Values = scalarValues(N)) F.Suppress = std::move(*Values); }); + Dict.handle("IncludeCleaner", [&](Node &N) { parse(F.IncludeCleaner, N); }); Dict.handle("ClangTidy", [&](Node &N) { parse(F.ClangTidy, N); }); Dict.parse(N); } + void parse(Fragment::DiagnosticsBlock::IncludeCleanerBlock &F, Node &N) { + DictParser Dict("IncludeCleaner", this); + Dict.handle("UnusedHeaders", [&](Node &N) { + F.UnusedHeaders = scalarValue(N, "UnusedIncludes"); + }); + Dict.parse(N); + } + void parse(Fragment::DiagnosticsBlock::ClangTidyBlock &F, Node &N) { DictParser Dict("ClangTidy", this); Dict.handle("Add", [&](Node &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 @@ -21,6 +21,7 @@ #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDE_CLEANER_H #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDE_CLEANER_H +#include "Config.h" #include "Headers.h" #include "ParsedAST.h" #include "clang/Basic/SourceLocation.h" @@ -63,6 +64,10 @@ std::vector computeUnusedIncludes(ParsedAST &AST); +std::vector issueUnusedHeadersDiagnostics(ParsedAST &AST, + const Config &Cfg, + 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,6 +7,9 @@ //===----------------------------------------------------------------------===// #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" @@ -133,6 +136,31 @@ } }; +// Returns the range starting at '#' and ending at EOL. +clangd::Range getDiagnosticRange(llvm::StringRef Code, unsigned Line, + unsigned HashOffset) { + clangd::Range Result; + Result.start.line = Line; + Result.end.line = Line; + + // Calculate the offset from beginning of the file, EOL to '#'. + llvm::StringRef Prefix = Code.drop_back(Code.size() - HashOffset); + while (!Prefix.empty() && Prefix.back() != '\n' && Prefix.back() != '\r') { + Prefix = Prefix.drop_back(); + } + unsigned HashLineOffset = lspLength( + Code.drop_back(Code.size() - HashOffset).drop_front(Prefix.size())); + + // Span the warning until the EOL or EOF. + Result.start.character = HashLineOffset; + Result.end.character = + HashLineOffset + + lspLength(Code.drop_front(HashOffset).take_until([](char C) { + return C == '\n' || C == '\r'; + })); + return Result; +} + } // namespace ReferencedLocations findReferencedLocations(ParsedAST &AST) { @@ -206,5 +234,39 @@ return getUnused(AST.getIncludeStructure(), ReferencedFiles); } +std::vector issueUnusedHeadersDiagnostics(ParsedAST &AST, + const Config &Cfg, + llvm::StringRef Code) { + if (Cfg.Diagnostics.IncludeCleaner.UnusedHeaders != + Config::UnusedHeadersPolicy::Strict || + Cfg.Diagnostics.SuppressAll || + Cfg.Diagnostics.Suppress.contains("clangd-unused-header")) + 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 = "included header is not used"; + D.Name = "unused-header"; + D.Source = Diag::DiagSource::Clangd; + D.File = FileName; + D.Severity = DiagnosticsEngine::Warning; + D.Tags.push_back(Unnecessary); + D.Range = getDiagnosticRange(Code, Inc->HashLine, Inc->HashOffset); + D.Fixes.emplace_back(); + D.Fixes.back().Message = "remove unused header"; + 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" @@ -342,6 +343,7 @@ llvm::Optional FixIncludes; // No need to run clang-tidy or IncludeFixerif we are not going to surface // diagnostics. + const Config &Cfg = Config::current(); if (PreserveDiags) { trace::Span Tracer("ClangTidyInit"); tidy::ClangTidyOptions ClangTidyOpts = @@ -366,7 +368,6 @@ Check->registerMatchers(&CTFinder); } - const Config &Cfg = Config::current(); ASTDiags.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel, const clang::Diagnostic &Info) { if (Cfg.Diagnostics.SuppressAll || @@ -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 = + issueUnusedHeadersDiagnostics(Result, Cfg, 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.IncludeCleaner.UnusedHeaders, + Config::UnusedHeadersPolicy::None); + + Frag = {}; + Frag.Diagnostics.IncludeCleaner.UnusedHeaders.emplace("None"); + EXPECT_TRUE(compileAndApply()); + EXPECT_EQ(Conf.Diagnostics.IncludeCleaner.UnusedHeaders, + Config::UnusedHeadersPolicy::None); + + Frag = {}; + Frag.Diagnostics.IncludeCleaner.UnusedHeaders.emplace("Strict"); + EXPECT_TRUE(compileAndApply()); + EXPECT_EQ(Conf.Diagnostics.IncludeCleaner.UnusedHeaders, + Config::UnusedHeadersPolicy::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,8 @@ CheckOptions: IgnoreMacros: true example-check.ExampleOption: 0 + IncludeCleaner: + UnusedHeaders: Strict )yaml"; auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback()); EXPECT_THAT(Diags.Diagnostics, IsEmpty()); @@ -83,6 +85,9 @@ EXPECT_THAT(Results[3].Diagnostics.ClangTidy.CheckOptions, ElementsAre(PairVal("IgnoreMacros", "true"), PairVal("example-check.ExampleOption", "0"))); + EXPECT_TRUE(Results[3].Diagnostics.IncludeCleaner.UnusedHeaders); + EXPECT_EQ("Strict", + *Results[3].Diagnostics.IncludeCleaner.UnusedHeaders.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 @@ -1464,6 +1464,45 @@ 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.IncludeCleaner.UnusedHeaders = + Config::UnusedHeadersPolicy::Strict; + WithContextValue WithCfg(Config::Key, std::move(Cfg)); + EXPECT_THAT( + *TU.build().getDiagnostics(), + UnorderedElementsAre( + AllOf(Diag(Test.range("diag"), "included header is not used"), + WithTag(DiagnosticTag::Unnecessary), DiagSource(Diag::Clangd), + WithFix(Fix(Test.range("fix"), "", "remove unused header"))))); + Cfg.Diagnostics.SuppressAll = true; + WithContextValue SuppressAllWithCfg(Config::Key, std::move(Cfg)); + EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty()); + Cfg.Diagnostics.SuppressAll = false; + Cfg.Diagnostics.Suppress = {"clangd-unused-header"}; + WithContextValue SuppressFilterWithCfg(Config::Key, std::move(Cfg)); + EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty()); +} + } // namespace } // namespace clangd } // namespace clang