[clang] fix RecursiveASTVisitor traversal from type to decl

[mizvekov]

For that visitor, it is not expected that a type can traverse into a declaration. This makes the MemberPointer visitor conform to that rule.

This turns the base class visitor into a CXXRecordType visitor, and only performs that visit in case it points to something different than the qualifier does.

Fixes a regression reported here: #132401 (comment)
As this fixes a regression which has not been released, there are no release notes.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Matheus Izvekov (mizvekov)

Changes

For that visitor, it is not expected that a type can traverse into a declaration. This makes the MemberPointer visitor conform to that rule.

This turns the base class visitor into a CXXRecordType visitor, and only performs that visit in case it points to something different than the qualifier does.

Fixes a regression reported here: #132401 (comment)
As this fixes a regression which has not been released, there are no release notes.

---

Full diff: https://github.com/llvm/llvm-project/pull/132551.diff

2 Files Affected:

• (modified) clang/include/clang/AST/RecursiveASTVisitor.h (+3-1)
• (modified) clang/test/SemaCXX/member-pointer.cpp (+11)

diff --git a/clang/include/clang/AST/RecursiveASTVisitor.h b/clang/include/clang/AST/RecursiveASTVisitor.h
index e93d1d8eab56f..0d5d515c0e6f7 100644
--- a/clang/include/clang/AST/RecursiveASTVisitor.h
+++ b/clang/include/clang/AST/RecursiveASTVisitor.h
@@ -1005,7 +1005,9 @@ DEF_TRAVERSE_TYPE(RValueReferenceType,
 
 DEF_TRAVERSE_TYPE(MemberPointerType, {
   TRY_TO(TraverseNestedNameSpecifier(T->getQualifier()));
-  TRY_TO(TraverseDecl(T->getMostRecentCXXRecordDecl()));
+  if (T->isSugared())
+    TRY_TO(TraverseType(
+        QualType(T->getMostRecentCXXRecordDecl()->getTypeForDecl(), 0)));
   TRY_TO(TraverseType(T->getPointeeType()));
 })
 
diff --git a/clang/test/SemaCXX/member-pointer.cpp b/clang/test/SemaCXX/member-pointer.cpp
index b6ab7d38610c8..3d9dd05755b8c 100644
--- a/clang/test/SemaCXX/member-pointer.cpp
+++ b/clang/test/SemaCXX/member-pointer.cpp
@@ -344,3 +344,14 @@ namespace GH132494 {
   };
   template struct A<E>; // expected-note {{requested here}}
 } // namespace GH132494
+
+namespace GH132401 {
+  template <typename Func> struct CallableHelper {
+    static auto Resolve() -> Func;
+  };
+  struct QIODevice {
+    void d_func() { (void)d_ptr; }
+    int d_ptr;
+  };
+  template struct CallableHelper<void (QIODevice::*)()>;
+} // namespace GH132401

[zyn0217]

I think it would be better to have a release note as it looks like a potential behavior change of ASTVisitor.

Otherwise feel free to merge to put out the fire.

[mizvekov]

It is a change but within the parameters of what the regressing commit already was. This is just changing what had already been changed there.

[bolshakov-a]

@mizvekov, could you clarify please what is the point of traversing the host type twice? The tests pass if just remove the if statement.

[mizvekov]

Which host type twice?

In case the MemberPointerType represents a class adjustment, this visits the actual class type, instead of just the as-written qualifier.

example: https://gcc.godbolt.org/z/M5qfGPGMf

I didn't expect we would have in-tree users of the RecursiveVisitor for this, but it seems useful for external users to leave it in, no?

[bolshakov-a]

Thanks, it becomes now clearer for me. However, I'm still not succeeded in making this code really working, because the default version of TraverseDecltypeType doesn't traverse the underlying desugared type:

llvm-project/clang/include/clang/AST/RecursiveASTVisitor.h

Lines 1101 to 1102 in be6ccc9

	DEF_TRAVERSE_TYPE(DecltypeType,
	{ TRY_TO(TraverseStmt(T->getUnderlyingExpr())); })

But, maybe, if someone provides his own version of TraverseDecltypeType in his visitor, then this may become useful...

My primary concern is that one may think that NestedNameSpecifier node is now always inserted between MemberPointerType and its class type node, but it may be not the case, and under very specific conditions.

By the way, it looks strange for me that int Typedef::* is not considered as a sugar.