Author: Haojian Wu Date: 2023-01-19T14:31:40+01:00 New Revision: 939dce12f9f35f7e0953a036c16e89d30011d047
URL: https://github.com/llvm/llvm-project/commit/939dce12f9f35f7e0953a036c16e89d30011d047 DIFF: https://github.com/llvm/llvm-project/commit/939dce12f9f35f7e0953a036c16e89d30011d047.diff LOG: [clangd] Implement unused include warnings with include-cleaner library. A prototype of using include-cleaner library in clangd: - (re)implement clangd's "unused include" warnings with the library - the new implementation is hidden under a flag `Config::UnusedIncludesPolicy::Experiment` Differential Revision: https://reviews.llvm.org/D140875 Added: Modified: clang-tools-extra/clangd/CMakeLists.txt clang-tools-extra/clangd/Config.h clang-tools-extra/clangd/ConfigCompile.cpp clang-tools-extra/clangd/IncludeCleaner.cpp clang-tools-extra/clangd/IncludeCleaner.h clang-tools-extra/clangd/ParsedAST.cpp clang-tools-extra/clangd/ParsedAST.h clang-tools-extra/clangd/Preamble.cpp clang-tools-extra/clangd/Preamble.h clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/CMakeLists.txt b/clang-tools-extra/clangd/CMakeLists.txt index a6b6e30df228b..af8a188056738 100644 --- a/clang-tools-extra/clangd/CMakeLists.txt +++ b/clang-tools-extra/clangd/CMakeLists.txt @@ -59,6 +59,7 @@ if(MSVC AND NOT CLANG_CL) endif() include_directories(BEFORE "${CMAKE_CURRENT_BINARY_DIR}/../clang-tidy") +include_directories(BEFORE "${CMAKE_CURRENT_SOURCE_DIR}/../include-cleaner/include") add_clang_library(clangDaemon AST.cpp @@ -162,6 +163,7 @@ clang_target_link_libraries(clangDaemon clangDriver clangFormat clangFrontend + clangIncludeCleaner clangIndex clangLex clangSema diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h index cbc9c79a66249..f41906b2f0faf 100644 --- a/clang-tools-extra/clangd/Config.h +++ b/clang-tools-extra/clangd/Config.h @@ -88,7 +88,13 @@ struct Config { bool StandardLibrary = true; } Index; - enum UnusedIncludesPolicy { Strict, None }; + enum UnusedIncludesPolicy { + /// Diagnose unused includes. + Strict, + None, + /// The same as Strict, but using the include-cleaner library. + Experiment, + }; /// Controls warnings and errors when parsing code. struct { bool SuppressAll = false; diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp index 7dd4e16c8f3cc..b1876e21ee30d 100644 --- a/clang-tools-extra/clangd/ConfigCompile.cpp +++ b/clang-tools-extra/clangd/ConfigCompile.cpp @@ -431,11 +431,13 @@ struct FragmentCompiler { }); if (F.UnusedIncludes) - if (auto Val = compileEnum<Config::UnusedIncludesPolicy>( - "UnusedIncludes", **F.UnusedIncludes) - .map("Strict", Config::UnusedIncludesPolicy::Strict) - .map("None", Config::UnusedIncludesPolicy::None) - .value()) + if (auto Val = + compileEnum<Config::UnusedIncludesPolicy>("UnusedIncludes", + **F.UnusedIncludes) + .map("Strict", Config::UnusedIncludesPolicy::Strict) + .map("Experiment", Config::UnusedIncludesPolicy::Experiment) + .map("None", Config::UnusedIncludesPolicy::None) + .value()) Out.Apply.push_back([Val](const Params &, Config &C) { C.Diagnostics.UnusedIncludes = *Val; }); diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp index dbed805142b71..5a7df2fc33f69 100644 --- a/clang-tools-extra/clangd/IncludeCleaner.cpp +++ b/clang-tools-extra/clangd/IncludeCleaner.cpp @@ -12,6 +12,8 @@ #include "ParsedAST.h" #include "Protocol.h" #include "SourceCode.h" +#include "clang-include-cleaner/Analysis.h" +#include "clang-include-cleaner/Types.h" #include "index/CanonicalIncludes.h" #include "support/Logger.h" #include "support/Trace.h" @@ -458,6 +460,9 @@ translateToHeaderIDs(const ReferencedFiles &Files, return TranslatedHeaderIDs; } +// This is the original clangd-own implementation for computing unused +// #includes. Eventually it will be deprecated and replaced by the +// include-cleaner-lib-based implementation. std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST) { const auto &SM = AST.getSourceManager(); @@ -469,11 +474,62 @@ std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST) { translateToHeaderIDs(ReferencedFiles, AST.getIncludeStructure(), SM); return getUnused(AST, ReferencedHeaders, ReferencedFiles.SpelledUmbrellas); } +std::vector<const Inclusion *> computeUnusedIncludesExperimental(ParsedAST &AST) { + const auto &SM = AST.getSourceManager(); + const auto &Includes = AST.getIncludeStructure(); + // FIXME: this map should probably be in IncludeStructure. + llvm::StringMap<llvm::SmallVector<IncludeStructure::HeaderID>> BySpelling; + for (const auto &Inc : Includes.MainFileIncludes) { + if (Inc.HeaderID) + BySpelling.try_emplace(Inc.Written) + .first->second.push_back( + static_cast<IncludeStructure::HeaderID>(*Inc.HeaderID)); + } + // FIXME: !!this is a hacky way to collect macro references. + std::vector<include_cleaner::SymbolReference> Macros; + auto& PP = AST.getPreprocessor(); + for (const syntax::Token &Tok : + AST.getTokens().spelledTokens(SM.getMainFileID())) { + auto Macro = locateMacroAt(Tok, PP); + if (!Macro) + continue; + if (auto DefLoc = Macro->Info->getDefinitionLoc(); DefLoc.isValid()) + Macros.push_back( + {Tok.location(), + include_cleaner::Macro{/*Name=*/PP.getIdentifierInfo(Tok.text(SM)), + DefLoc}, + include_cleaner::RefType::Explicit}); + } + llvm::DenseSet<IncludeStructure::HeaderID> Used; + include_cleaner::walkUsed( + AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros, + AST.getPragmaIncludes(), SM, + [&](const include_cleaner::SymbolReference &Ref, + llvm::ArrayRef<include_cleaner::Header> Providers) { + for (const auto &H : Providers) { + switch (H.kind()) { + case include_cleaner::Header::Physical: + if (auto HeaderID = Includes.getID(H.physical())) + Used.insert(*HeaderID); + break; + case include_cleaner::Header::Standard: + for (auto HeaderID : Includes.StdlibHeaders.lookup(H.standard())) + Used.insert(HeaderID); + break; + case include_cleaner::Header::Verbatim: + for (auto HeaderID : BySpelling.lookup(H.verbatim())) + Used.insert(HeaderID); + break; + } + } + }); + return getUnused(AST, Used, /*ReferencedPublicHeaders*/{}); +} std::vector<Diag> issueUnusedIncludesDiagnostics(ParsedAST &AST, llvm::StringRef Code) { const Config &Cfg = Config::current(); - if (Cfg.Diagnostics.UnusedIncludes != Config::UnusedIncludesPolicy::Strict || + if (Cfg.Diagnostics.UnusedIncludes == Config::UnusedIncludesPolicy::None || Cfg.Diagnostics.SuppressAll || Cfg.Diagnostics.Suppress.contains("unused-includes")) return {}; @@ -487,7 +543,11 @@ std::vector<Diag> issueUnusedIncludesDiagnostics(ParsedAST &AST, .getFileEntryRefForID(AST.getSourceManager().getMainFileID()) ->getName() .str(); - for (const auto *Inc : computeUnusedIncludes(AST)) { + const auto &UnusedIncludes = + Cfg.Diagnostics.UnusedIncludes == Config::UnusedIncludesPolicy::Experiment + ? computeUnusedIncludesExperimental(AST) + : computeUnusedIncludes(AST); + for (const auto *Inc : UnusedIncludes) { Diag D; D.Message = llvm::formatv("included header {0} is not used directly", diff --git a/clang-tools-extra/clangd/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h index 8d8a5f75b3d3a..a7beb9c3c9d4b 100644 --- a/clang-tools-extra/clangd/IncludeCleaner.h +++ b/clang-tools-extra/clangd/IncludeCleaner.h @@ -97,6 +97,10 @@ getUnused(ParsedAST &AST, const llvm::StringSet<> &ReferencedPublicHeaders); std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST); +// The same as computeUnusedIncludes, but it is an experimental and +// include-cleaner-lib-based implementation. +std::vector<const Inclusion *> +computeUnusedIncludesExperimental(ParsedAST &AST); std::vector<Diag> issueUnusedIncludesDiagnostics(ParsedAST &AST, llvm::StringRef Code); diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index d10da8cf28552..bf639a6fb58e7 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -23,6 +23,7 @@ #include "Preamble.h" #include "SourceCode.h" #include "TidyProvider.h" +#include "clang-include-cleaner/Record.h" #include "index/CanonicalIncludes.h" #include "index/Index.h" #include "index/Symbol.h" @@ -801,6 +802,12 @@ ParsedAST::ParsedAST(PathRef TUPath, llvm::StringRef Version, assert(this->Action); } +const include_cleaner::PragmaIncludes *ParsedAST::getPragmaIncludes() const { + if (!Preamble) + return nullptr; + return &Preamble->Pragmas; +} + std::optional<llvm::StringRef> ParsedAST::preambleVersion() const { if (!Preamble) return std::nullopt; diff --git a/clang-tools-extra/clangd/ParsedAST.h b/clang-tools-extra/clangd/ParsedAST.h index c0f4d65c030d0..27fa7e6ddd872 100644 --- a/clang-tools-extra/clangd/ParsedAST.h +++ b/clang-tools-extra/clangd/ParsedAST.h @@ -25,6 +25,7 @@ #include "Diagnostics.h" #include "Headers.h" #include "Preamble.h" +#include "clang-include-cleaner/Record.h" #include "index/CanonicalIncludes.h" #include "support/Path.h" #include "clang/Frontend/FrontendAction.h" @@ -106,6 +107,9 @@ class ParsedAST { /// Tokens recorded while parsing the main file. /// (!) does not have tokens from the preamble. const syntax::TokenBuffer &getTokens() const { return Tokens; } + /// Returns the PramaIncludes from the preamble. + /// Might be null if AST is built without a preamble. + const include_cleaner::PragmaIncludes *getPragmaIncludes() const; /// Returns the version of the ParseInputs this AST was built from. llvm::StringRef version() const { return Version; } diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp index 44a532511c16e..15eea9bb036bd 100644 --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -11,6 +11,7 @@ #include "Config.h" #include "Headers.h" #include "SourceCode.h" +#include "clang-include-cleaner/Record.h" #include "support/Logger.h" #include "support/ThreadsafeFS.h" #include "support/Trace.h" @@ -77,6 +78,9 @@ class CppFilePreambleCallbacks : public PreambleCallbacks { std::vector<PragmaMark> takeMarks() { return std::move(Marks); } + include_cleaner::PragmaIncludes takePragmaIncludes() { + return std::move(Pragmas); + } CanonicalIncludes takeCanonicalIncludes() { return std::move(CanonIncludes); } bool isMainFileIncludeGuarded() const { return IsMainFileIncludeGuarded; } @@ -118,6 +122,9 @@ class CppFilePreambleCallbacks : public PreambleCallbacks { LangOpts = &CI.getLangOpts(); SourceMgr = &CI.getSourceManager(); Includes.collect(CI); + if (Config::current().Diagnostics.UnusedIncludes == + Config::UnusedIncludesPolicy::Experiment) + Pragmas.record(CI); if (BeforeExecuteCallback) BeforeExecuteCallback(CI); } @@ -187,6 +194,7 @@ class CppFilePreambleCallbacks : public PreambleCallbacks { PreambleParsedCallback ParsedCallback; IncludeStructure Includes; CanonicalIncludes CanonIncludes; + include_cleaner::PragmaIncludes Pragmas; MainFileMacros Macros; std::vector<PragmaMark> Marks; bool IsMainFileIncludeGuarded = false; @@ -560,6 +568,7 @@ buildPreamble(PathRef FileName, CompilerInvocation CI, Result->CompileCommand = Inputs.CompileCommand; Result->Diags = std::move(Diags); Result->Includes = CapturedInfo.takeIncludes(); + Result->Pragmas = CapturedInfo.takePragmaIncludes(); Result->Macros = CapturedInfo.takeMacros(); Result->Marks = CapturedInfo.takeMarks(); Result->CanonIncludes = CapturedInfo.takeCanonicalIncludes(); diff --git a/clang-tools-extra/clangd/Preamble.h b/clang-tools-extra/clangd/Preamble.h index f108dd4fd69fc..bd5fefcf8f0bc 100644 --- a/clang-tools-extra/clangd/Preamble.h +++ b/clang-tools-extra/clangd/Preamble.h @@ -27,6 +27,7 @@ #include "Diagnostics.h" #include "FS.h" #include "Headers.h" +#include "clang-include-cleaner/Record.h" #include "index/CanonicalIncludes.h" #include "support/Path.h" #include "clang/Frontend/CompilerInvocation.h" @@ -57,6 +58,8 @@ struct PreambleData { // Processes like code completions and go-to-definitions will need #include // information, and their compile action skips preamble range. IncludeStructure Includes; + // Captures #include-mapping information in #included headers. + include_cleaner::PragmaIncludes Pragmas; // Macros defined in the preamble section of the main file. // Users care about headers vs main-file, not preamble vs non-preamble. // These should be treated as main-file entities e.g. for code completion. diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp index 0e4163122d255..d2dce9105cd81 100644 --- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp +++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp @@ -342,6 +342,8 @@ TEST(IncludeCleaner, StdlibUnused) { auto AST = TU.build(); EXPECT_THAT(computeUnusedIncludes(AST), ElementsAre(Pointee(writtenInclusion("<queue>")))); + EXPECT_THAT(computeUnusedIncludesExperimental(AST), + ElementsAre(Pointee(writtenInclusion("<queue>")))); } TEST(IncludeCleaner, GetUnusedHeaders) { @@ -377,6 +379,10 @@ TEST(IncludeCleaner, GetUnusedHeaders) { computeUnusedIncludes(AST), UnorderedElementsAre(Pointee(writtenInclusion("\"unused.h\"")), Pointee(writtenInclusion("\"dir/unused.h\"")))); + EXPECT_THAT( + computeUnusedIncludesExperimental(AST), + UnorderedElementsAre(Pointee(writtenInclusion("\"unused.h\"")), + Pointee(writtenInclusion("\"dir/unused.h\"")))); } TEST(IncludeCleaner, VirtualBuffers) { @@ -531,6 +537,9 @@ TEST(IncludeCleaner, IWYUPragmas) { // IWYU pragma: private, include "public.h" void foo() {} )cpp"); + Config Cfg; + Cfg.Diagnostics.UnusedIncludes = Config::Experiment; + WithContextValue Ctx(Config::Key, std::move(Cfg)); ParsedAST AST = TU.build(); auto ReferencedFiles = findReferencedFiles( @@ -545,6 +554,7 @@ TEST(IncludeCleaner, IWYUPragmas) { ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID())); EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty())); EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty()); + EXPECT_THAT(computeUnusedIncludesExperimental(AST), IsEmpty()); } TEST(IncludeCleaner, RecursiveInclusion) { @@ -573,6 +583,7 @@ TEST(IncludeCleaner, RecursiveInclusion) { EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty())); EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty()); + EXPECT_THAT(computeUnusedIncludesExperimental(AST), IsEmpty()); } TEST(IncludeCleaner, IWYUPragmaExport) { @@ -597,6 +608,7 @@ TEST(IncludeCleaner, IWYUPragmaExport) { // FIXME: This is not correct: foo.h is unused but is not diagnosed as such // because we ignore headers with IWYU export pragmas for now. EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty()); + EXPECT_THAT(computeUnusedIncludesExperimental(AST), IsEmpty()); } TEST(IncludeCleaner, NoDiagsForObjC) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
