[clang] preserve class type sugar when taking pointer to member

This changes the MemberPointerType representation to use
a NestedNameSpecifier instead of a Type to represent the base
class.

Since the qualifiers are always parsed as nested names, there
was an impedance mismatch when converting these back and
forth into types, and this led to issues in preserving
sugar.

The nested names are indeed a better match for these,
as the differences which a QualType can represent cannot be
expressed syntatically, and they represent the use case more
exactly, being either dependent or referring to a CXXRecord,
unqualified.

This patch also makes the MemberPointerType able to represent
sugar for a {up/downcast}cast conversion of the base class,
although for now the underlying type is canonical, as preserving
the sugar up to that point requires further work.

As usual, includes a few drive-by fixes in order to make use
of the improvements.

Author: mizvekov

clang-tools-extra/clang-tidy/bugprone/ComparePointerToMemberVirtualFunctionCheck.cpp

@@ -70,10 +70,7 @@ void ComparePointerToMemberVirtualFunctionCheck::check(
   // compare with variable which type is pointer to member function.
   llvm::SmallVector<SourceLocation, 12U> SameSignatureVirtualMethods{};
   const auto *MPT = cast<MemberPointerType>(DRE->getType().getCanonicalType());
-  const Type *T = MPT->getClass();
-  if (T == nullptr)
-    return;
-  const CXXRecordDecl *RD = T->getAsCXXRecordDecl();
+  const CXXRecordDecl *RD = MPT->getMostRecentCXXRecordDecl();
   if (RD == nullptr)
     return;
 

clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp

@@ -493,8 +493,7 @@ UseNullptrCheck::UseNullptrCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       NullMacrosStr(Options.get("NullMacros", "NULL")),
       IgnoredTypes(utils::options::parseStringList(Options.get(
-          "IgnoredTypes",
-          "std::_CmpUnspecifiedParam::;^std::__cmp_cat::__unspec"))) {
+          "IgnoredTypes", "_CmpUnspecifiedParam;^std::__cmp_cat::__unspec"))) {
   StringRef(NullMacrosStr).split(NullMacros, ",");
 }
 

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

@@ -178,7 +178,9 @@ bool isFunctionPointerConvertible(QualType From, QualType To) {
 
     // Note: converting Derived::* to Base::* is a different kind of conversion,
     // called Pointer-to-member conversion.
-    return FromMember->getClass() == ToMember->getClass() &&
+    return FromMember->getQualifier() == ToMember->getQualifier() &&
+           FromMember->getMostRecentCXXRecordDecl() ==
+               ToMember->getMostRecentCXXRecordDecl() &&
            FromMember->getPointeeType() == ToMember->getPointeeType();
   }
 
@@ -219,8 +221,8 @@ bool isQualificationConvertiblePointer(QualType From, QualType To,
 
     if (P1->isMemberPointerType())
       return P2->isMemberPointerType() &&
-             P1->getAs<MemberPointerType>()->getClass() ==
-                 P2->getAs<MemberPointerType>()->getClass();
+             P1->getAs<MemberPointerType>()->getMostRecentCXXRecordDecl() ==
+                 P2->getAs<MemberPointerType>()->getMostRecentCXXRecordDecl();
 
     if (P1->isConstantArrayType())
       return P2->isConstantArrayType() &&

clang-tools-extra/clangd/unittests/FindTargetTests.cpp

@@ -1489,7 +1489,7 @@ TEST_F(FindExplicitReferencesTest, AllRefsInFoo) {
         "4: targets = {a}\n"
         "5: targets = {a::b}, qualifier = 'a::'\n"
         "6: targets = {a::b::S}\n"
-        "7: targets = {a::b::S::type}, qualifier = 'struct S::'\n"
+        "7: targets = {a::b::S::type}, qualifier = 'S::'\n"
         "8: targets = {y}, decl\n"},
        {R"cpp(
          void foo() {

clang/docs/ReleaseNotes.rst

@@ -161,8 +161,8 @@ related warnings within the method body.
   ``__attribute__((model("large")))`` on non-TLS globals in x86-64 compilations.
   This forces the global to be considered small or large in regards to the
   x86-64 code model, regardless of the code model specified for the compilation.
-- Clang now emits a warning ``-Wreserved-init-priority`` instead of a hard error 
-  when ``__attribute__((init_priority(n)))`` is used with values of n in the 
+- Clang now emits a warning ``-Wreserved-init-priority`` instead of a hard error
+  when ``__attribute__((init_priority(n)))`` is used with values of n in the
   reserved range [0, 100]. The warning will be treated as an error by default.
 
 - There is a new ``format_matches`` attribute to complement the existing
@@ -233,6 +233,7 @@ Improvements to Clang's diagnostics
   under the subgroup ``-Wunsafe-buffer-usage-in-libc-call``.
 - Diagnostics on chained comparisons (``a < b < c``) are now an error by default. This can be disabled with
   ``-Wno-error=parentheses``.
+- Better preservation of member pointer type sugar.
 - The ``-Wshift-bool`` warning has been added to warn about shifting a boolean. (#GH28334)
 
 - The :doc:`ThreadSafetyAnalysis` now supports ``-Wthread-safety-pointer``,

clang/include/clang/AST/ASTContext.h

@@ -1558,10 +1558,9 @@ class ASTContext : public RefCountedBase<ASTContext> {
   QualType getRValueReferenceType(QualType T) const;
 
   /// Return the uniqued reference to the type for a member pointer to
-  /// the specified type in the specified class.
-  ///
-  /// The class \p Cls is a \c Type because it could be a dependent name.
-  QualType getMemberPointerType(QualType T, const Type *Cls) const;
+  /// the specified type in the specified nested name.
+  QualType getMemberPointerType(QualType T, NestedNameSpecifier *Qualifier,
+                                const CXXRecordDecl *Cls) const;
 
   /// Return a non-unique reference to the type for a variable array of
   /// the specified element type.

clang/include/clang/AST/ASTNodeTraverser.h

@@ -393,7 +393,9 @@ class ASTNodeTraverser
     Visit(T->getPointeeType());
   }
   void VisitMemberPointerType(const MemberPointerType *T) {
-    Visit(T->getClass());
+    // FIXME: Provide a NestedNameSpecifier visitor.
+    Visit(T->getQualifier()->getAsType());
+    Visit(T->getMostRecentCXXRecordDecl());
     Visit(T->getPointeeType());
   }
   void VisitArrayType(const ArrayType *T) { Visit(T->getElementType()); }
@@ -485,7 +487,8 @@ class ASTNodeTraverser
     }
   }
   void VisitMemberPointerTypeLoc(MemberPointerTypeLoc TL) {
-    Visit(TL.getClassTInfo()->getTypeLoc());
+    // FIXME: Provide NestedNamespecifierLoc visitor.
+    Visit(TL.getQualifierLoc().getTypeLoc());
   }
   void VisitVariableArrayTypeLoc(VariableArrayTypeLoc TL) {
     Visit(TL.getSizeExpr());

clang/include/clang/AST/CanonicalType.h

@@ -453,7 +453,9 @@ template<>
 struct CanProxyAdaptor<MemberPointerType>
   : public CanProxyBase<MemberPointerType> {
   LLVM_CLANG_CANPROXY_TYPE_ACCESSOR(getPointeeType)
-  LLVM_CLANG_CANPROXY_SIMPLE_ACCESSOR(const Type *, getClass)
+  LLVM_CLANG_CANPROXY_SIMPLE_ACCESSOR(NestedNameSpecifier *, getQualifier)
+  LLVM_CLANG_CANPROXY_SIMPLE_ACCESSOR(const CXXRecordDecl *,
+                                      getMostRecentCXXRecordDecl)
 };
 
 // CanProxyAdaptors for arrays are intentionally unimplemented because

clang/include/clang/AST/RecursiveASTVisitor.h

@@ -1004,7 +1004,8 @@ DEF_TRAVERSE_TYPE(RValueReferenceType,
                   { TRY_TO(TraverseType(T->getPointeeType())); })
 
 DEF_TRAVERSE_TYPE(MemberPointerType, {
-  TRY_TO(TraverseType(QualType(T->getClass(), 0)));
+  TRY_TO(TraverseNestedNameSpecifier(T->getQualifier()));
+  TRY_TO(TraverseDecl(T->getMostRecentCXXRecordDecl()));
   TRY_TO(TraverseType(T->getPointeeType()));
 })
 
@@ -1269,10 +1270,10 @@ DEF_TRAVERSE_TYPELOC(RValueReferenceType,
 // We traverse this in the type case as well, but how is it not reached through
 // the pointee type?
 DEF_TRAVERSE_TYPELOC(MemberPointerType, {
-  if (auto *TSI = TL.getClassTInfo())
-    TRY_TO(TraverseTypeLoc(TSI->getTypeLoc()));
+  if (NestedNameSpecifierLoc QL = TL.getQualifierLoc())
+    TRY_TO(TraverseNestedNameSpecifierLoc(QL));
   else
-    TRY_TO(TraverseType(QualType(TL.getTypePtr()->getClass(), 0)));
+    TRY_TO(TraverseNestedNameSpecifier(TL.getTypePtr()->getQualifier()));
   TRY_TO(TraverseTypeLoc(TL.getPointeeLoc()));
 })
 

clang/include/clang/AST/Type.h

@@ -3524,14 +3524,16 @@ class MemberPointerType : public Type, public llvm::FoldingSetNode {
   QualType PointeeType;
 
   /// The class of which the pointee is a member. Must ultimately be a
-  /// RecordType, but could be a typedef or a template parameter too.
-  const Type *Class;
+  /// CXXRecordType, but could be a typedef or a template parameter too.
+  NestedNameSpecifier *Qualifier;
 
-  MemberPointerType(QualType Pointee, const Type *Cls, QualType CanonicalPtr)
+  MemberPointerType(QualType Pointee, NestedNameSpecifier *Qualifier,
+                    QualType CanonicalPtr)
       : Type(MemberPointer, CanonicalPtr,
-             (Cls->getDependence() & ~TypeDependence::VariablyModified) |
+             (toTypeDependence(Qualifier->getDependence()) &
+              ~TypeDependence::VariablyModified) |
                  Pointee->getDependence()),
-        PointeeType(Pointee), Class(Cls) {}
+        PointeeType(Pointee), Qualifier(Qualifier) {}
 
 public:
   QualType getPointeeType() const { return PointeeType; }
@@ -3548,21 +3550,21 @@ class MemberPointerType : public Type, public llvm::FoldingSetNode {
     return !PointeeType->isFunctionProtoType();
   }
 
-  const Type *getClass() const { return Class; }
+  NestedNameSpecifier *getQualifier() const { return Qualifier; }
   CXXRecordDecl *getMostRecentCXXRecordDecl() const;
 
-  bool isSugared() const { return false; }
-  QualType desugar() const { return QualType(this, 0); }
+  bool isSugared() const;
+  QualType desugar() const {
+    return isSugared() ? getCanonicalTypeInternal() : QualType(this, 0);
+  }
 
   void Profile(llvm::FoldingSetNodeID &ID) {
-    Profile(ID, getPointeeType(), getClass());
+    Profile(ID, getPointeeType(), getQualifier(), getMostRecentCXXRecordDecl());
   }
 
   static void Profile(llvm::FoldingSetNodeID &ID, QualType Pointee,
-                      const Type *Class) {
-    ID.AddPointer(Pointee.getAsOpaquePtr());
-    ID.AddPointer(Class);
-  }
+                      const NestedNameSpecifier *Qualifier,
+                      const CXXRecordDecl *Cls);
 
   static bool classof(const Type *T) {
     return T->getTypeClass() == MemberPointer;