https://github.com/dmaclach created https://github.com/llvm/llvm-project/pull/212564
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. >From 8426bf431269e6cf52601a94233199f61925398e Mon Sep 17 00:00:00 2001 From: Dave MacLachlan <[email protected]> Date: Tue, 28 Jul 2026 10:38:28 -0700 Subject: [PATCH] [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 e3e610b8c33d8..aaa66dd8ba989 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 @@ -468,6 +504,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 @@ -561,8 +608,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 6bf0bde9cfa10..74db5118e92d6 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); @@ -866,5 +868,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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
