[clang-tools-extra] [clangd] Track IWYU pragmas for non-preamble includes (PR #75612)
kadir çetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 18 12:53:47 PST 2023
https://github.com/kadircet updated https://github.com/llvm/llvm-project/pull/75612
>From 8801af07ad4e469f832570b5027e1522f41a6d06 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya <kadircet at google.com>
Date: Fri, 15 Dec 2023 15:14:05 +0100
Subject: [PATCH 1/2] [clangd] Track IWYU pragmas for non-preamble includes
---
clang-tools-extra/clangd/Hover.cpp | 4 ++--
clang-tools-extra/clangd/IncludeCleaner.cpp | 4 ++--
clang-tools-extra/clangd/ParsedAST.cpp | 21 +++++++++++--------
clang-tools-extra/clangd/ParsedAST.h | 9 ++++----
clang-tools-extra/clangd/XRefs.cpp | 2 +-
clang-tools-extra/clangd/index/FileIndex.cpp | 2 +-
.../clangd/unittests/FileIndexTests.cpp | 12 +++++------
.../clangd/unittests/IncludeCleanerTests.cpp | 2 ++
clang-tools-extra/clangd/unittests/TestTU.cpp | 4 ++--
.../include/clang-include-cleaner/Record.h | 2 +-
.../include-cleaner/lib/Record.cpp | 5 +++--
11 files changed, 36 insertions(+), 31 deletions(-)
diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp
index 82323fe16c82b6..06b949bc4a2b55 100644
--- a/clang-tools-extra/clangd/Hover.cpp
+++ b/clang-tools-extra/clangd/Hover.cpp
@@ -1194,7 +1194,7 @@ void maybeAddSymbolProviders(ParsedAST &AST, HoverInfo &HI,
const SourceManager &SM = AST.getSourceManager();
llvm::SmallVector<include_cleaner::Header> RankedProviders =
- include_cleaner::headersForSymbol(Sym, SM, AST.getPragmaIncludes().get());
+ include_cleaner::headersForSymbol(Sym, SM, &AST.getPragmaIncludes());
if (RankedProviders.empty())
return;
@@ -1254,7 +1254,7 @@ void maybeAddUsedSymbols(ParsedAST &AST, HoverInfo &HI, const Inclusion &Inc) {
llvm::DenseSet<include_cleaner::Symbol> UsedSymbols;
include_cleaner::walkUsed(
AST.getLocalTopLevelDecls(), collectMacroReferences(AST),
- AST.getPragmaIncludes().get(), AST.getPreprocessor(),
+ &AST.getPragmaIncludes(), AST.getPreprocessor(),
[&](const include_cleaner::SymbolReference &Ref,
llvm::ArrayRef<include_cleaner::Header> Providers) {
if (Ref.RT != include_cleaner::RefType::Explicit ||
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp
index 2f34c949349200..563a1f5d22f5b5 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -311,7 +311,7 @@ getUnused(ParsedAST &AST,
auto IncludeID = static_cast<IncludeStructure::HeaderID>(*MFI.HeaderID);
if (ReferencedFiles.contains(IncludeID))
continue;
- if (!mayConsiderUnused(MFI, AST, AST.getPragmaIncludes().get())) {
+ if (!mayConsiderUnused(MFI, AST, &AST.getPragmaIncludes())) {
dlog("{0} was not used, but is not eligible to be diagnosed as unused",
MFI.Written);
continue;
@@ -403,7 +403,7 @@ IncludeCleanerFindings computeIncludeCleanerFindings(ParsedAST &AST) {
.getBuiltinDir();
include_cleaner::walkUsed(
AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
- AST.getPragmaIncludes().get(), AST.getPreprocessor(),
+ &AST.getPragmaIncludes(), AST.getPreprocessor(),
[&](const include_cleaner::SymbolReference &Ref,
llvm::ArrayRef<include_cleaner::Header> Providers) {
bool Satisfied = false;
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index d91ce7283ecee4..14a91797f4d2ea 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -653,6 +653,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
}
IncludeStructure Includes;
+ include_cleaner::PragmaIncludes PI;
// If we are using a preamble, copy existing includes.
if (Preamble) {
Includes = Preamble->Includes;
@@ -660,11 +661,15 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
// Replay the preamble includes so that clang-tidy checks can see them.
ReplayPreamble::attach(Patch->preambleIncludes(), *Clang,
Patch->modifiedBounds());
+ PI = *Preamble->Pragmas;
}
// Important: collectIncludeStructure is registered *after* ReplayPreamble!
// Otherwise we would collect the replayed includes again...
// (We can't *just* use the replayed includes, they don't have Resolved path).
Includes.collect(*Clang);
+ // Same for pragma-includes, we're already inheriting preamble includes, so we
+ // should only receive callbacks for non-preamble mainfile includes.
+ PI.record(*Clang);
// Copy over the macros in the preamble region of the main file, and combine
// with non-preamble macros below.
MainFileMacros Macros;
@@ -735,7 +740,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
ParsedAST Result(Filename, 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(Diags), std::move(Includes), std::move(PI));
llvm::move(getIncludeCleanerDiags(Result, Inputs.Contents),
std::back_inserter(Result.Diags));
return std::move(Result);
@@ -828,23 +833,21 @@ ParsedAST::ParsedAST(PathRef TUPath, llvm::StringRef Version,
syntax::TokenBuffer Tokens, MainFileMacros Macros,
std::vector<PragmaMark> Marks,
std::vector<Decl *> LocalTopLevelDecls,
- std::vector<Diag> Diags, IncludeStructure Includes)
+ std::vector<Diag> Diags, IncludeStructure Includes,
+ include_cleaner::PragmaIncludes PI)
: TUPath(TUPath), Version(Version), Preamble(std::move(Preamble)),
Clang(std::move(Clang)), Action(std::move(Action)),
Tokens(std::move(Tokens)), Macros(std::move(Macros)),
Marks(std::move(Marks)), Diags(std::move(Diags)),
LocalTopLevelDecls(std::move(LocalTopLevelDecls)),
- Includes(std::move(Includes)) {
- Resolver = std::make_unique<HeuristicResolver>(getASTContext());
+ Includes(std::move(Includes)), PI(std::move(PI)),
+ Resolver(std::make_unique<HeuristicResolver>(getASTContext())) {
assert(this->Clang);
assert(this->Action);
}
-std::shared_ptr<const include_cleaner::PragmaIncludes>
-ParsedAST::getPragmaIncludes() const {
- if (!Preamble)
- return nullptr;
- return Preamble->Pragmas;
+const include_cleaner::PragmaIncludes &ParsedAST::getPragmaIncludes() const {
+ return PI;
}
std::optional<llvm::StringRef> ParsedAST::preambleVersion() const {
diff --git a/clang-tools-extra/clangd/ParsedAST.h b/clang-tools-extra/clangd/ParsedAST.h
index c68fdba6bd26cd..63e564bd68a78c 100644
--- a/clang-tools-extra/clangd/ParsedAST.h
+++ b/clang-tools-extra/clangd/ParsedAST.h
@@ -103,10 +103,8 @@ 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.
- std::shared_ptr<const include_cleaner::PragmaIncludes>
- getPragmaIncludes() const;
+ /// Returns the PramaIncludes for preamble + main file includes.
+ const include_cleaner::PragmaIncludes &getPragmaIncludes() const;
/// Returns the version of the ParseInputs this AST was built from.
llvm::StringRef version() const { return Version; }
@@ -129,7 +127,7 @@ class ParsedAST {
std::unique_ptr<FrontendAction> Action, syntax::TokenBuffer Tokens,
MainFileMacros Macros, std::vector<PragmaMark> Marks,
std::vector<Decl *> LocalTopLevelDecls, std::vector<Diag> Diags,
- IncludeStructure Includes);
+ IncludeStructure Includes, include_cleaner::PragmaIncludes PI);
Path TUPath;
std::string Version;
// In-memory preambles must outlive the AST, it is important that this member
@@ -159,6 +157,7 @@ class ParsedAST {
// top-level decls from the preamble.
std::vector<Decl *> LocalTopLevelDecls;
IncludeStructure Includes;
+ include_cleaner::PragmaIncludes PI;
std::unique_ptr<HeuristicResolver> Resolver;
};
diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp
index f55d2a56239563..5f41f788a69393 100644
--- a/clang-tools-extra/clangd/XRefs.cpp
+++ b/clang-tools-extra/clangd/XRefs.cpp
@@ -1339,7 +1339,7 @@ maybeFindIncludeReferences(ParsedAST &AST, Position Pos,
auto Converted = convertIncludes(AST);
include_cleaner::walkUsed(
AST.getLocalTopLevelDecls(), collectMacroReferences(AST),
- AST.getPragmaIncludes().get(), AST.getPreprocessor(),
+ &AST.getPragmaIncludes(), AST.getPreprocessor(),
[&](const include_cleaner::SymbolReference &Ref,
llvm::ArrayRef<include_cleaner::Header> Providers) {
if (Ref.RT != include_cleaner::RefType::Explicit ||
diff --git a/clang-tools-extra/clangd/index/FileIndex.cpp b/clang-tools-extra/clangd/index/FileIndex.cpp
index 5052c661cfc5ef..eb9562d2b6bf81 100644
--- a/clang-tools-extra/clangd/index/FileIndex.cpp
+++ b/clang-tools-extra/clangd/index/FileIndex.cpp
@@ -223,7 +223,7 @@ FileShardedIndex::getShard(llvm::StringRef Uri) const {
SlabTuple indexMainDecls(ParsedAST &AST) {
return indexSymbols(
AST.getASTContext(), AST.getPreprocessor(), AST.getLocalTopLevelDecls(),
- &AST.getMacros(), *AST.getPragmaIncludes(),
+ &AST.getMacros(), AST.getPragmaIncludes(),
/*IsIndexMainAST=*/true, AST.version(), /*CollectMainFileRefs=*/true);
}
diff --git a/clang-tools-extra/clangd/unittests/FileIndexTests.cpp b/clang-tools-extra/clangd/unittests/FileIndexTests.cpp
index cf30b388d234df..9f713564b2c01f 100644
--- a/clang-tools-extra/clangd/unittests/FileIndexTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FileIndexTests.cpp
@@ -176,7 +176,7 @@ void update(FileIndex &M, llvm::StringRef Basename, llvm::StringRef Code) {
auto AST = File.build();
M.updatePreamble(testPath(File.Filename), /*Version=*/"null",
AST.getASTContext(), AST.getPreprocessor(),
- *AST.getPragmaIncludes());
+ AST.getPragmaIncludes());
}
TEST(FileIndexTest, CustomizedURIScheme) {
@@ -254,7 +254,7 @@ TEST(FileIndexTest, IWYUPragmaExport) {
auto AST = File.build();
M.updatePreamble(testPath(File.Filename), /*Version=*/"null",
AST.getASTContext(), AST.getPreprocessor(),
- *AST.getPragmaIncludes());
+ AST.getPragmaIncludes());
auto Symbols = runFuzzyFind(M, "");
EXPECT_THAT(
@@ -446,7 +446,7 @@ TEST(FileIndexTest, Relations) {
FileIndex Index;
Index.updatePreamble(testPath(TU.Filename), /*Version=*/"null",
AST.getASTContext(), AST.getPreprocessor(),
- *AST.getPragmaIncludes());
+ AST.getPragmaIncludes());
SymbolID A = findSymbol(TU.headerSymbols(), "A").ID;
uint32_t Results = 0;
RelationsRequest Req;
@@ -567,7 +567,7 @@ TEST(FileIndexTest, StalePreambleSymbolsDeleted) {
auto AST = File.build();
M.updatePreamble(testPath(File.Filename), /*Version=*/"null",
AST.getASTContext(), AST.getPreprocessor(),
- *AST.getPragmaIncludes());
+ AST.getPragmaIncludes());
EXPECT_THAT(runFuzzyFind(M, ""), UnorderedElementsAre(qName("a")));
File.Filename = "f2.cpp";
@@ -575,7 +575,7 @@ TEST(FileIndexTest, StalePreambleSymbolsDeleted) {
AST = File.build();
M.updatePreamble(testPath(File.Filename), /*Version=*/"null",
AST.getASTContext(), AST.getPreprocessor(),
- *AST.getPragmaIncludes());
+ AST.getPragmaIncludes());
EXPECT_THAT(runFuzzyFind(M, ""), UnorderedElementsAre(qName("b")));
}
@@ -720,7 +720,7 @@ TEST(FileIndexTest, Profile) {
auto AST = TestTU::withHeaderCode("int a;").build();
FI.updateMain(FileName, AST);
FI.updatePreamble(FileName, "v1", AST.getASTContext(), AST.getPreprocessor(),
- *AST.getPragmaIncludes());
+ AST.getPragmaIncludes());
llvm::BumpPtrAllocator Alloc;
MemoryTree MT(&Alloc);
diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
index 5a6524dec2f09a..7ed4a9103e1f24 100644
--- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -316,8 +316,10 @@ TEST(IncludeCleaner, IWYUPragmas) {
#include "public.h"
void bar() { foo(); }
+ #include "keep_main_file.h" // IWYU pragma: keep
)cpp";
TU.AdditionalFiles["behind_keep.h"] = guard("");
+ TU.AdditionalFiles["keep_main_file.h"] = guard("");
TU.AdditionalFiles["exported.h"] = guard("");
TU.AdditionalFiles["public.h"] = guard("#include \"private.h\"");
TU.AdditionalFiles["private.h"] = guard(R"cpp(
diff --git a/clang-tools-extra/clangd/unittests/TestTU.cpp b/clang-tools-extra/clangd/unittests/TestTU.cpp
index e65ae825b41677..1f02c04125b1ea 100644
--- a/clang-tools-extra/clangd/unittests/TestTU.cpp
+++ b/clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -164,7 +164,7 @@ SymbolSlab TestTU::headerSymbols() const {
auto AST = build();
return std::get<0>(indexHeaderSymbols(
/*Version=*/"null", AST.getASTContext(), AST.getPreprocessor(),
- *AST.getPragmaIncludes()));
+ AST.getPragmaIncludes()));
}
RefSlab TestTU::headerRefs() const {
@@ -177,7 +177,7 @@ std::unique_ptr<SymbolIndex> TestTU::index() const {
auto Idx = std::make_unique<FileIndex>();
Idx->updatePreamble(testPath(Filename), /*Version=*/"null",
AST.getASTContext(), AST.getPreprocessor(),
- *AST.getPragmaIncludes());
+ AST.getPragmaIncludes());
Idx->updateMain(testPath(Filename), AST);
return std::move(Idx);
}
diff --git a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
index 2e0b462ce16df1..5e245736d4dbb4 100644
--- a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
+++ b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
@@ -113,7 +113,7 @@ class PragmaIncludes {
llvm::DenseSet<llvm::sys::fs::UniqueID> ShouldKeep;
/// Owns the strings.
- llvm::BumpPtrAllocator Arena;
+ std::shared_ptr<llvm::BumpPtrAllocator> Arena;
// FIXME: add support for clang use_instead pragma
};
diff --git a/clang-tools-extra/include-cleaner/lib/Record.cpp b/clang-tools-extra/include-cleaner/lib/Record.cpp
index bd726cff12a97d..a606b109e5c2e9 100644
--- a/clang-tools-extra/include-cleaner/lib/Record.cpp
+++ b/clang-tools-extra/include-cleaner/lib/Record.cpp
@@ -178,7 +178,8 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
: RecordPragma(CI.getPreprocessor(), Out) {}
RecordPragma(const Preprocessor &P, PragmaIncludes *Out)
: SM(P.getSourceManager()), HeaderInfo(P.getHeaderSearchInfo()), Out(Out),
- UniqueStrings(Arena) {}
+ Arena(std::make_shared<llvm::BumpPtrAllocator>()),
+ UniqueStrings(*Arena) {}
void FileChanged(SourceLocation Loc, FileChangeReason Reason,
SrcMgr::CharacteristicKind FileType,
@@ -336,7 +337,7 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
const SourceManager &SM;
const HeaderSearch &HeaderInfo;
PragmaIncludes *Out;
- llvm::BumpPtrAllocator Arena;
+ std::shared_ptr<llvm::BumpPtrAllocator> Arena;
/// Intern table for strings. Contents are on the arena.
llvm::StringSaver UniqueStrings;
>From 7d078ee1ec8cc712719a54fc547def49f31ee641 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya <kadircet at google.com>
Date: Mon, 18 Dec 2023 21:43:47 +0100
Subject: [PATCH 2/2] Make sure PI keeps old strings alive
Indicate that once an allocator is put into PI it is immutable, and also
make sure all the previous allocators out-lives the PI they were
recorded on.
---
.../include/clang-include-cleaner/Record.h | 3 ++-
.../include-cleaner/lib/Record.cpp | 2 +-
.../include-cleaner/unittests/RecordTest.cpp | 25 +++++++++++++++++++
3 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
index 5e245736d4dbb4..2dcb5ea2555c5e 100644
--- a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
+++ b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
@@ -113,7 +113,8 @@ class PragmaIncludes {
llvm::DenseSet<llvm::sys::fs::UniqueID> ShouldKeep;
/// Owns the strings.
- std::shared_ptr<llvm::BumpPtrAllocator> Arena;
+ /// Each record() pushes a new one, while keeping all the old strings alive.
+ std::vector<std::shared_ptr<const llvm::BumpPtrAllocator>> Arena;
// FIXME: add support for clang use_instead pragma
};
diff --git a/clang-tools-extra/include-cleaner/lib/Record.cpp b/clang-tools-extra/include-cleaner/lib/Record.cpp
index a606b109e5c2e9..c93c56adf650d9 100644
--- a/clang-tools-extra/include-cleaner/lib/Record.cpp
+++ b/clang-tools-extra/include-cleaner/lib/Record.cpp
@@ -205,7 +205,7 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
std::unique(It.getSecond().begin(), It.getSecond().end()),
It.getSecond().end());
}
- Out->Arena = std::move(Arena);
+ Out->Arena.emplace_back(std::move(Arena));
}
void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
diff --git a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
index 0f2ded5f183453..d1f7590b222681 100644
--- a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
@@ -588,5 +588,30 @@ TEST_F(PragmaIncludeTest, OutlivesFMAndSM) {
EXPECT_THAT(PI.getExporters(Private2FE.get(), FM),
testing::ElementsAre(llvm::cantFail(FM.getFileRef("public.h"))));
}
+
+TEST_F(PragmaIncludeTest, CanRecordManyTimes) {
+ Inputs.Code = R"cpp(
+ #include "public.h"
+ )cpp";
+ Inputs.ExtraFiles["public.h"] = R"cpp(
+ #include "private.h"
+ )cpp";
+ Inputs.ExtraFiles["private.h"] = R"cpp(
+ // IWYU pragma: private, include "public.h"
+ )cpp";
+
+ TestAST Processed = build();
+ auto &FM = Processed.fileManager();
+ auto PrivateFE = FM.getFile("private.h");
+ llvm::StringRef Public = PI.getPublic(PrivateFE.get());
+ EXPECT_EQ(Public, "\"public.h\"");
+
+ // This build populates same PI during build, but this time we don't have
+ // any IWYU pragmas. Make sure strings from previous recordings are still
+ // alive.
+ Inputs.Code = "";
+ build();
+ EXPECT_EQ(Public, "\"public.h\"");
+}
} // namespace
} // namespace clang::include_cleaner
More information about the cfe-commits
mailing list