https://github.com/dmaclach updated https://github.com/llvm/llvm-project/pull/212564
>From 3dc8cdffbe79821b459d09846cd22a135f6976da Mon Sep 17 00:00:00 2001 From: Dave MacLachlan <[email protected]> Date: Tue, 28 Jul 2026 10:38:28 -0700 Subject: [PATCH 1/4] [clang][include-cleaner] Support ObjC @selector expressions in WalkAST This change adds support for resolving Objective-C @selector expressions to their corresponding method or property declarations. A pre-pass (buildObjCSelectorMap) is introduced to map selectors to their declarations (methods, property getters, and property setters) across the translation unit. When the AST walker encounters an ObjCSelectorExpr, it reports the matching declarations as ambiguous references. --- .../include-cleaner/lib/Analysis.cpp | 47 ++++---- .../include-cleaner/lib/AnalysisInternal.h | 13 ++- .../include-cleaner/lib/HTMLReport.cpp | 46 ++++---- .../include-cleaner/lib/WalkAST.cpp | 62 +++++++++- .../include-cleaner/unittests/WalkASTTest.cpp | 108 ++++++++++++++++-- 5 files changed, 220 insertions(+), 56 deletions(-) diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp index e48a380211af0..5dfa291573723 100644 --- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp +++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp @@ -56,28 +56,33 @@ void walkUsed(llvm::ArrayRef<Decl *> ASTRoots, const auto &SM = PP.getSourceManager(); // This is duplicated in writeHTMLReport, changes should be mirrored there. tooling::stdlib::Recognizer Recognizer; + ObjCSelectorMap SelectorDecls; + if (!ASTRoots.empty()) { + SelectorDecls = buildObjCSelectorMap(ASTRoots.front()->getASTContext()); + } for (auto *Root : ASTRoots) { - walkAST(*Root, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { - auto SpellLoc = SM.getSpellingLoc(Loc); - // Tokens resulting from macro concatenation ends up in scratch space and - // clang currently doesn't have a good/simple APIs for tracking where - // pieces of a concataned token originated from. - // So we use the macro expansion location instead, and downgrade reference - // type to ambigious to prevent false negatives. - if (SM.isWrittenInScratchSpace(SpellLoc)) { - Loc = SM.getExpansionLoc(Loc); - if (RT == RefType::Explicit) - RT = RefType::Ambiguous; - SpellLoc = SM.getSpellingLoc(Loc); - } - auto FID = SM.getFileID(SpellLoc); - if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) - return; - // FIXME: Most of the work done here is repetitive. It might be useful to - // have a cache/batching. - SymbolReference SymRef{ND, Loc, RT}; - return CB(SymRef, headersForSymbol(ND, PP, PI)); - }); + walkAST(*Root, SelectorDecls, + [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { + auto SpellLoc = SM.getSpellingLoc(Loc); + // Tokens resulting from macro concatenation ends up in scratch + // space and clang currently doesn't have a good/simple APIs for + // tracking where pieces of a concataned token originated from. So + // we use the macro expansion location instead, and downgrade + // reference type to ambigious to prevent false negatives. + if (SM.isWrittenInScratchSpace(SpellLoc)) { + Loc = SM.getExpansionLoc(Loc); + if (RT == RefType::Explicit) + RT = RefType::Ambiguous; + SpellLoc = SM.getSpellingLoc(Loc); + } + auto FID = SM.getFileID(SpellLoc); + if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) + return; + // FIXME: Most of the work done here is repetitive. It might be + // useful to have a cache/batching. + SymbolReference SymRef{ND, Loc, RT}; + return CB(SymRef, headersForSymbol(ND, PP, PI)); + }); } for (const SymbolReference &MacroRef : MacroRefs) { assert(MacroRef.Target.kind() == Symbol::Macro); diff --git a/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h b/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h index 7d170fd15014d..c9c3042423a56 100644 --- a/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h +++ b/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h @@ -25,9 +25,12 @@ #include "clang-include-cleaner/Analysis.h" #include "clang-include-cleaner/Record.h" #include "clang-include-cleaner/Types.h" +#include "clang/Basic/IdentifierTable.h" #include "clang/Basic/LangOptions.h" #include "clang/Lex/Preprocessor.h" +#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/STLFunctionalExtras.h" +#include "llvm/ADT/SmallVector.h" #include <vector> namespace clang { @@ -38,6 +41,14 @@ class NamedDecl; class SourceLocation; namespace include_cleaner { +using ObjCSelectorMap = + llvm::DenseMap<Selector, llvm::SmallVector<NamedDecl *, 2>>; + +/// Builds a map from Objective-C Selectors to their declarations in the given +/// ASTContext. +/// This is used to optimize selector lookups during AST walking. +ObjCSelectorMap buildObjCSelectorMap(ASTContext &Ctx); + /// Traverses part of the AST from \p Root, finding uses of symbols. /// /// Each use is reported to the callback: @@ -50,7 +61,7 @@ namespace include_cleaner { /// /// walkAST is typically called once per top-level declaration in the file /// being analyzed, in order to find all references within it. -void walkAST(Decl &Root, +void walkAST(Decl &Root, const ObjCSelectorMap &SelectorDecls, llvm::function_ref<void(SourceLocation, NamedDecl &, RefType)>); /// Finds the headers that provide the symbol location. diff --git a/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp b/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp index 3e067f84432ac..c7a8b1728925f 100644 --- a/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp +++ b/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp @@ -503,29 +503,31 @@ void writeHTMLReport(FileID File, const include_cleaner::Includes &Includes, llvm::raw_ostream &OS) { Reporter R(OS, Ctx, PP, Includes, PI, File); const auto &SM = Ctx.getSourceManager(); + ObjCSelectorMap SelectorDecls = buildObjCSelectorMap(Ctx); for (Decl *Root : Roots) - walkAST(*Root, [&](SourceLocation Loc, const NamedDecl &D, RefType T) { - // FIXME: we should merge this logic with `walkUsed` to prevent - // divergences in the future. It isn't trivial though, as we also update - // RefType. Since HTMLReport is only used for debugging purposes, - // divergences aren't critical. - auto SpellLoc = SM.getSpellingLoc(Loc); - // Tokens resulting from macro concatenation ends up in scratch space and - // clang currently doesn't have a good/simple APIs for tracking where - // pieces of a concataned token originated from. - // So we use the macro expansion location instead, and downgrade reference - // type to ambigious to prevent false negatives. - if (SM.isWrittenInScratchSpace(SpellLoc)) { - Loc = SM.getExpansionLoc(Loc); - if (T == RefType::Explicit) - T = RefType::Ambiguous; - SpellLoc = SM.getSpellingLoc(Loc); - } - auto FID = SM.getFileID(SpellLoc); - if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) - return; - R.addRef(SymbolReference{D, Loc, T}); - }); + walkAST(*Root, SelectorDecls, + [&](SourceLocation Loc, const NamedDecl &D, RefType T) { + // FIXME: we should merge this logic with `walkUsed` to prevent + // divergences in the future. It isn't trivial though, as we also + // update RefType. Since HTMLReport is only used for debugging + // purposes, divergences aren't critical. + auto SpellLoc = SM.getSpellingLoc(Loc); + // Tokens resulting from macro concatenation ends up in scratch + // space and clang currently doesn't have a good/simple APIs for + // tracking where pieces of a concataned token originated from. So + // we use the macro expansion location instead, and downgrade + // reference type to ambigious to prevent false negatives. + if (SM.isWrittenInScratchSpace(SpellLoc)) { + Loc = SM.getExpansionLoc(Loc); + if (T == RefType::Explicit) + T = RefType::Ambiguous; + SpellLoc = SM.getSpellingLoc(Loc); + } + auto FID = SM.getFileID(SpellLoc); + if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) + return; + R.addRef(SymbolReference{D, Loc, T}); + }); for (const SymbolReference &Ref : MacroRefs) { if (!SM.isWrittenInMainFile(SM.getSpellingLoc(Ref.RefLocation))) continue; diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp index 7d15f96405903..03bd3e8f61d00 100644 --- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp +++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp @@ -12,9 +12,11 @@ #include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" #include "clang/AST/DeclFriend.h" +#include "clang/AST/DeclObjC.h" #include "clang/AST/DeclTemplate.h" #include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" +#include "clang/AST/ExprObjC.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/TemplateBase.h" #include "clang/AST/TemplateName.h" @@ -24,10 +26,12 @@ #include "clang/Basic/OperatorKinds.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/Specifiers.h" +#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/STLFunctionalExtras.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Support/Casting.h" #include "llvm/Support/ErrorHandling.h" +#include <utility> namespace clang::include_cleaner { namespace { @@ -39,8 +43,39 @@ bool isOperatorNewDelete(OverloadedOperatorKind OpKind) { using DeclCallback = llvm::function_ref<void(SourceLocation, NamedDecl &, RefType)>; +class ObjCSelectorDeclMapBuilder + : public RecursiveASTVisitor<ObjCSelectorDeclMapBuilder> { +public: + ObjCSelectorMap build() && { return std::move(Map); } + + bool TraverseDecl(clang::Decl *D) { + if (!D) + return true; + if (auto *Container = llvm::dyn_cast<clang::ObjCContainerDecl>(D)) { + for (clang::ObjCMethodDecl *M : Container->methods()) { + if (M) { + Map[M->getSelector()].push_back(M); + } + } + for (clang::ObjCPropertyDecl *Prop : Container->properties()) { + if (Prop) { + if (auto Getter = Prop->getGetterName(); !Getter.isNull()) + Map[Getter].push_back(Prop); + if (auto Setter = Prop->getSetterName(); !Setter.isNull()) + Map[Setter].push_back(Prop); + } + } + } + return RecursiveASTVisitor::TraverseDecl(D); + } + +private: + ObjCSelectorMap Map; +}; + class ASTWalker : public RecursiveASTVisitor<ASTWalker> { DeclCallback Callback; + const ObjCSelectorMap &SelectorDecls; void report(SourceLocation Loc, NamedDecl *ND, RefType RT = RefType::Explicit) { @@ -102,7 +137,8 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { } public: - ASTWalker(DeclCallback Callback) : Callback(Callback) {} + ASTWalker(DeclCallback Callback, const ObjCSelectorMap &SelectorDecls) + : Callback(Callback), SelectorDecls(SelectorDecls) {} // Operators are almost always ADL extension points and by design references // to them doesn't count as uses (generally the type should provide them, so @@ -479,6 +515,17 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { return true; } + bool VisitObjCSelectorExpr(ObjCSelectorExpr *E) { + auto Sel = E->getSelector(); + auto It = SelectorDecls.find(Sel); + if (It != SelectorDecls.end()) { + for (NamedDecl *ND : It->second) { + report(E->getSelectorNameLoc(), ND, RefType::Ambiguous); + } + } + return true; + } + bool VisitCastExpr(CastExpr *E) { // Handle implicit or explicit casts between Objective-C object pointers // aimed towards protocol-qualification (e.g., `ClassName *` to @@ -572,8 +619,17 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { } // namespace -void walkAST(Decl &Root, DeclCallback Callback) { - ASTWalker(Callback).TraverseDecl(&Root); +ObjCSelectorMap buildObjCSelectorMap(ASTContext &Ctx) { + ObjCSelectorDeclMapBuilder Builder; + if (Ctx.getLangOpts().ObjC) { + Builder.TraverseDecl(Ctx.getTranslationUnitDecl()); + } + return std::move(Builder).build(); +} + +void walkAST(Decl &Root, const ObjCSelectorMap &SelectorDecls, + DeclCallback Callback) { + ASTWalker(Callback, SelectorDecls).TraverseDecl(&Root); } } // namespace clang::include_cleaner diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp index cf9a5a365edb6..6dd01f44f1223 100644 --- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp @@ -67,18 +67,20 @@ testWalk(llvm::StringRef TargetCode, llvm::StringRef ReferencingCode, std::vector<Decl::Kind> TargetDecls; // Perform the walk, and capture the offsets of the referenced targets. std::unordered_map<RefType, std::vector<size_t>> ReferencedOffsets; + ObjCSelectorMap SelectorDecls = buildObjCSelectorMap(AST.context()); for (Decl *D : AST.context().getTranslationUnitDecl()->decls()) { if (ReferencingFile != SM.getDecomposedExpansionLoc(D->getLocation()).first) continue; - walkAST(*D, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { - if (SM.getFileLoc(Loc) != ReferencingLoc) - return; - auto NDLoc = SM.getDecomposedLoc(SM.getFileLoc(ND.getLocation())); - if (NDLoc.first != TargetFile) - return; - ReferencedOffsets[RT].push_back(NDLoc.second); - TargetDecls.push_back(ND.getKind()); - }); + walkAST(*D, SelectorDecls, + [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { + if (SM.getFileLoc(Loc) != ReferencingLoc) + return; + auto NDLoc = SM.getDecomposedLoc(SM.getFileLoc(ND.getLocation())); + if (NDLoc.first != TargetFile) + return; + ReferencedOffsets[RT].push_back(NDLoc.second); + TargetDecls.push_back(ND.getKind()); + }); } for (auto &Entry : ReferencedOffsets) llvm::sort(Entry.second); @@ -1164,5 +1166,93 @@ TEST(WalkAST, ObjCIvarRefExprFree) { {"-x", "objective-c"}); } +TEST(WalkAST, ObjCSelectorExpr) { + testWalk(R"objc( + @interface MyClass + $ambiguous^- (void)doSomething; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^doSomething); + } + )objc", + {"-x", "objective-c"}); +} + +TEST(WalkAST, ObjCSelectorExprPropertyGetter) { + testWalk(R"objc( + @interface MyClass + @property(nonatomic) int $ambiguous^foo; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^foo); + } + )objc", + {"-x", "objective-c"}); +} + +TEST(WalkAST, ObjCSelectorExprPropertySetter) { + testWalk(R"objc( + @interface MyClass + @property(nonatomic) int $ambiguous^foo; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^setFoo:); + } + )objc", + {"-x", "objective-c"}); +} + +TEST(WalkAST, ObjCSelectorExprMultipleMatches) { + testWalk(R"objc( + @interface MyClass1 + $ambiguous^- (void)doSomething; + @end + + @interface MyClass2 + $ambiguous^- (void)doSomething; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^doSomething); + } + )objc", + {"-x", "objective-c"}); +} + +TEST(WalkAST, ObjCSelectorExprInProtocol) { + testWalk(R"objc( + @protocol MyProtocol + $ambiguous^- (void)protocolMethod; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^protocolMethod); + } + )objc", + {"-x", "objective-c"}); +} + +TEST(WalkAST, ObjCSelectorExprMultiColon) { + testWalk(R"objc( + @interface MyClass + $ambiguous^- (void)doA:(int)a withB:(int)b; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^doA:withB:); + } + )objc", + {"-x", "objective-c"}); +} + } // namespace } // namespace clang::include_cleaner >From af1df5124fcb965dcb555d8b0db97b826c55aeee Mon Sep 17 00:00:00 2001 From: Dave MacLachlan <[email protected]> Date: Fri, 31 Jul 2026 14:34:32 -0700 Subject: [PATCH 2/4] Updated with special case for read only properties. Added test --- clang-tools-extra/include-cleaner/lib/WalkAST.cpp | 7 +++++-- .../include-cleaner/unittests/WalkASTTest.cpp | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp index 03bd3e8f61d00..f88a01dcdc5b5 100644 --- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp +++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp @@ -61,8 +61,11 @@ class ObjCSelectorDeclMapBuilder if (Prop) { if (auto Getter = Prop->getGetterName(); !Getter.isNull()) Map[Getter].push_back(Prop); - if (auto Setter = Prop->getSetterName(); !Setter.isNull()) - Map[Setter].push_back(Prop); + if (!Prop->isReadOnly()) { + if (auto Setter = Prop->getSetterName(); !Setter.isNull()) { + Map[Setter].push_back(Prop); + } + } } } } diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp index 6dd01f44f1223..bfad8276fa2e6 100644 --- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp @@ -1208,6 +1208,21 @@ TEST(WalkAST, ObjCSelectorExprPropertySetter) { {"-x", "objective-c"}); } +TEST(WalkAST, ObjCSelectorExprReadOnlyPropertySetter) { + // Read-only properties do not generate setter selectors. + testWalk(R"objc( + @interface MyClass + @property(readonly, nonatomic) int foo; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^setFoo:); + } + )objc", + {"-x", "objective-c"}); +} + TEST(WalkAST, ObjCSelectorExprMultipleMatches) { testWalk(R"objc( @interface MyClass1 >From 83554689626e96d601e159f8c94ec8011af4c6a5 Mon Sep 17 00:00:00 2001 From: Dave MacLachlan <[email protected]> Date: Fri, 7 Aug 2026 16:53:52 -0700 Subject: [PATCH 3/4] Cleaned up passing by rvalue ref as it isn't needed. --- clang-tools-extra/include-cleaner/lib/WalkAST.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp index f88a01dcdc5b5..254685c8f558a 100644 --- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp +++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp @@ -46,7 +46,7 @@ using DeclCallback = class ObjCSelectorDeclMapBuilder : public RecursiveASTVisitor<ObjCSelectorDeclMapBuilder> { public: - ObjCSelectorMap build() && { return std::move(Map); } + ObjCSelectorMap takeMap() { return std::move(Map); } bool TraverseDecl(clang::Decl *D) { if (!D) @@ -627,7 +627,7 @@ ObjCSelectorMap buildObjCSelectorMap(ASTContext &Ctx) { if (Ctx.getLangOpts().ObjC) { Builder.TraverseDecl(Ctx.getTranslationUnitDecl()); } - return std::move(Builder).build(); + return Builder.takeMap(); } void walkAST(Decl &Root, const ObjCSelectorMap &SelectorDecls, >From dc564119504de9d80a4b9961b3b1623d20b1d6b0 Mon Sep 17 00:00:00 2001 From: Dave MacLachlan <[email protected]> Date: Thu, 13 Aug 2026 11:42:09 -0700 Subject: [PATCH 4/4] Based on comments: - Changed over to a conditional post traversal IFF there are selectors to be resolved. - Resolved issues with both the getter and the property being recorded (and added test) - Added tests for `@property(getter=isFoo, setter=setTheFoo)) int foo` - Cleaned up style nits --- .../include-cleaner/lib/Analysis.cpp | 47 +++++----- .../include-cleaner/lib/AnalysisInternal.h | 13 +-- .../include-cleaner/lib/HTMLReport.cpp | 46 +++++----- .../include-cleaner/lib/WalkAST.cpp | 83 ++++++++++------- .../include-cleaner/unittests/WalkASTTest.cpp | 92 +++++++++++++++---- 5 files changed, 171 insertions(+), 110 deletions(-) diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp index 5dfa291573723..e48a380211af0 100644 --- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp +++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp @@ -56,33 +56,28 @@ void walkUsed(llvm::ArrayRef<Decl *> ASTRoots, const auto &SM = PP.getSourceManager(); // This is duplicated in writeHTMLReport, changes should be mirrored there. tooling::stdlib::Recognizer Recognizer; - ObjCSelectorMap SelectorDecls; - if (!ASTRoots.empty()) { - SelectorDecls = buildObjCSelectorMap(ASTRoots.front()->getASTContext()); - } for (auto *Root : ASTRoots) { - walkAST(*Root, SelectorDecls, - [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { - auto SpellLoc = SM.getSpellingLoc(Loc); - // Tokens resulting from macro concatenation ends up in scratch - // space and clang currently doesn't have a good/simple APIs for - // tracking where pieces of a concataned token originated from. So - // we use the macro expansion location instead, and downgrade - // reference type to ambigious to prevent false negatives. - if (SM.isWrittenInScratchSpace(SpellLoc)) { - Loc = SM.getExpansionLoc(Loc); - if (RT == RefType::Explicit) - RT = RefType::Ambiguous; - SpellLoc = SM.getSpellingLoc(Loc); - } - auto FID = SM.getFileID(SpellLoc); - if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) - return; - // FIXME: Most of the work done here is repetitive. It might be - // useful to have a cache/batching. - SymbolReference SymRef{ND, Loc, RT}; - return CB(SymRef, headersForSymbol(ND, PP, PI)); - }); + walkAST(*Root, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { + auto SpellLoc = SM.getSpellingLoc(Loc); + // Tokens resulting from macro concatenation ends up in scratch space and + // clang currently doesn't have a good/simple APIs for tracking where + // pieces of a concataned token originated from. + // So we use the macro expansion location instead, and downgrade reference + // type to ambigious to prevent false negatives. + if (SM.isWrittenInScratchSpace(SpellLoc)) { + Loc = SM.getExpansionLoc(Loc); + if (RT == RefType::Explicit) + RT = RefType::Ambiguous; + SpellLoc = SM.getSpellingLoc(Loc); + } + auto FID = SM.getFileID(SpellLoc); + if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) + return; + // FIXME: Most of the work done here is repetitive. It might be useful to + // have a cache/batching. + SymbolReference SymRef{ND, Loc, RT}; + return CB(SymRef, headersForSymbol(ND, PP, PI)); + }); } for (const SymbolReference &MacroRef : MacroRefs) { assert(MacroRef.Target.kind() == Symbol::Macro); diff --git a/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h b/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h index c9c3042423a56..7d170fd15014d 100644 --- a/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h +++ b/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h @@ -25,12 +25,9 @@ #include "clang-include-cleaner/Analysis.h" #include "clang-include-cleaner/Record.h" #include "clang-include-cleaner/Types.h" -#include "clang/Basic/IdentifierTable.h" #include "clang/Basic/LangOptions.h" #include "clang/Lex/Preprocessor.h" -#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/STLFunctionalExtras.h" -#include "llvm/ADT/SmallVector.h" #include <vector> namespace clang { @@ -41,14 +38,6 @@ class NamedDecl; class SourceLocation; namespace include_cleaner { -using ObjCSelectorMap = - llvm::DenseMap<Selector, llvm::SmallVector<NamedDecl *, 2>>; - -/// Builds a map from Objective-C Selectors to their declarations in the given -/// ASTContext. -/// This is used to optimize selector lookups during AST walking. -ObjCSelectorMap buildObjCSelectorMap(ASTContext &Ctx); - /// Traverses part of the AST from \p Root, finding uses of symbols. /// /// Each use is reported to the callback: @@ -61,7 +50,7 @@ ObjCSelectorMap buildObjCSelectorMap(ASTContext &Ctx); /// /// walkAST is typically called once per top-level declaration in the file /// being analyzed, in order to find all references within it. -void walkAST(Decl &Root, const ObjCSelectorMap &SelectorDecls, +void walkAST(Decl &Root, llvm::function_ref<void(SourceLocation, NamedDecl &, RefType)>); /// Finds the headers that provide the symbol location. diff --git a/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp b/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp index c7a8b1728925f..3e067f84432ac 100644 --- a/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp +++ b/clang-tools-extra/include-cleaner/lib/HTMLReport.cpp @@ -503,31 +503,29 @@ void writeHTMLReport(FileID File, const include_cleaner::Includes &Includes, llvm::raw_ostream &OS) { Reporter R(OS, Ctx, PP, Includes, PI, File); const auto &SM = Ctx.getSourceManager(); - ObjCSelectorMap SelectorDecls = buildObjCSelectorMap(Ctx); for (Decl *Root : Roots) - walkAST(*Root, SelectorDecls, - [&](SourceLocation Loc, const NamedDecl &D, RefType T) { - // FIXME: we should merge this logic with `walkUsed` to prevent - // divergences in the future. It isn't trivial though, as we also - // update RefType. Since HTMLReport is only used for debugging - // purposes, divergences aren't critical. - auto SpellLoc = SM.getSpellingLoc(Loc); - // Tokens resulting from macro concatenation ends up in scratch - // space and clang currently doesn't have a good/simple APIs for - // tracking where pieces of a concataned token originated from. So - // we use the macro expansion location instead, and downgrade - // reference type to ambigious to prevent false negatives. - if (SM.isWrittenInScratchSpace(SpellLoc)) { - Loc = SM.getExpansionLoc(Loc); - if (T == RefType::Explicit) - T = RefType::Ambiguous; - SpellLoc = SM.getSpellingLoc(Loc); - } - auto FID = SM.getFileID(SpellLoc); - if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) - return; - R.addRef(SymbolReference{D, Loc, T}); - }); + walkAST(*Root, [&](SourceLocation Loc, const NamedDecl &D, RefType T) { + // FIXME: we should merge this logic with `walkUsed` to prevent + // divergences in the future. It isn't trivial though, as we also update + // RefType. Since HTMLReport is only used for debugging purposes, + // divergences aren't critical. + auto SpellLoc = SM.getSpellingLoc(Loc); + // Tokens resulting from macro concatenation ends up in scratch space and + // clang currently doesn't have a good/simple APIs for tracking where + // pieces of a concataned token originated from. + // So we use the macro expansion location instead, and downgrade reference + // type to ambigious to prevent false negatives. + if (SM.isWrittenInScratchSpace(SpellLoc)) { + Loc = SM.getExpansionLoc(Loc); + if (T == RefType::Explicit) + T = RefType::Ambiguous; + SpellLoc = SM.getSpellingLoc(Loc); + } + auto FID = SM.getFileID(SpellLoc); + if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) + return; + R.addRef(SymbolReference{D, Loc, T}); + }); for (const SymbolReference &Ref : MacroRefs) { if (!SM.isWrittenInMainFile(SM.getSpellingLoc(Ref.RefLocation))) continue; diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp index 254685c8f558a..770b64b065846 100644 --- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp +++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp @@ -27,6 +27,7 @@ #include "clang/Basic/SourceLocation.h" #include "clang/Basic/Specifiers.h" #include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/DenseSet.h" #include "llvm/ADT/STLFunctionalExtras.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Support/Casting.h" @@ -43,28 +44,35 @@ bool isOperatorNewDelete(OverloadedOperatorKind OpKind) { using DeclCallback = llvm::function_ref<void(SourceLocation, NamedDecl &, RefType)>; -class ObjCSelectorDeclMapBuilder - : public RecursiveASTVisitor<ObjCSelectorDeclMapBuilder> { +using SelectorMap = llvm::DenseMap<Selector, llvm::SmallVector<NamedDecl *, 2>>; + +class TargetedSelectorDeclCollector + : public RecursiveASTVisitor<TargetedSelectorDeclCollector> { public: - ObjCSelectorMap takeMap() { return std::move(Map); } + explicit TargetedSelectorDeclCollector( + const llvm::DenseSet<Selector> &NeededSelectors) + : NeededSelectors(NeededSelectors) {} + + SelectorMap takeMap() { return std::move(Map); } bool TraverseDecl(clang::Decl *D) { if (!D) return true; if (auto *Container = llvm::dyn_cast<clang::ObjCContainerDecl>(D)) { - for (clang::ObjCMethodDecl *M : Container->methods()) { - if (M) { + for (auto *M : Container->methods()) { + if (M && !M->isPropertyAccessor() && + NeededSelectors.contains(M->getSelector())) Map[M->getSelector()].push_back(M); - } } - for (clang::ObjCPropertyDecl *Prop : Container->properties()) { + for (auto *Prop : Container->properties()) { if (Prop) { - if (auto Getter = Prop->getGetterName(); !Getter.isNull()) + if (auto Getter = Prop->getGetterName(); + !Getter.isNull() && NeededSelectors.contains(Getter)) Map[Getter].push_back(Prop); if (!Prop->isReadOnly()) { - if (auto Setter = Prop->getSetterName(); !Setter.isNull()) { + if (auto Setter = Prop->getSetterName(); + !Setter.isNull() && NeededSelectors.contains(Setter)) Map[Setter].push_back(Prop); - } } } } @@ -73,12 +81,13 @@ class ObjCSelectorDeclMapBuilder } private: - ObjCSelectorMap Map; + const llvm::DenseSet<Selector> &NeededSelectors; + SelectorMap Map; }; class ASTWalker : public RecursiveASTVisitor<ASTWalker> { DeclCallback Callback; - const ObjCSelectorMap &SelectorDecls; + llvm::SmallVector<ObjCSelectorExpr *, 4> RecordedSelectors; void report(SourceLocation Loc, NamedDecl *ND, RefType RT = RefType::Explicit) { @@ -140,8 +149,11 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { } public: - ASTWalker(DeclCallback Callback, const ObjCSelectorMap &SelectorDecls) - : Callback(Callback), SelectorDecls(SelectorDecls) {} + ASTWalker(DeclCallback Callback) : Callback(Callback) {} + + llvm::ArrayRef<ObjCSelectorExpr *> getRecordedSelectors() const { + return RecordedSelectors; + } // Operators are almost always ADL extension points and by design references // to them doesn't count as uses (generally the type should provide them, so @@ -519,13 +531,7 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { } bool VisitObjCSelectorExpr(ObjCSelectorExpr *E) { - auto Sel = E->getSelector(); - auto It = SelectorDecls.find(Sel); - if (It != SelectorDecls.end()) { - for (NamedDecl *ND : It->second) { - report(E->getSelectorNameLoc(), ND, RefType::Ambiguous); - } - } + RecordedSelectors.push_back(E); return true; } @@ -622,17 +628,32 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> { } // namespace -ObjCSelectorMap buildObjCSelectorMap(ASTContext &Ctx) { - ObjCSelectorDeclMapBuilder Builder; - if (Ctx.getLangOpts().ObjC) { - Builder.TraverseDecl(Ctx.getTranslationUnitDecl()); +void walkAST(Decl &Root, DeclCallback Callback) { + ASTWalker Walker(Callback); + Walker.TraverseDecl(&Root); + + ASTContext &Ctx = Root.getASTContext(); + if (!Ctx.getLangOpts().ObjC) + return; + + auto RecordedSelectors = Walker.getRecordedSelectors(); + if (RecordedSelectors.empty()) + return; + + llvm::DenseSet<Selector> NeededSelectors; + for (const auto *E : RecordedSelectors) + NeededSelectors.insert(E->getSelector()); + TargetedSelectorDeclCollector Collector(NeededSelectors); + Collector.TraverseDecl(Ctx.getTranslationUnitDecl()); + auto Map = Collector.takeMap(); + for (const auto *E : RecordedSelectors) { + auto It = Map.find(E->getSelector()); + if (It != Map.end()) { + for (NamedDecl *ND : It->second) + Callback(E->getSelectorNameLoc(), + *cast<NamedDecl>(ND->getCanonicalDecl()), RefType::Ambiguous); + } } - return Builder.takeMap(); -} - -void walkAST(Decl &Root, const ObjCSelectorMap &SelectorDecls, - DeclCallback Callback) { - ASTWalker(Callback, SelectorDecls).TraverseDecl(&Root); } } // namespace clang::include_cleaner diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp index bfad8276fa2e6..200a21466738b 100644 --- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp @@ -67,20 +67,18 @@ testWalk(llvm::StringRef TargetCode, llvm::StringRef ReferencingCode, std::vector<Decl::Kind> TargetDecls; // Perform the walk, and capture the offsets of the referenced targets. std::unordered_map<RefType, std::vector<size_t>> ReferencedOffsets; - ObjCSelectorMap SelectorDecls = buildObjCSelectorMap(AST.context()); for (Decl *D : AST.context().getTranslationUnitDecl()->decls()) { if (ReferencingFile != SM.getDecomposedExpansionLoc(D->getLocation()).first) continue; - walkAST(*D, SelectorDecls, - [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { - if (SM.getFileLoc(Loc) != ReferencingLoc) - return; - auto NDLoc = SM.getDecomposedLoc(SM.getFileLoc(ND.getLocation())); - if (NDLoc.first != TargetFile) - return; - ReferencedOffsets[RT].push_back(NDLoc.second); - TargetDecls.push_back(ND.getKind()); - }); + walkAST(*D, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) { + if (SM.getFileLoc(Loc) != ReferencingLoc) + return; + auto NDLoc = SM.getDecomposedLoc(SM.getFileLoc(ND.getLocation())); + if (NDLoc.first != TargetFile) + return; + ReferencedOffsets[RT].push_back(NDLoc.second); + TargetDecls.push_back(ND.getKind()); + }); } for (auto &Entry : ReferencedOffsets) llvm::sort(Entry.second); @@ -1181,31 +1179,33 @@ TEST(WalkAST, ObjCSelectorExpr) { } TEST(WalkAST, ObjCSelectorExprPropertyGetter) { - testWalk(R"objc( + auto Decls = testWalk(R"objc( @interface MyClass @property(nonatomic) int $ambiguous^foo; @end )objc", - R"objc( + R"objc( void test() { SEL s = @selector(^foo); } )objc", - {"-x", "objective-c"}); + {"-x", "objective-c"}); + EXPECT_THAT(Decls, ElementsAre(Decl::ObjCProperty)); } TEST(WalkAST, ObjCSelectorExprPropertySetter) { - testWalk(R"objc( + auto Decls = testWalk(R"objc( @interface MyClass @property(nonatomic) int $ambiguous^foo; @end )objc", - R"objc( + R"objc( void test() { SEL s = @selector(^setFoo:); } )objc", - {"-x", "objective-c"}); + {"-x", "objective-c"}); + EXPECT_THAT(Decls, ElementsAre(Decl::ObjCProperty)); } TEST(WalkAST, ObjCSelectorExprReadOnlyPropertySetter) { @@ -1269,5 +1269,63 @@ TEST(WalkAST, ObjCSelectorExprMultiColon) { {"-x", "objective-c"}); } +TEST(WalkAST, ObjCPropertyRefExprCustomGetter) { + testWalk(R"objc( + @interface $implicit^MyClass + @property(getter=isFoo, setter=setTheFoo:, nonatomic) int $explicit^foo; + @end + )objc", + R"objc( + void test(MyClass *obj) { + int x = obj.^foo; + } + )objc", + {"-x", "objective-c"}); +} + +TEST(WalkAST, ObjCPropertyRefExprCustomSetter) { + testWalk(R"objc( + @interface $implicit^MyClass + @property(getter=isFoo, setter=setTheFoo:, nonatomic) int $explicit^foo; + @end + )objc", + R"objc( + void test(MyClass *obj) { + obj.^foo = 42; + } + )objc", + {"-x", "objective-c"}); +} + +TEST(WalkAST, ObjCSelectorExprCustomPropertyGetter) { + auto Decls = testWalk(R"objc( + @interface MyClass + @property(getter=isFoo, setter=setTheFoo:, nonatomic) int $ambiguous^foo; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^isFoo); + } + )objc", + {"-x", "objective-c"}); + EXPECT_THAT(Decls, ElementsAre(Decl::ObjCProperty)); +} + +TEST(WalkAST, ObjCSelectorExprCustomPropertySetter) { + auto Decls = testWalk(R"objc( + @interface MyClass + @property(getter=isFoo, setter=setTheFoo:, nonatomic) int $ambiguous^foo; + @end + )objc", + R"objc( + void test() { + SEL s = @selector(^setTheFoo:); + } + )objc", + {"-x", "objective-c"}); + EXPECT_THAT(Decls, ElementsAre(Decl::ObjCProperty)); +} + } // namespace } // namespace clang::include_cleaner _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
