Skip to content

Commit 7837110

Browse files
kadircetkadircet
authored
[clangd] Track IWYU pragmas for non-preamble includes (#75612)
This makes PragmaIncldues copyable, and copies it from preamble when building a new AST. Fixes clangd/clangd#1843 Fixes clangd/clangd#1571
1 parent b3d2642 commit 7837110

File tree

12 files changed

+63
-32
lines changed

12 files changed

+63
-32
lines changed

‎clang-tools-extra/clangd/Hover.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1194,7 +1194,7 @@ void maybeAddSymbolProviders(ParsedAST &AST, HoverInfo &HI,
11941194

11951195
const SourceManager &SM = AST.getSourceManager();
11961196
llvm::SmallVector<include_cleaner::Header> RankedProviders =
1197-
include_cleaner::headersForSymbol(Sym, SM, AST.getPragmaIncludes().get());
1197+
include_cleaner::headersForSymbol(Sym, SM, &AST.getPragmaIncludes());
11981198
if (RankedProviders.empty())
11991199
return;
12001200

@@ -1254,7 +1254,7 @@ void maybeAddUsedSymbols(ParsedAST &AST, HoverInfo &HI, const Inclusion &Inc) {
12541254
llvm::DenseSet<include_cleaner::Symbol> UsedSymbols;
12551255
include_cleaner::walkUsed(
12561256
AST.getLocalTopLevelDecls(), collectMacroReferences(AST),
1257-
AST.getPragmaIncludes().get(), AST.getPreprocessor(),
1257+
&AST.getPragmaIncludes(), AST.getPreprocessor(),
12581258
[&](const include_cleaner::SymbolReference &Ref,
12591259
llvm::ArrayRef<include_cleaner::Header> Providers) {
12601260
if (Ref.RT != include_cleaner::RefType::Explicit ||

‎clang-tools-extra/clangd/IncludeCleaner.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -311,7 +311,7 @@ getUnused(ParsedAST &AST,
311311
auto IncludeID = static_cast<IncludeStructure::HeaderID>(*MFI.HeaderID);
312312
if (ReferencedFiles.contains(IncludeID))
313313
continue;
314-
if (!mayConsiderUnused(MFI, AST, AST.getPragmaIncludes().get())) {
314+
if (!mayConsiderUnused(MFI, AST, &AST.getPragmaIncludes())) {
315315
dlog("{0} was not used, but is not eligible to be diagnosed as unused",
316316
MFI.Written);
317317
continue;
@@ -403,7 +403,7 @@ IncludeCleanerFindings computeIncludeCleanerFindings(ParsedAST &AST) {
403403
.getBuiltinDir();
404404
include_cleaner::walkUsed(
405405
AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
406-
AST.getPragmaIncludes().get(), AST.getPreprocessor(),
406+
&AST.getPragmaIncludes(), AST.getPreprocessor(),
407407
[&](const include_cleaner::SymbolReference &Ref,
408408
llvm::ArrayRef<include_cleaner::Header> Providers) {
409409
bool Satisfied = false;

‎clang-tools-extra/clangd/ParsedAST.cpp

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -653,18 +653,23 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
653653
}
654654

655655
IncludeStructure Includes;
656+
include_cleaner::PragmaIncludes PI;
656657
// If we are using a preamble, copy existing includes.
657658
if (Preamble) {
658659
Includes = Preamble->Includes;
659660
Includes.MainFileIncludes = Patch->preambleIncludes();
660661
// Replay the preamble includes so that clang-tidy checks can see them.
661662
ReplayPreamble::attach(Patch->preambleIncludes(), *Clang,
662663
Patch->modifiedBounds());
664+
PI = *Preamble->Pragmas;
663665
}
664666
// Important: collectIncludeStructure is registered *after* ReplayPreamble!
665667
// Otherwise we would collect the replayed includes again...
666668
// (We can't *just* use the replayed includes, they don't have Resolved path).
667669
Includes.collect(*Clang);
670+
// Same for pragma-includes, we're already inheriting preamble includes, so we
671+
// should only receive callbacks for non-preamble mainfile includes.
672+
PI.record(*Clang);
668673
// Copy over the macros in the preamble region of the main file, and combine
669674
// with non-preamble macros below.
670675
MainFileMacros Macros;
@@ -735,7 +740,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
735740
ParsedAST Result(Filename, Inputs.Version, std::move(Preamble),
736741
std::move(Clang), std::move(Action), std::move(Tokens),
737742
std::move(Macros), std::move(Marks), std::move(ParsedDecls),
738-
std::move(Diags), std::move(Includes));
743+
std::move(Diags), std::move(Includes), std::move(PI));
739744
llvm::move(getIncludeCleanerDiags(Result, Inputs.Contents),
740745
std::back_inserter(Result.Diags));
741746
return std::move(Result);
@@ -828,23 +833,21 @@ ParsedAST::ParsedAST(PathRef TUPath, llvm::StringRef Version,
828833
syntax::TokenBuffer Tokens, MainFileMacros Macros,
829834
std::vector<PragmaMark> Marks,
830835
std::vector<Decl *> LocalTopLevelDecls,
831-
std::vector<Diag> Diags, IncludeStructure Includes)
836+
std::vector<Diag> Diags, IncludeStructure Includes,
837+
include_cleaner::PragmaIncludes PI)
832838
: TUPath(TUPath), Version(Version), Preamble(std::move(Preamble)),
833839
Clang(std::move(Clang)), Action(std::move(Action)),
834840
Tokens(std::move(Tokens)), Macros(std::move(Macros)),
835841
Marks(std::move(Marks)), Diags(std::move(Diags)),
836842
LocalTopLevelDecls(std::move(LocalTopLevelDecls)),
837-
Includes(std::move(Includes)) {
838-
Resolver = std::make_unique<HeuristicResolver>(getASTContext());
843+
Includes(std::move(Includes)), PI(std::move(PI)),
844+
Resolver(std::make_unique<HeuristicResolver>(getASTContext())) {
839845
assert(this->Clang);
840846
assert(this->Action);
841847
}
842848

843-
std::shared_ptr<const include_cleaner::PragmaIncludes>
844-
ParsedAST::getPragmaIncludes() const {
845-
if (!Preamble)
846-
return nullptr;
847-
return Preamble->Pragmas;
849+
const include_cleaner::PragmaIncludes &ParsedAST::getPragmaIncludes() const {
850+
return PI;
848851
}
849852

850853
std::optional<llvm::StringRef> ParsedAST::preambleVersion() const {

‎clang-tools-extra/clangd/ParsedAST.h

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -103,10 +103,8 @@ class ParsedAST {
103103
/// Tokens recorded while parsing the main file.
104104
/// (!) does not have tokens from the preamble.
105105
const syntax::TokenBuffer &getTokens() const { return Tokens; }
106-
/// Returns the PramaIncludes from the preamble.
107-
/// Might be null if AST is built without a preamble.
108-
std::shared_ptr<const include_cleaner::PragmaIncludes>
109-
getPragmaIncludes() const;
106+
/// Returns the PramaIncludes for preamble + main file includes.
107+
const include_cleaner::PragmaIncludes &getPragmaIncludes() const;
110108

111109
/// Returns the version of the ParseInputs this AST was built from.
112110
llvm::StringRef version() const { return Version; }
@@ -129,7 +127,7 @@ class ParsedAST {
129127
std::unique_ptr<FrontendAction> Action, syntax::TokenBuffer Tokens,
130128
MainFileMacros Macros, std::vector<PragmaMark> Marks,
131129
std::vector<Decl *> LocalTopLevelDecls, std::vector<Diag> Diags,
132-
IncludeStructure Includes);
130+
IncludeStructure Includes, include_cleaner::PragmaIncludes PI);
133131
Path TUPath;
134132
std::string Version;
135133
// In-memory preambles must outlive the AST, it is important that this member
@@ -159,6 +157,7 @@ class ParsedAST {
159157
// top-level decls from the preamble.
160158
std::vector<Decl *> LocalTopLevelDecls;
161159
IncludeStructure Includes;
160+
include_cleaner::PragmaIncludes PI;
162161
std::unique_ptr<HeuristicResolver> Resolver;
163162
};
164163

‎clang-tools-extra/clangd/XRefs.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1339,7 +1339,7 @@ maybeFindIncludeReferences(ParsedAST &AST, Position Pos,
13391339
auto Converted = convertIncludes(AST);
13401340
include_cleaner::walkUsed(
13411341
AST.getLocalTopLevelDecls(), collectMacroReferences(AST),
1342-
AST.getPragmaIncludes().get(), AST.getPreprocessor(),
1342+
&AST.getPragmaIncludes(), AST.getPreprocessor(),
13431343
[&](const include_cleaner::SymbolReference &Ref,
13441344
llvm::ArrayRef<include_cleaner::Header> Providers) {
13451345
if (Ref.RT != include_cleaner::RefType::Explicit ||

‎clang-tools-extra/clangd/index/FileIndex.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ FileShardedIndex::getShard(llvm::StringRef Uri) const {
223223
SlabTuple indexMainDecls(ParsedAST &AST) {
224224
return indexSymbols(
225225
AST.getASTContext(), AST.getPreprocessor(), AST.getLocalTopLevelDecls(),
226-
&AST.getMacros(), *AST.getPragmaIncludes(),
226+
&AST.getMacros(), AST.getPragmaIncludes(),
227227
/*IsIndexMainAST=*/true, AST.version(), /*CollectMainFileRefs=*/true);
228228
}
229229

‎clang-tools-extra/clangd/unittests/FileIndexTests.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ void update(FileIndex &M, llvm::StringRef Basename, llvm::StringRef Code) {
176176
auto AST = File.build();
177177
M.updatePreamble(testPath(File.Filename), /*Version=*/"null",
178178
AST.getASTContext(), AST.getPreprocessor(),
179-
*AST.getPragmaIncludes());
179+
AST.getPragmaIncludes());
180180
}
181181

182182
TEST(FileIndexTest, CustomizedURIScheme) {
@@ -254,7 +254,7 @@ TEST(FileIndexTest, IWYUPragmaExport) {
254254
auto AST = File.build();
255255
M.updatePreamble(testPath(File.Filename), /*Version=*/"null",
256256
AST.getASTContext(), AST.getPreprocessor(),
257-
*AST.getPragmaIncludes());
257+
AST.getPragmaIncludes());
258258

259259
auto Symbols = runFuzzyFind(M, "");
260260
EXPECT_THAT(
@@ -446,7 +446,7 @@ TEST(FileIndexTest, Relations) {
446446
FileIndex Index;
447447
Index.updatePreamble(testPath(TU.Filename), /*Version=*/"null",
448448
AST.getASTContext(), AST.getPreprocessor(),
449-
*AST.getPragmaIncludes());
449+
AST.getPragmaIncludes());
450450
SymbolID A = findSymbol(TU.headerSymbols(), "A").ID;
451451
uint32_t Results = 0;
452452
RelationsRequest Req;
@@ -567,15 +567,15 @@ TEST(FileIndexTest, StalePreambleSymbolsDeleted) {
567567
auto AST = File.build();
568568
M.updatePreamble(testPath(File.Filename), /*Version=*/"null",
569569
AST.getASTContext(), AST.getPreprocessor(),
570-
*AST.getPragmaIncludes());
570+
AST.getPragmaIncludes());
571571
EXPECT_THAT(runFuzzyFind(M, ""), UnorderedElementsAre(qName("a")));
572572

573573
File.Filename = "f2.cpp";
574574
File.HeaderCode = "int b;";
575575
AST = File.build();
576576
M.updatePreamble(testPath(File.Filename), /*Version=*/"null",
577577
AST.getASTContext(), AST.getPreprocessor(),
578-
*AST.getPragmaIncludes());
578+
AST.getPragmaIncludes());
579579
EXPECT_THAT(runFuzzyFind(M, ""), UnorderedElementsAre(qName("b")));
580580
}
581581

@@ -720,7 +720,7 @@ TEST(FileIndexTest, Profile) {
720720
auto AST = TestTU::withHeaderCode("int a;").build();
721721
FI.updateMain(FileName, AST);
722722
FI.updatePreamble(FileName, "v1", AST.getASTContext(), AST.getPreprocessor(),
723-
*AST.getPragmaIncludes());
723+
AST.getPragmaIncludes());
724724

725725
llvm::BumpPtrAllocator Alloc;
726726
MemoryTree MT(&Alloc);

‎clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -316,8 +316,10 @@ TEST(IncludeCleaner, IWYUPragmas) {
316316
#include "public.h"
317317
318318
void bar() { foo(); }
319+
#include "keep_main_file.h" // IWYU pragma: keep
319320
)cpp";
320321
TU.AdditionalFiles["behind_keep.h"] = guard("");
322+
TU.AdditionalFiles["keep_main_file.h"] = guard("");
321323
TU.AdditionalFiles["exported.h"] = guard("");
322324
TU.AdditionalFiles["public.h"] = guard("#include \"private.h\"");
323325
TU.AdditionalFiles["private.h"] = guard(R"cpp(

‎clang-tools-extra/clangd/unittests/TestTU.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ SymbolSlab TestTU::headerSymbols() const {
164164
auto AST = build();
165165
return std::get<0>(indexHeaderSymbols(
166166
/*Version=*/"null", AST.getASTContext(), AST.getPreprocessor(),
167-
*AST.getPragmaIncludes()));
167+
AST.getPragmaIncludes()));
168168
}
169169

170170
RefSlab TestTU::headerRefs() const {
@@ -177,7 +177,7 @@ std::unique_ptr<SymbolIndex> TestTU::index() const {
177177
auto Idx = std::make_unique<FileIndex>();
178178
Idx->updatePreamble(testPath(Filename), /*Version=*/"null",
179179
AST.getASTContext(), AST.getPreprocessor(),
180-
*AST.getPragmaIncludes());
180+
AST.getPragmaIncludes());
181181
Idx->updateMain(testPath(Filename), AST);
182182
return std::move(Idx);
183183
}

‎clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,8 @@ class PragmaIncludes {
113113
llvm::DenseSet<llvm::sys::fs::UniqueID> ShouldKeep;
114114

115115
/// Owns the strings.
116-
llvm::BumpPtrAllocator Arena;
116+
/// Each record() pushes a new one, while keeping all the old strings alive.
117+
std::vector<std::shared_ptr<const llvm::BumpPtrAllocator>> Arena;
117118

118119
// FIXME: add support for clang use_instead pragma
119120
};

‎clang-tools-extra/include-cleaner/lib/Record.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,8 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
178178
: RecordPragma(CI.getPreprocessor(), Out) {}
179179
RecordPragma(const Preprocessor &P, PragmaIncludes *Out)
180180
: SM(P.getSourceManager()), HeaderInfo(P.getHeaderSearchInfo()), Out(Out),
181-
UniqueStrings(Arena) {}
181+
Arena(std::make_shared<llvm::BumpPtrAllocator>()),
182+
UniqueStrings(*Arena) {}
182183

183184
void FileChanged(SourceLocation Loc, FileChangeReason Reason,
184185
SrcMgr::CharacteristicKind FileType,
@@ -204,7 +205,7 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
204205
std::unique(It.getSecond().begin(), It.getSecond().end()),
205206
It.getSecond().end());
206207
}
207-
Out->Arena = std::move(Arena);
208+
Out->Arena.emplace_back(std::move(Arena));
208209
}
209210

210211
void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
@@ -336,7 +337,7 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
336337
const SourceManager &SM;
337338
const HeaderSearch &HeaderInfo;
338339
PragmaIncludes *Out;
339-
llvm::BumpPtrAllocator Arena;
340+
std::shared_ptr<llvm::BumpPtrAllocator> Arena;
340341
/// Intern table for strings. Contents are on the arena.
341342
llvm::StringSaver UniqueStrings;
342343

‎clang-tools-extra/include-cleaner/unittests/RecordTest.cpp

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -588,5 +588,30 @@ TEST_F(PragmaIncludeTest, OutlivesFMAndSM) {
588588
EXPECT_THAT(PI.getExporters(Private2FE.get(), FM),
589589
testing::ElementsAre(llvm::cantFail(FM.getFileRef("public.h"))));
590590
}
591+
592+
TEST_F(PragmaIncludeTest, CanRecordManyTimes) {
593+
Inputs.Code = R"cpp(
594+
#include "public.h"
595+
)cpp";
596+
Inputs.ExtraFiles["public.h"] = R"cpp(
597+
#include "private.h"
598+
)cpp";
599+
Inputs.ExtraFiles["private.h"] = R"cpp(
600+
// IWYU pragma: private, include "public.h"
601+
)cpp";
602+
603+
TestAST Processed = build();
604+
auto &FM = Processed.fileManager();
605+
auto PrivateFE = FM.getFile("private.h");
606+
llvm::StringRef Public = PI.getPublic(PrivateFE.get());
607+
EXPECT_EQ(Public, "\"public.h\"");
608+
609+
// This build populates same PI during build, but this time we don't have
610+
// any IWYU pragmas. Make sure strings from previous recordings are still
611+
// alive.
612+
Inputs.Code = "";
613+
build();
614+
EXPECT_EQ(Public, "\"public.h\"");
615+
}
591616
} // namespace
592617
} // namespace clang::include_cleaner

0 commit comments

Comments
 (0)