[clang-tools-extra] 939dce1 - [clangd] Implement unused include warnings with include-cleaner library.
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 19 05:31:54 PST 2023
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) {
More information about the cfe-commits
mailing list