[include-cleaner] Respect langopts when analyzing macro names (#123634)

Fixes https://github.com/llvm/llvm-project/issues/113926.
Fixes https://github.com/llvm/llvm-project/issues/63976.
This commit is contained in:
kadir çetinkaya
2025-01-21 14:48:08 +01:00
committed by GitHub
parent 67a412f072
commit ec6c3448d3
11 changed files with 77 additions and 40 deletions

View File

@@ -1193,12 +1193,13 @@ void maybeAddSymbolProviders(ParsedAST &AST, HoverInfo &HI,
include_cleaner::Symbol Sym) { include_cleaner::Symbol Sym) {
trace::Span Tracer("Hover::maybeAddSymbolProviders"); trace::Span Tracer("Hover::maybeAddSymbolProviders");
const SourceManager &SM = AST.getSourceManager();
llvm::SmallVector<include_cleaner::Header> RankedProviders = llvm::SmallVector<include_cleaner::Header> RankedProviders =
include_cleaner::headersForSymbol(Sym, SM, &AST.getPragmaIncludes()); include_cleaner::headersForSymbol(Sym, AST.getPreprocessor(),
&AST.getPragmaIncludes());
if (RankedProviders.empty()) if (RankedProviders.empty())
return; return;
const SourceManager &SM = AST.getSourceManager();
std::string Result; std::string Result;
include_cleaner::Includes ConvertedIncludes = convertIncludes(AST); include_cleaner::Includes ConvertedIncludes = convertIncludes(AST);
for (const auto &P : RankedProviders) { for (const auto &P : RankedProviders) {

View File

@@ -888,7 +888,7 @@ void SymbolCollector::setIncludeLocation(const Symbol &S, SourceLocation DefLoc,
// might run while parsing, rather than at the end of a translation unit. // might run while parsing, rather than at the end of a translation unit.
// Hence we see more and more redecls over time. // Hence we see more and more redecls over time.
SymbolProviders[S.ID] = SymbolProviders[S.ID] =
include_cleaner::headersForSymbol(Sym, SM, Opts.PragmaIncludes); include_cleaner::headersForSymbol(Sym, *PP, Opts.PragmaIncludes);
} }
llvm::StringRef getStdHeader(const Symbol *S, const LangOptions &LangOpts) { llvm::StringRef getStdHeader(const Symbol *S, const LangOptions &LangOpts) {

View File

@@ -90,7 +90,7 @@ std::string fixIncludes(const AnalysisResults &Results,
/// Returned headers are sorted by relevance, first element is the most /// Returned headers are sorted by relevance, first element is the most
/// likely provider for the symbol. /// likely provider for the symbol.
llvm::SmallVector<Header> headersForSymbol(const Symbol &S, llvm::SmallVector<Header> headersForSymbol(const Symbol &S,
const SourceManager &SM, const Preprocessor &PP,
const PragmaIncludes *PI); const PragmaIncludes *PI);
} // namespace include_cleaner } // namespace include_cleaner
} // namespace clang } // namespace clang

View File

@@ -64,7 +64,7 @@ void walkUsed(llvm::ArrayRef<Decl *> ASTRoots,
// FIXME: Most of the work done here is repetitive. It might be useful to // FIXME: Most of the work done here is repetitive. It might be useful to
// have a cache/batching. // have a cache/batching.
SymbolReference SymRef{ND, Loc, RT}; SymbolReference SymRef{ND, Loc, RT};
return CB(SymRef, headersForSymbol(ND, SM, PI)); return CB(SymRef, headersForSymbol(ND, PP, PI));
}); });
} }
for (const SymbolReference &MacroRef : MacroRefs) { for (const SymbolReference &MacroRef : MacroRefs) {
@@ -72,7 +72,7 @@ void walkUsed(llvm::ArrayRef<Decl *> ASTRoots,
if (!SM.isWrittenInMainFile(SM.getSpellingLoc(MacroRef.RefLocation)) || if (!SM.isWrittenInMainFile(SM.getSpellingLoc(MacroRef.RefLocation)) ||
shouldIgnoreMacroReference(PP, MacroRef.Target.macro())) shouldIgnoreMacroReference(PP, MacroRef.Target.macro()))
continue; continue;
CB(MacroRef, headersForSymbol(MacroRef.Target, SM, PI)); CB(MacroRef, headersForSymbol(MacroRef.Target, PP, PI));
} }
} }

View File

@@ -25,6 +25,8 @@
#include "clang-include-cleaner/Analysis.h" #include "clang-include-cleaner/Analysis.h"
#include "clang-include-cleaner/Record.h" #include "clang-include-cleaner/Record.h"
#include "clang-include-cleaner/Types.h" #include "clang-include-cleaner/Types.h"
#include "clang/Basic/LangOptions.h"
#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/STLFunctionalExtras.h" #include "llvm/ADT/STLFunctionalExtras.h"
#include <vector> #include <vector>
@@ -57,13 +59,14 @@ llvm::SmallVector<Hinted<Header>> findHeaders(const SymbolLocation &Loc,
const PragmaIncludes *PI); const PragmaIncludes *PI);
/// A set of locations that provides the declaration. /// A set of locations that provides the declaration.
std::vector<Hinted<SymbolLocation>> locateSymbol(const Symbol &S); std::vector<Hinted<SymbolLocation>> locateSymbol(const Symbol &S,
const LangOptions &LO);
/// Write an HTML summary of the analysis to the given stream. /// Write an HTML summary of the analysis to the given stream.
void writeHTMLReport(FileID File, const Includes &, void writeHTMLReport(FileID File, const Includes &,
llvm::ArrayRef<Decl *> Roots, llvm::ArrayRef<Decl *> Roots,
llvm::ArrayRef<SymbolReference> MacroRefs, ASTContext &Ctx, llvm::ArrayRef<SymbolReference> MacroRefs, ASTContext &Ctx,
const HeaderSearch &HS, PragmaIncludes *PI, const Preprocessor &PP, PragmaIncludes *PI,
llvm::raw_ostream &OS); llvm::raw_ostream &OS);
} // namespace include_cleaner } // namespace include_cleaner

View File

@@ -18,6 +18,7 @@
#include "clang/Basic/FileEntry.h" #include "clang/Basic/FileEntry.h"
#include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h" #include "clang/Basic/SourceManager.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Tooling/Inclusions/StandardLibrary.h" #include "clang/Tooling/Inclusions/StandardLibrary.h"
#include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/STLExtras.h"
@@ -239,8 +240,9 @@ llvm::SmallVector<Hinted<Header>> findHeaders(const SymbolLocation &Loc,
} }
llvm::SmallVector<Header> headersForSymbol(const Symbol &S, llvm::SmallVector<Header> headersForSymbol(const Symbol &S,
const SourceManager &SM, const Preprocessor &PP,
const PragmaIncludes *PI) { const PragmaIncludes *PI) {
const auto &SM = PP.getSourceManager();
// Get headers for all the locations providing Symbol. Same header can be // Get headers for all the locations providing Symbol. Same header can be
// reached through different traversals, deduplicate those into a single // reached through different traversals, deduplicate those into a single
// Header by merging their hints. // Header by merging their hints.
@@ -248,7 +250,7 @@ llvm::SmallVector<Header> headersForSymbol(const Symbol &S,
if (auto SpecialHeaders = headersForSpecialSymbol(S, SM, PI)) { if (auto SpecialHeaders = headersForSpecialSymbol(S, SM, PI)) {
Headers = std::move(*SpecialHeaders); Headers = std::move(*SpecialHeaders);
} else { } else {
for (auto &Loc : locateSymbol(S)) for (auto &Loc : locateSymbol(S, PP.getLangOpts()))
Headers.append(applyHints(findHeaders(Loc, SM, PI), Loc.Hint)); Headers.append(applyHints(findHeaders(Loc, SM, PI), Loc.Hint));
} }
// If two Headers probably refer to the same file (e.g. Verbatim(foo.h) and // If two Headers probably refer to the same file (e.g. Verbatim(foo.h) and

View File

@@ -21,6 +21,7 @@
#include "clang/Basic/SourceManager.h" #include "clang/Basic/SourceManager.h"
#include "clang/Lex/HeaderSearch.h" #include "clang/Lex/HeaderSearch.h"
#include "clang/Lex/Lexer.h" #include "clang/Lex/Lexer.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Tooling/Inclusions/StandardLibrary.h" #include "clang/Tooling/Inclusions/StandardLibrary.h"
#include "llvm/Support/ScopedPrinter.h" #include "llvm/Support/ScopedPrinter.h"
#include "llvm/Support/raw_ostream.h" #include "llvm/Support/raw_ostream.h"
@@ -135,7 +136,7 @@ class Reporter {
llvm::raw_ostream &OS; llvm::raw_ostream &OS;
const ASTContext &Ctx; const ASTContext &Ctx;
const SourceManager &SM; const SourceManager &SM;
const HeaderSearch &HS; const Preprocessor &PP;
const include_cleaner::Includes &Includes; const include_cleaner::Includes &Includes;
const PragmaIncludes *PI; const PragmaIncludes *PI;
FileID MainFile; FileID MainFile;
@@ -170,9 +171,9 @@ class Reporter {
void fillTarget(Ref &R) { void fillTarget(Ref &R) {
// Duplicates logic from walkUsed(), which doesn't expose SymbolLocations. // Duplicates logic from walkUsed(), which doesn't expose SymbolLocations.
for (auto &Loc : locateSymbol(R.Sym)) for (auto &Loc : locateSymbol(R.Sym, Ctx.getLangOpts()))
R.Locations.push_back(Loc); R.Locations.push_back(Loc);
R.Headers = headersForSymbol(R.Sym, SM, PI); R.Headers = headersForSymbol(R.Sym, PP, PI);
for (const auto &H : R.Headers) { for (const auto &H : R.Headers) {
R.Includes.append(Includes.match(H)); R.Includes.append(Includes.match(H));
@@ -189,14 +190,15 @@ class Reporter {
R.Includes.end()); R.Includes.end());
if (!R.Headers.empty()) if (!R.Headers.empty())
R.Insert = spellHeader({R.Headers.front(), HS, MainFE}); R.Insert =
spellHeader({R.Headers.front(), PP.getHeaderSearchInfo(), MainFE});
} }
public: public:
Reporter(llvm::raw_ostream &OS, ASTContext &Ctx, const HeaderSearch &HS, Reporter(llvm::raw_ostream &OS, ASTContext &Ctx, const Preprocessor &PP,
const include_cleaner::Includes &Includes, const PragmaIncludes *PI, const include_cleaner::Includes &Includes, const PragmaIncludes *PI,
FileID MainFile) FileID MainFile)
: OS(OS), Ctx(Ctx), SM(Ctx.getSourceManager()), HS(HS), : OS(OS), Ctx(Ctx), SM(Ctx.getSourceManager()), PP(PP),
Includes(Includes), PI(PI), MainFile(MainFile), Includes(Includes), PI(PI), MainFile(MainFile),
MainFE(SM.getFileEntryForID(MainFile)) {} MainFE(SM.getFileEntryForID(MainFile)) {}
@@ -498,9 +500,9 @@ private:
void writeHTMLReport(FileID File, const include_cleaner::Includes &Includes, void writeHTMLReport(FileID File, const include_cleaner::Includes &Includes,
llvm::ArrayRef<Decl *> Roots, llvm::ArrayRef<Decl *> Roots,
llvm::ArrayRef<SymbolReference> MacroRefs, ASTContext &Ctx, llvm::ArrayRef<SymbolReference> MacroRefs, ASTContext &Ctx,
const HeaderSearch &HS, PragmaIncludes *PI, const Preprocessor &PP, PragmaIncludes *PI,
llvm::raw_ostream &OS) { llvm::raw_ostream &OS) {
Reporter R(OS, Ctx, HS, Includes, PI, File); Reporter R(OS, Ctx, PP, Includes, PI, File);
const auto& SM = Ctx.getSourceManager(); const auto& SM = Ctx.getSourceManager();
for (Decl *Root : Roots) for (Decl *Root : Roots)
walkAST(*Root, [&](SourceLocation Loc, const NamedDecl &D, RefType T) { walkAST(*Root, [&](SourceLocation Loc, const NamedDecl &D, RefType T) {

View File

@@ -54,20 +54,24 @@ std::vector<Hinted<SymbolLocation>> locateDecl(const Decl &D) {
return Result; return Result;
} }
std::vector<Hinted<SymbolLocation>> locateMacro(const Macro &M) { std::vector<Hinted<SymbolLocation>> locateMacro(const Macro &M,
const tooling::stdlib::Lang L) {
// FIXME: Should we also provide physical locations? // FIXME: Should we also provide physical locations?
if (auto SS = tooling::stdlib::Symbol::named("", M.Name->getName())) if (auto SS = tooling::stdlib::Symbol::named("", M.Name->getName(), L))
return {{*SS, Hints::CompleteSymbol}}; return {{*SS, Hints::CompleteSymbol}};
return {{M.Definition, Hints::CompleteSymbol}}; return {{M.Definition, Hints::CompleteSymbol}};
} }
} // namespace } // namespace
std::vector<Hinted<SymbolLocation>> locateSymbol(const Symbol &S) { std::vector<Hinted<SymbolLocation>> locateSymbol(const Symbol &S,
const LangOptions &LO) {
const auto L = !LO.CPlusPlus && LO.C99 ? tooling::stdlib::Lang::C
: tooling::stdlib::Lang::CXX;
switch (S.kind()) { switch (S.kind()) {
case Symbol::Declaration: case Symbol::Declaration:
return locateDecl(S.declaration()); return locateDecl(S.declaration());
case Symbol::Macro: case Symbol::Macro:
return locateMacro(S.macro()); return locateMacro(S.macro(), L);
} }
llvm_unreachable("Unknown Symbol::Kind enum"); llvm_unreachable("Unknown Symbol::Kind enum");
} }

View File

@@ -216,10 +216,9 @@ private:
++Errors; ++Errors;
return; return;
} }
writeHTMLReport( writeHTMLReport(AST.Ctx->getSourceManager().getMainFileID(), PP.Includes,
AST.Ctx->getSourceManager().getMainFileID(), PP.Includes, AST.Roots, AST.Roots, PP.MacroReferences, *AST.Ctx,
PP.MacroReferences, *AST.Ctx, getCompilerInstance().getPreprocessor(), &PI, OS);
getCompilerInstance().getPreprocessor().getHeaderSearchInfo(), &PI, OS);
} }
}; };
class ActionFactory : public tooling::FrontendActionFactory { class ActionFactory : public tooling::FrontendActionFactory {

View File

@@ -306,7 +306,7 @@ protected:
if (!V.Out) if (!V.Out)
ADD_FAILURE() << "Couldn't find any decls named " << Name << "."; ADD_FAILURE() << "Couldn't find any decls named " << Name << ".";
assert(V.Out); assert(V.Out);
return headersForSymbol(*V.Out, AST->sourceManager(), &PI); return headersForSymbol(*V.Out, AST->preprocessor(), &PI);
} }
llvm::SmallVector<Header> headersForFoo() { return headersFor("foo"); } llvm::SmallVector<Header> headersForFoo() { return headersFor("foo"); }
}; };
@@ -611,13 +611,12 @@ TEST_F(HeadersForSymbolTest, AmbiguousStdSymbolsUsingShadow) {
Visitor V; Visitor V;
V.TraverseDecl(AST->context().getTranslationUnitDecl()); V.TraverseDecl(AST->context().getTranslationUnitDecl());
ASSERT_TRUE(V.Out) << "Couldn't find a DeclRefExpr!"; ASSERT_TRUE(V.Out) << "Couldn't find a DeclRefExpr!";
EXPECT_THAT(headersForSymbol(*(V.Out->getFoundDecl()), EXPECT_THAT(
AST->sourceManager(), &PI), headersForSymbol(*(V.Out->getFoundDecl()), AST->preprocessor(), &PI),
UnorderedElementsAre( UnorderedElementsAre(
Header(*tooling::stdlib::Header::named("<cstdio>")))); Header(*tooling::stdlib::Header::named("<cstdio>"))));
} }
TEST_F(HeadersForSymbolTest, StandardHeaders) { TEST_F(HeadersForSymbolTest, StandardHeaders) {
Inputs.Code = R"cpp( Inputs.Code = R"cpp(
#include "stdlib_internal.h" #include "stdlib_internal.h"
@@ -636,6 +635,30 @@ TEST_F(HeadersForSymbolTest, StandardHeaders) {
tooling::stdlib::Header::named("<assert.h>"))); tooling::stdlib::Header::named("<assert.h>")));
} }
TEST_F(HeadersForSymbolTest, StdlibLangForMacros) {
Inputs.Code = R"cpp(
#define EOF 0
void foo() { EOF; }
)cpp";
{
buildAST();
const Macro Eof{AST->preprocessor().getIdentifierInfo("EOF"), {}};
EXPECT_THAT(
headersForSymbol(Eof, AST->preprocessor(), nullptr),
UnorderedElementsAre(tooling::stdlib::Header::named("<cstdio>"),
tooling::stdlib::Header::named("<stdio.h>")));
}
{
Inputs.ExtraArgs.push_back("-xc");
buildAST();
const Macro Eof{AST->preprocessor().getIdentifierInfo("EOF"), {}};
EXPECT_THAT(headersForSymbol(Eof, AST->preprocessor(), nullptr),
UnorderedElementsAre(tooling::stdlib::Header::named(
"<stdio.h>", tooling::stdlib::Lang::C)));
}
}
TEST_F(HeadersForSymbolTest, ExporterNoNameMatch) { TEST_F(HeadersForSymbolTest, ExporterNoNameMatch) {
Inputs.Code = R"cpp( Inputs.Code = R"cpp(
#include "exporter/foo.h" #include "exporter/foo.h"

View File

@@ -11,6 +11,7 @@
#include "clang/AST/Decl.h" #include "clang/AST/Decl.h"
#include "clang/AST/DeclBase.h" #include "clang/AST/DeclBase.h"
#include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/RecursiveASTVisitor.h"
#include "clang/Basic/LangOptions.h"
#include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceLocation.h"
#include "clang/Lex/Preprocessor.h" #include "clang/Lex/Preprocessor.h"
#include "clang/Testing/TestAST.h" #include "clang/Testing/TestAST.h"
@@ -96,6 +97,8 @@ public:
Results.emplace_back(SM.getComposedLoc(FID, Offset)); Results.emplace_back(SM.getComposedLoc(FID, Offset));
return Results; return Results;
} }
const LangOptions &langOpts() { return AST.preprocessor().getLangOpts(); }
}; };
TEST(LocateSymbol, Decl) { TEST(LocateSymbol, Decl) {
@@ -110,7 +113,7 @@ TEST(LocateSymbol, Decl) {
for (auto &Case : Cases) { for (auto &Case : Cases) {
SCOPED_TRACE(Case); SCOPED_TRACE(Case);
LocateExample Test(Case); LocateExample Test(Case);
EXPECT_THAT(locateSymbol(Test.findDecl("foo")), EXPECT_THAT(locateSymbol(Test.findDecl("foo"), Test.langOpts()),
ElementsAreArray(Test.points())); ElementsAreArray(Test.points()));
} }
} }
@@ -119,12 +122,12 @@ TEST(LocateSymbol, Stdlib) {
{ {
LocateExample Test("namespace std { struct vector; }"); LocateExample Test("namespace std { struct vector; }");
EXPECT_THAT( EXPECT_THAT(
locateSymbol(Test.findDecl("vector")), locateSymbol(Test.findDecl("vector"), Test.langOpts()),
ElementsAre(*tooling::stdlib::Symbol::named("std::", "vector"))); ElementsAre(*tooling::stdlib::Symbol::named("std::", "vector")));
} }
{ {
LocateExample Test("#define assert(x)\nvoid foo() { assert(true); }"); LocateExample Test("#define assert(x)\nvoid foo() { assert(true); }");
EXPECT_THAT(locateSymbol(Test.findMacro("assert")), EXPECT_THAT(locateSymbol(Test.findMacro("assert"), Test.langOpts()),
ElementsAre(*tooling::stdlib::Symbol::named("", "assert"))); ElementsAre(*tooling::stdlib::Symbol::named("", "assert")));
} }
} }
@@ -132,7 +135,7 @@ TEST(LocateSymbol, Stdlib) {
TEST(LocateSymbol, Macros) { TEST(LocateSymbol, Macros) {
// Make sure we preserve the last one. // Make sure we preserve the last one.
LocateExample Test("#define FOO\n#undef FOO\n#define ^FOO"); LocateExample Test("#define FOO\n#undef FOO\n#define ^FOO");
EXPECT_THAT(locateSymbol(Test.findMacro("FOO")), EXPECT_THAT(locateSymbol(Test.findMacro("FOO"), Test.langOpts()),
ElementsAreArray(Test.points())); ElementsAreArray(Test.points()));
} }
@@ -143,7 +146,7 @@ TEST(LocateSymbol, CompleteSymbolHint) {
{ {
// stdlib symbols are always complete. // stdlib symbols are always complete.
LocateExample Test("namespace std { struct vector; }"); LocateExample Test("namespace std { struct vector; }");
EXPECT_THAT(locateSymbol(Test.findDecl("vector")), EXPECT_THAT(locateSymbol(Test.findDecl("vector"), Test.langOpts()),
ElementsAre(HintedSymbol( ElementsAre(HintedSymbol(
*tooling::stdlib::Symbol::named("std::", "vector"), *tooling::stdlib::Symbol::named("std::", "vector"),
Hints::CompleteSymbol))); Hints::CompleteSymbol)));
@@ -151,7 +154,7 @@ TEST(LocateSymbol, CompleteSymbolHint) {
{ {
// macros are always complete. // macros are always complete.
LocateExample Test("#define ^FOO"); LocateExample Test("#define ^FOO");
EXPECT_THAT(locateSymbol(Test.findMacro("FOO")), EXPECT_THAT(locateSymbol(Test.findMacro("FOO"), Test.langOpts()),
ElementsAre(HintedSymbol(Test.points().front(), ElementsAre(HintedSymbol(Test.points().front(),
Hints::CompleteSymbol))); Hints::CompleteSymbol)));
} }
@@ -165,7 +168,7 @@ TEST(LocateSymbol, CompleteSymbolHint) {
for (auto &Case : Cases) { for (auto &Case : Cases) {
SCOPED_TRACE(Case); SCOPED_TRACE(Case);
LocateExample Test(Case); LocateExample Test(Case);
EXPECT_THAT(locateSymbol(Test.findDecl("foo")), EXPECT_THAT(locateSymbol(Test.findDecl("foo"), Test.langOpts()),
ElementsAre(HintedSymbol(Test.points().front(), Hints::None), ElementsAre(HintedSymbol(Test.points().front(), Hints::None),
HintedSymbol(Test.points().back(), HintedSymbol(Test.points().back(),
Hints::CompleteSymbol))); Hints::CompleteSymbol)));
@@ -181,7 +184,7 @@ TEST(LocateSymbol, CompleteSymbolHint) {
for (auto &Case : Cases) { for (auto &Case : Cases) {
SCOPED_TRACE(Case); SCOPED_TRACE(Case);
LocateExample Test(Case); LocateExample Test(Case);
EXPECT_THAT(locateSymbol(Test.findDecl("foo")), EXPECT_THAT(locateSymbol(Test.findDecl("foo"), Test.langOpts()),
Each(Field(&Hinted<SymbolLocation>::Hint, Each(Field(&Hinted<SymbolLocation>::Hint,
Eq(Hints::CompleteSymbol)))); Eq(Hints::CompleteSymbol))));
} }