diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h index 72694788cfbfa..0e48d2f0ceb66 100644 --- a/clang-tools-extra/clangd/Config.h +++ b/clang-tools-extra/clangd/Config.h @@ -86,6 +86,7 @@ struct Config { ExternalIndexSpec External; } Index; + enum UnusedIncludesPolicy { Strict, None }; /// Controls warnings and errors when parsing code. struct { bool SuppressAll = false; @@ -97,6 +98,8 @@ struct Config { 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 index ad117b3d77be4..a7b881a28ac06 100644 --- a/clang-tools-extra/clangd/ConfigCompile.cpp +++ b/clang-tools-extra/clangd/ConfigCompile.cpp @@ -414,6 +414,16 @@ struct FragmentCompiler { 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 index 8e4110e6dba13..a9ca7b462ec1e 100644 --- a/clang-tools-extra/clangd/ConfigFragment.h +++ b/clang-tools-extra/clangd/ConfigFragment.h @@ -210,6 +210,20 @@ struct Fragment { /// This often has other advantages, such as skipping some analysis. std::vector> Suppress; + /// Controls how clangd will correct "unnecessary #include directives. + /// clangd can warn if a header is `#include`d but not used, and suggest + /// removing it. + // + /// Strict means a header is unused if it does not *directly* provide any + /// symbol used in the file. Removing it may still break compilation if it + /// transitively includes headers that are used. This should be fixed by + /// including those headers 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 index 8a493346d2ce9..331c18333045d 100644 --- a/clang-tools-extra/clangd/ConfigYAML.cpp +++ b/clang-tools-extra/clangd/ConfigYAML.cpp @@ -118,6 +118,9 @@ class Parser { 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.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index c4f174993e2d6..6dd69192497fa 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -485,6 +485,9 @@ void toLSPDiags( 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/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h index b9155ae4a7d1e..a422c785db34b 100644 --- a/clang-tools-extra/clangd/Diagnostics.h +++ b/clang-tools-extra/clangd/Diagnostics.h @@ -101,6 +101,7 @@ struct Diag : DiagBase { 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/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp index 4178b27857250..50f5a926111ef 100644 --- a/clang-tools-extra/clangd/IncludeCleaner.cpp +++ b/clang-tools-extra/clangd/IncludeCleaner.cpp @@ -7,9 +7,14 @@ //===----------------------------------------------------------------------===// #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" +#include "llvm/Support/Path.h" namespace clang { namespace clangd { @@ -149,6 +154,20 @@ struct ReferencedFiles { } }; +// 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) { @@ -222,5 +241,47 @@ std::vector computeUnusedIncludes(ParsedAST &AST) { 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", + llvm::sys::path::filename( + Inc->Written.substr(1, Inc->Written.size() - 2), + llvm::sys::path::Style::posix)); + 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); + // FIXME(kirillbobyrev): Removing inclusion might break the code if the + // used headers are only reachable transitively through this one. Suggest + // including them directly instead. + // FIXME(kirillbobyrev): Add fix suggestion for adding IWYU pragmas + // (keep/export) remove the warning once we support IWYU pragmas. + 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/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h index b232c09cae586..19d471e2e761e 100644 --- a/clang-tools-extra/clangd/IncludeCleaner.h +++ b/clang-tools-extra/clangd/IncludeCleaner.h @@ -63,6 +63,9 @@ getUnused(const IncludeStructure &Includes, std::vector computeUnusedIncludes(ParsedAST &AST); +std::vector issueUnusedIncludesDiagnostics(ParsedAST &AST, + llvm::StringRef Code); + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 5a3d909eb9293..a62169c8c241f 100644 --- 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 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, 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 index 564629aa7d3a2..87d8b9d976f0f 100644 --- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp +++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp @@ -244,6 +244,25 @@ TEST_F(ConfigCompileTests, PathSpecMatch) { } } +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 index d8fbf1a949a92..bc88cb1260660 100644 --- a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp +++ b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp @@ -67,6 +67,7 @@ CompileFlags: { Add: [foo, bar] } 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 @@ CompileFlags: { Add: [foo, bar] } 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 index 1b778e4d03880..c3c56bdbbe03b 100644 --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -1479,6 +1479,44 @@ TEST(Diagnostics, Tags) { 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