https://github.com/tJener updated https://github.com/llvm/llvm-project/pull/237451
>From 808aa904eb29a6d5a706721b7db1b2d533434475 Mon Sep 17 00:00:00 2001 From: Eric Li <[email protected]> Date: Fri, 30 Oct 2026 20:11:00 -0400 Subject: [PATCH 1/2] [clang][AST] Don't give `ObjCProtocolLoc` pointer identity in `DynTypedNode` `DynTypedNode` has stored `ObjCProtocolLoc` by value since the node was added in 805f7a4fa4ce, but `NKI_ObjCProtocolLoc` was placed after `NKI_LastKindWithoutPointerIdentity`, so `hasPointerIdentity()` returned true for it. `getMemoizationData()` then returned (by chance?) the stored `ObjCProtocolDecl` pointer. As a result, two references to the same protocol at different locations compared equal through `DynTypedNode`. This caused `ASTContext::getParents()` to behave incorrectly. This commit changes `ObjCProtocolLoc` to be considered as not having pointer identity. Subsequently, we handle `ObjCProtocolLoc` in `DynTypedNode::operator<`, `operator==` and `getHashValue`, using both the protocol pointer and the source location. To catch this class of mismatch at compile time, this commit also introduces a `static_assert` to check that how it stores the node matches `hasPointerIdentity()` for the node kind. --- clang/include/clang/AST/ASTTypeTraits.h | 45 +++++++++++++++++-- clang/lib/AST/ASTTypeTraits.cpp | 2 +- .../unittests/AST/ASTContextParentMapTest.cpp | 14 ++++++ clang/unittests/AST/ASTTypeTraitsTest.cpp | 39 ++++++++++++++++ 4 files changed, 95 insertions(+), 5 deletions(-) diff --git a/clang/include/clang/AST/ASTTypeTraits.h b/clang/include/clang/AST/ASTTypeTraits.h index ee81826bfe1383..db2c2d451df937 100644 --- a/clang/include/clang/AST/ASTTypeTraits.h +++ b/clang/include/clang/AST/ASTTypeTraits.h @@ -133,6 +133,7 @@ class ASTNodeKind { NKI_TemplateName, NKI_NestedNameSpecifier, NKI_NestedNameSpecifierLoc, + NKI_ObjCProtocolLoc, NKI_QualType, #define TYPELOC(CLASS, PARENT) NKI_##CLASS##TypeLoc, #include "clang/AST/TypeLocNodes.def" @@ -156,7 +157,6 @@ class ASTNodeKind { NKI_Attr, #define ATTR(A) NKI_##A##Attr, #include "clang/Basic/AttrList.inc" - NKI_ObjCProtocolLoc, NKI_ConceptReference, NKI_OffsetOfNode, NKI_NumberOfKinds @@ -253,9 +253,15 @@ inline raw_ostream &operator<<(raw_ostream &OS, ASTNodeKind K) { class DynTypedNode { public: /// Creates a \c DynTypedNode from \c Node. - template <typename T> - static DynTypedNode create(const T &Node) { - return BaseConverter<T>::create(Node); + template <typename T> static DynTypedNode create(const T &Node) { + using Converter = BaseConverter<T>; + static_assert( + Converter::StoresPointer == + ASTNodeKind::getFromNodeKind<typename Converter::StoredType>() + .hasPointerIdentity(), + "hasPointerIdentity() must agree with whether DynTypedNode " + "stores a node by pointer"); + return Converter::create(Node); } /// Retrieve the stored node as type \c T. @@ -341,6 +347,15 @@ class DynTypedNode { return getUnchecked<NestedNameSpecifier>().getAsVoidPointer() < Other.getUnchecked<NestedNameSpecifier>().getAsVoidPointer(); + if (ASTNodeKind::getFromNodeKind<ObjCProtocolLoc>().isSame(NodeKind)) { + auto PLA = getUnchecked<ObjCProtocolLoc>(); + auto PLB = Other.getUnchecked<ObjCProtocolLoc>(); + return std::make_pair(PLA.getProtocol(), + PLA.getLocation().getRawEncoding()) < + std::make_pair(PLB.getProtocol(), + PLB.getLocation().getRawEncoding()); + } + assert(getMemoizationData() && Other.getMemoizationData()); return getMemoizationData() < Other.getMemoizationData(); } @@ -365,6 +380,13 @@ class DynTypedNode { return getUnchecked<NestedNameSpecifier>() == Other.getUnchecked<NestedNameSpecifier>(); + if (ASTNodeKind::getFromNodeKind<ObjCProtocolLoc>().isSame(NodeKind)) { + auto PLA = getUnchecked<ObjCProtocolLoc>(); + auto PLB = Other.getUnchecked<ObjCProtocolLoc>(); + return PLA.getProtocol() == PLB.getProtocol() && + PLA.getLocation() == PLB.getLocation(); + } + assert(getMemoizationData() && Other.getMemoizationData()); return getMemoizationData() == Other.getMemoizationData(); } @@ -396,6 +418,13 @@ class DynTypedNode { return llvm::DenseMapInfo<NestedNameSpecifier>::getHashValue( Val.getUnchecked<NestedNameSpecifier>()); + if (ASTNodeKind::getFromNodeKind<ObjCProtocolLoc>().isSame( + Val.NodeKind)) { + auto PL = Val.getUnchecked<ObjCProtocolLoc>(); + return llvm::hash_combine(PL.getProtocol(), + PL.getLocation().getRawEncoding()); + } + assert(Val.getMemoizationData()); return llvm::hash_value(Val.getMemoizationData()); } @@ -410,6 +439,8 @@ class DynTypedNode { /// Converter that uses dyn_cast<T> from a stored BaseT*. template <typename T, typename BaseT> struct DynCastPtrConverter { + static constexpr bool StoresPointer = true; + using StoredType = BaseT; static const T *get(ASTNodeKind NodeKind, const void *Storage) { if (ASTNodeKind::getFromNodeKind<T>().isBaseOf(NodeKind)) return &getUnchecked(NodeKind, Storage); @@ -430,6 +461,8 @@ class DynTypedNode { /// Converter that stores T* (by pointer). template <typename T> struct PtrConverter { + static constexpr bool StoresPointer = true; + using StoredType = T; static const T *get(ASTNodeKind NodeKind, const void *Storage) { if (ASTNodeKind::getFromNodeKind<T>().isSame(NodeKind)) return &getUnchecked(NodeKind, Storage); @@ -450,6 +483,8 @@ class DynTypedNode { /// Converter that stores T (by value). template <typename T> struct ValueConverter { + static constexpr bool StoresPointer = false; + using StoredType = T; static const T *get(ASTNodeKind NodeKind, const void *Storage) { if (ASTNodeKind::getFromNodeKind<T>().isSame(NodeKind)) return reinterpret_cast<const T *>(Storage); @@ -473,6 +508,8 @@ class DynTypedNode { template <typename T, typename BaseT, typename = std::enable_if_t<(sizeof(T) == sizeof(BaseT))>> struct DynCastValueConverter { + static constexpr bool StoresPointer = false; + using StoredType = BaseT; static const T *get(ASTNodeKind NodeKind, const void *Storage) { if (ASTNodeKind::getFromNodeKind<T>().isBaseOf(NodeKind)) return &getUnchecked(NodeKind, Storage); diff --git a/clang/lib/AST/ASTTypeTraits.cpp b/clang/lib/AST/ASTTypeTraits.cpp index 9e3b7786892b2c..05fb82380e6f02 100644 --- a/clang/lib/AST/ASTTypeTraits.cpp +++ b/clang/lib/AST/ASTTypeTraits.cpp @@ -32,6 +32,7 @@ const ASTNodeKind::KindInfo ASTNodeKind::AllKindInfo[] = { {NKI_None, "TemplateName"}, {NKI_None, "NestedNameSpecifier"}, {NKI_None, "NestedNameSpecifierLoc"}, + {NKI_None, "ObjCProtocolLoc"}, {NKI_None, "QualType"}, #define TYPELOC(CLASS, PARENT) {NKI_##PARENT, #CLASS "TypeLoc"}, #include "clang/AST/TypeLocNodes.def" @@ -54,7 +55,6 @@ const ASTNodeKind::KindInfo ASTNodeKind::AllKindInfo[] = { {NKI_None, "Attr"}, #define ATTR(A) {NKI_Attr, #A "Attr"}, #include "clang/Basic/AttrList.inc" - {NKI_None, "ObjCProtocolLoc"}, {NKI_None, "ConceptReference"}, {NKI_None, "OffsetOfNode"}, }; diff --git a/clang/unittests/AST/ASTContextParentMapTest.cpp b/clang/unittests/AST/ASTContextParentMapTest.cpp index 545eaf36191717..0e18c41e111901 100644 --- a/clang/unittests/AST/ASTContextParentMapTest.cpp +++ b/clang/unittests/AST/ASTContextParentMapTest.cpp @@ -49,6 +49,20 @@ TEST(GetParents, ReturnsParentForNestedNameSpecifierLoc) { nestedNameSpecifierLoc(hasParent(typeLoc())))); } +TEST(GetParents, ReturnsParentForObjCProtocolLoc) { + auto AST = tooling::buildASTFromCodeWithArgs( + "@protocol P @end @interface I <P> @end", {"-x", "objective-c"}); + ASTContext &Ctx = AST->getASTContext(); + auto Matches = match(objcInterfaceDecl(hasName("I")).bind("i"), Ctx); + ASSERT_EQ(Matches.size(), 1u); + const auto *I = Matches[0].getNodeAs<ObjCInterfaceDecl>("i"); + + ObjCProtocolLoc ProtocolLoc(*I->protocol_begin(), *I->protocol_loc_begin()); + auto Parents = Ctx.getParents(ProtocolLoc); + ASSERT_EQ(Parents.size(), 1u); + EXPECT_EQ(Parents[0].get<ObjCInterfaceDecl>(), I); +} + TEST(GetParents, ReturnsParentInsideTemplateInstantiations) { MatchVerifier<Decl> DeclVerifier; EXPECT_TRUE(DeclVerifier.match( diff --git a/clang/unittests/AST/ASTTypeTraitsTest.cpp b/clang/unittests/AST/ASTTypeTraitsTest.cpp index cb4adc9b8bf795..b444c4ebb7bb53 100644 --- a/clang/unittests/AST/ASTTypeTraitsTest.cpp +++ b/clang/unittests/AST/ASTTypeTraitsTest.cpp @@ -127,6 +127,7 @@ TEST(ASTNodeKind, ConstexprHasPointerIdentity) { EXPECT_FALSE(HasPointerIdentity<TypeLoc>); EXPECT_FALSE(HasPointerIdentity<QualType>); EXPECT_FALSE(HasPointerIdentity<NestedNameSpecifier>); + EXPECT_FALSE(HasPointerIdentity<ObjCProtocolLoc>); EXPECT_FALSE(HasPointerIdentity<Foo>); constexpr bool DefaultConstructedHasPointerIdentity = @@ -244,6 +245,44 @@ TEST(DynTypedNode, QualType) { EXPECT_FALSE(Node < Node); } +TEST(DynTypedNode, ObjCProtocolLoc) { + std::string code = R"cc( + @protocol P + @end + @interface I <P> + @end + @interface J <P> + @end + )cc"; + auto AST = + clang::tooling::buildASTFromCodeWithArgs(code, {"-x", "objective-c"}); + auto IMatches = + match(objcInterfaceDecl(hasName("I")).bind("i"), AST->getASTContext()); + auto JMatches = + match(objcInterfaceDecl(hasName("J")).bind("j"), AST->getASTContext()); + ASSERT_EQ(IMatches.size(), 1u); + ASSERT_EQ(JMatches.size(), 1u); + const auto *I = IMatches[0].getNodeAs<ObjCInterfaceDecl>("i"); + const auto *J = JMatches[0].getNodeAs<ObjCInterfaceDecl>("j"); + + // Both references name the same protocol at different locations. + ObjCProtocolLoc InI(*I->protocol_begin(), *I->protocol_loc_begin()); + ObjCProtocolLoc InJ(*J->protocol_begin(), *J->protocol_loc_begin()); + ASSERT_EQ(InI.getProtocol(), InJ.getProtocol()); + + DynTypedNode Node = DynTypedNode::create(InI); + DynTypedNode Copy = DynTypedNode::create(InI); + DynTypedNode Other = DynTypedNode::create(InJ); + EXPECT_EQ(Node.getMemoizationData(), nullptr); + EXPECT_TRUE(Node == Copy); + EXPECT_FALSE(Node < Copy); + EXPECT_FALSE(Copy < Node); + EXPECT_EQ(DynTypedNode::DenseMapInfo::getHashValue(Node), + DynTypedNode::DenseMapInfo::getHashValue(Copy)); + EXPECT_FALSE(Node == Other); + EXPECT_NE(Node < Other, Other < Node); +} + TEST(DynTypedNode, NestedNameSpecifier) { std::string code = R"cc(namespace a { struct b {}; } a::b x;)cc"; auto AST = clang::tooling::buildASTFromCode(code); >From e05a1c0e44829ba18997d3985b937963dfd075c7 Mon Sep 17 00:00:00 2001 From: Eric Li <[email protected]> Date: Mon, 2 Nov 2026 01:19:32 -0500 Subject: [PATCH 2/2] fixup! [clang][AST] Don't give `ObjCProtocolLoc` pointer identity in `DynTypedNode` --- clang/include/clang/AST/ASTTypeTraits.h | 6 ++---- clang/unittests/AST/ASTTypeTraitsTest.cpp | 17 +++++++++-------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/clang/include/clang/AST/ASTTypeTraits.h b/clang/include/clang/AST/ASTTypeTraits.h index db2c2d451df937..ac0479850dca83 100644 --- a/clang/include/clang/AST/ASTTypeTraits.h +++ b/clang/include/clang/AST/ASTTypeTraits.h @@ -350,10 +350,8 @@ class DynTypedNode { if (ASTNodeKind::getFromNodeKind<ObjCProtocolLoc>().isSame(NodeKind)) { auto PLA = getUnchecked<ObjCProtocolLoc>(); auto PLB = Other.getUnchecked<ObjCProtocolLoc>(); - return std::make_pair(PLA.getProtocol(), - PLA.getLocation().getRawEncoding()) < - std::make_pair(PLB.getProtocol(), - PLB.getLocation().getRawEncoding()); + return std::make_pair(PLA.getProtocol(), PLA.getLocation()) < + std::make_pair(PLB.getProtocol(), PLB.getLocation()); } assert(getMemoizationData() && Other.getMemoizationData()); diff --git a/clang/unittests/AST/ASTTypeTraitsTest.cpp b/clang/unittests/AST/ASTTypeTraitsTest.cpp index b444c4ebb7bb53..12477820235029 100644 --- a/clang/unittests/AST/ASTTypeTraitsTest.cpp +++ b/clang/unittests/AST/ASTTypeTraitsTest.cpp @@ -246,7 +246,7 @@ TEST(DynTypedNode, QualType) { } TEST(DynTypedNode, ObjCProtocolLoc) { - std::string code = R"cc( + static constexpr StringRef Code = R"cc( @protocol P @end @interface I <P> @@ -255,7 +255,7 @@ TEST(DynTypedNode, ObjCProtocolLoc) { @end )cc"; auto AST = - clang::tooling::buildASTFromCodeWithArgs(code, {"-x", "objective-c"}); + clang::tooling::buildASTFromCodeWithArgs(Code, {"-x", "objective-c"}); auto IMatches = match(objcInterfaceDecl(hasName("I")).bind("i"), AST->getASTContext()); auto JMatches = @@ -284,8 +284,9 @@ TEST(DynTypedNode, ObjCProtocolLoc) { } TEST(DynTypedNode, NestedNameSpecifier) { - std::string code = R"cc(namespace a { struct b {}; } a::b x;)cc"; - auto AST = clang::tooling::buildASTFromCode(code); + static constexpr StringRef Code = + R"cc(namespace a { struct b {}; } a::b x;)cc"; + auto AST = clang::tooling::buildASTFromCode(Code); auto matches = match(nestedNameSpecifier(specifiesNamespace(hasName("a"))).bind("nns"), AST->getASTContext()); @@ -307,8 +308,8 @@ TEST(DynTypedNode, NestedNameSpecifier) { } TEST(DynTypedNode, TypeLoc) { - std::string code = R"cc(void example() { int abc; })cc"; - auto AST = clang::tooling::buildASTFromCode(code); + static constexpr StringRef Code = R"cc(void example() { int abc; })cc"; + auto AST = clang::tooling::buildASTFromCode(Code); auto matches = match(traverse(TK_AsIs, varDecl(hasName("abc"), hasTypeLoc(typeLoc().bind("tl")))), @@ -322,8 +323,8 @@ TEST(DynTypedNode, TypeLoc) { } TEST(DynTypedNode, PointerTypeLoc) { - std::string code = R"cc(void example() { int *abc; })cc"; - auto AST = clang::tooling::buildASTFromCode(code); + static constexpr StringRef Code = R"cc(void example() { int *abc; })cc"; + auto AST = clang::tooling::buildASTFromCode(Code); auto matches = match(traverse(TK_AsIs, varDecl(hasName("abc"), hasTypeLoc(typeLoc().bind("ptl")))), _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
