[ObjC] For type substitution in generics use a regular recursive type visitor.

[vsapsai]

Summary

Switch to the inheritance-based visitor from the lambda-based visitor to
allow both preorder and postorder customizations during type
transformation. NFC intended.

[erik.pilkington]

clang/lib/AST/Type.cpp

1295

Does this works with type sugar? i.e. previously calling Ty->getAs<ObjCObjectType>() would have stripped through TypedefType, but its not obvious to me that this traversal is doing the right thing for that case.

[vsapsai]

clang/lib/AST/Type.cpp

1295

You are right, great catch. And I have a test case to confirm that.

I've started this refactoring to avoid copy-pasting

QualType modifiedType = recurse(T->getModifiedType());
if (modifiedType.isNull())
  return {};

QualType equivalentType = recurse(T->getEquivalentType());
if (equivalentType.isNull())
  return {};

if (modifiedType.getAsOpaquePtr()
      == T->getModifiedType().getAsOpaquePtr() &&
    equivalentType.getAsOpaquePtr()
      == T->getEquivalentType().getAsOpaquePtr())
  return QualType(T, 0);

and use BaseType::VisitAttributedType(attrType) instead. I think it is possible to achieve the previous behaviour with the traditional recursive visitor. But ideas that I have are pretty complicated and I don't think that's the right trade-off.

[vsapsai]

• Add a failing test case.

[vsapsai]

I've added a test case with typedef and think it should be emitting a warning. I.e., the behaviour with typedef should be the same as without it, modulo different pretty-printing in diagnostic. The interesting part is that this test is failing even without my change. I'll look into that and depending on the fix, I'll update this change accordingly.

[vsapsai]

clang/lib/AST/Type.cpp

1295

The test case I've added isn't really testing regressions in this refactoring, it uncovers already broken functionality. After some investigation turns out there is no simple test case that would be passing before this change and failing afterwards. It is so because we were going past type sugar only in stripObjCKindOfType which is called from sameObjCTypeArgs to compare type args and type args are canonicalized during construction, so there is no type sugar. We also have stripObjCKindOfTypeAndQuals which has different implementation.

My next step is to have a separate fix for TypedefTypeParam.

[vsapsai]

Rebase on top of https://reviews.llvm.org/D57270

[erik.pilkington]

LGTM too, I agree this should be NFC with the parent commit.

[vsapsai]

Thanks for the review, Erik.

clang/lib/AST/Type.cpp

const ObjCObjectPointerType *ObjCObjectPointerType::stripObjCKindOfTypeAndQuals(
                                const ASTContext &ctx) const {
   if (!isKindOfType() && qual_empty())
     return this;
 
   QualType obj = getObjectType()->stripObjCKindOfTypeAndQuals(ctx);
   return ctx.getObjCObjectPointerType(obj)->castAs<ObjCObjectPointerType>();
 }
 
-template<typename F>
-static QualType simpleTransform(ASTContext &ctx, QualType type, F &&f);
-
 namespace {
 
-/// Visitor used by simpleTransform() to perform the transformation.
-template<typename F>
-struct SimpleTransformVisitor
-         : public TypeVisitor<SimpleTransformVisitor<F>, QualType> {
+/// Visitor used to perform a simple type transformation that does not change
+/// the semantics of the type.
+template <typename Derived>
+struct SimpleTransformVisitor : public TypeVisitor<Derived, QualType> {
   ASTContext &Ctx;
-  F &&TheFunc;
 
   QualType recurse(QualType type) {
-    return simpleTransform(Ctx, type, std::move(TheFunc));
+    // Split out the qualifiers from the type.
+    SplitQualType splitType = type.split();
+
+    // Visit the type itself.
+    QualType result = static_cast<Derived *>(this)->Visit(splitType.Ty);
+    if (result.isNull())
+      return result;
+
+    // Reconstruct the transformed type by applying the local qualifiers
+    // from the split type.
+    return Ctx.getQualifiedType(result, splitType.Quals);
   }
 
 public:
-  SimpleTransformVisitor(ASTContext &ctx, F &&f)
-      : Ctx(ctx), TheFunc(std::move(f)) {}
+  explicit SimpleTransformVisitor(ASTContext &ctx) : Ctx(ctx) {}
 
   // None of the clients of this transformation can occur where
   // there are dependent types, so skip dependent types.
 #define TYPE(Class, Base)
 #define DEPENDENT_TYPE(Class, Base) \
   QualType Visit##Class##Type(const Class##Type *T) { return QualType(T, 0); }
 #include "clang/AST/TypeNodes.def"
 
if (valueType.getAsOpaquePtr()
       return QualType(T, 0);
 
     return Ctx.getAtomicType(valueType);
   }
 
 #undef TRIVIAL_TYPE_CLASS
 };
 
-} // namespace
-
-/// Perform a simple type transformation that does not change the
-/// semantics of the type.
-template<typename F>
-static QualType simpleTransform(ASTContext &ctx, QualType type, F &&f) {
-  // Transform the type. If it changed, return the transformed result.
-  QualType transformed = f(type);
-  if (transformed.getAsOpaquePtr() != type.getAsOpaquePtr())
-    return transformed;
-
-  // Split out the qualifiers from the type.
-  SplitQualType splitType = type.split();
-
-  // Visit the type itself.
-  SimpleTransformVisitor<F> visitor(ctx, std::forward<F>(f));
-  QualType result = visitor.Visit(splitType.Ty);
-  if (result.isNull())
-    return result;
-
-  // Reconstruct the transformed type by applying the local qualifiers
-  // from the split type.
-  return ctx.getQualifiedType(result, splitType.Quals);
-}
-
-/// Substitute the given type arguments for Objective-C type
-/// parameters within the given type, recursively.
-QualType QualType::substObjCTypeArgs(
-           ASTContext &ctx,
-           ArrayRef<QualType> typeArgs,
-           ObjCSubstitutionContext context) const {
-  return simpleTransform(ctx, *this,
-                         [&](QualType type) -> QualType {
-    SplitQualType splitType = type.split();
+struct SubstObjCTypeArgsVisitor
+    : public SimpleTransformVisitor<SubstObjCTypeArgsVisitor> {
+  using BaseType = SimpleTransformVisitor<SubstObjCTypeArgsVisitor>;
+
+  ArrayRef<QualType> TypeArgs;
+  ObjCSubstitutionContext SubstContext;
+
+  SubstObjCTypeArgsVisitor(ASTContext &ctx, ArrayRef<QualType> typeArgs,
+                           ObjCSubstitutionContext context)
+      : BaseType(ctx), TypeArgs(typeArgs), SubstContext(context) {}
 
+  QualType VisitObjCTypeParamType(const ObjCTypeParamType *OTPTy) {
     // Replace an Objective-C type parameter reference with the corresponding
     // type argument.
-    if (const auto *OTPTy = dyn_cast<ObjCTypeParamType>(splitType.Ty)) {
-      ObjCTypeParamDecl *typeParam = OTPTy->getDecl();
-      // If we have type arguments, use them.
-      if (!typeArgs.empty()) {
-        QualType argType = typeArgs[typeParam->getIndex()];
-        if (OTPTy->qual_empty())
-          return ctx.getQualifiedType(argType, splitType.Quals);
+    if (!TypeArgs.empty()) {
+      QualType argType = TypeArgs[typeParam->getIndex()];
+        return argType;
 
-        // Apply protocol lists if exists.
-        bool hasError;
-        SmallVector<ObjCProtocolDecl*, 8> protocolsVec;
-        protocolsVec.append(OTPTy->qual_begin(),
-                            OTPTy->qual_end());
-        ArrayRef<ObjCProtocolDecl *> protocolsToApply = protocolsVec;
-        QualType resultTy = ctx.applyObjCProtocolQualifiers(argType,
-            protocolsToApply, hasError, true/*allowOnPointerType*/);
-
-        return ctx.getQualifiedType(resultTy, splitType.Quals);
-      }
+      SmallVector<ObjCProtocolDecl *, 8> protocolsVec;
+      protocolsVec.append(OTPTy->qual_begin(), OTPTy->qual_end());
+      return Ctx.applyObjCProtocolQualifiers(
+          argType, protocolsToApply, hasError, true/*allowOnPointerType*/);
 
-      switch (context) {
-      case ObjCSubstitutionContext::Ordinary:
-      case ObjCSubstitutionContext::Parameter:
-      case ObjCSubstitutionContext::Superclass:
-        // Substitute the bound.
-        return ctx.getQualifiedType(typeParam->getUnderlyingType(),
-                                    splitType.Quals);
+    switch (SubstContext) {
+      return typeParam->getUnderlyingType();
 
-      case ObjCSubstitutionContext::Result:
-      case ObjCSubstitutionContext::Property: {
-        // Substitute the __kindof form of the underlying type.
-        const auto *objPtr = typeParam->getUnderlyingType()
-          ->castAs<ObjCObjectPointerType>();
+      const auto *objPtr =
+          typeParam->getUnderlyingType()->castAs<ObjCObjectPointerType>();
 
-        // __kindof types, id, and Class don't need an additional
-        // __kindof.
-        if (objPtr->isKindOfType() || objPtr->isObjCIdOrClassType())
-          return ctx.getQualifiedType(typeParam->getUnderlyingType(),
-                                      splitType.Quals);
+        return typeParam->getUnderlyingType();
 
-        // Add __kindof.
-        const auto *obj = objPtr->getObjectType();
-        QualType resultTy = ctx.getObjCObjectType(obj->getBaseType(),
-                                                  obj->getTypeArgsAsWritten(),
-                                                  obj->getProtocols(),
-                                                  /*isKindOf=*/true);
+      QualType resultTy = Ctx.getObjCObjectType(
+          obj->getBaseType(), obj->getTypeArgsAsWritten(), obj->getProtocols(),
 
-        // Rebuild object pointer type.
-        resultTy = ctx.getObjCObjectPointerType(resultTy);
-        return ctx.getQualifiedType(resultTy, splitType.Quals);
+      return Ctx.getObjCObjectPointerType(resultTy);
       }
-      }
-    }
 
-    // If we have a function type, update the context appropriately.
-    if (const auto *funcType = dyn_cast<FunctionType>(splitType.Ty)) {
-      // Substitute result type.
-      QualType returnType = funcType->getReturnType().substObjCTypeArgs(
-                              ctx,
-                              typeArgs,
-                              ObjCSubstitutionContext::Result);
-      if (returnType.isNull())
-        return {};
+  QualType VisitFunctionType(const FunctionType *funcType) {
+    // If we have a function type, update the substitution context
+    // appropriately.
+
+    //Substitute result type.
+        Ctx, TypeArgs, ObjCSubstitutionContext::Result);
 
-      // Handle non-prototyped functions, which only substitute into the result
-      // type.
-      if (isa<FunctionNoProtoType>(funcType)) {
-        // If the return type was unchanged, do nothing.
-        if (returnType.getAsOpaquePtr()
-              == funcType->getReturnType().getAsOpaquePtr())
-          return type;
+      if (returnType.getAsOpaquePtr() ==
+          funcType->getReturnType().getAsOpaquePtr())
+        return BaseType::VisitFunctionType(funcType);
 
-        // Otherwise, build a new type.
-        return ctx.getFunctionNoProtoType(returnType, funcType->getExtInfo());
-      }
+      return Ctx.getFunctionNoProtoType(returnType, funcType->getExtInfo());
 
-      const auto *funcProtoType = cast<FunctionProtoType>(funcType);
 
-      // Transform parameter types.
-      SmallVector<QualType, 4> paramTypes;
-      bool paramChanged = false;
-      for (auto paramType : funcProtoType->getParamTypes()) {
-        QualType newParamType = paramType.substObjCTypeArgs(
-                                  ctx,
-                                  typeArgs,
-                                  ObjCSubstitutionContext::Parameter);
-        if (newParamType.isNull())
-          return {};
+          Ctx, TypeArgs, ObjCSubstitutionContext::Parameter);
 
-        if (newParamType.getAsOpaquePtr() != paramType.getAsOpaquePtr())
-          paramChanged = true;
 
-        paramTypes.push_back(newParamType);
-      }
 
-      // Transform extended info.
-      FunctionProtoType::ExtProtoInfo info = funcProtoType->getExtProtoInfo();
-      bool exceptionChanged = false;
-      if (info.ExceptionSpec.Type == EST_Dynamic) {
-        SmallVector<QualType, 4> exceptionTypes;
-        for (auto exceptionType : info.ExceptionSpec.Exceptions) {
-          QualType newExceptionType = exceptionType.substObjCTypeArgs(
-                                        ctx,
-                                        typeArgs,
-                                        ObjCSubstitutionContext::Ordinary);
-          if (newExceptionType.isNull())
-            return {};
+            Ctx, TypeArgs, ObjCSubstitutionContext::Ordinary);
 
-          if (newExceptionType.getAsOpaquePtr()
-              != exceptionType.getAsOpaquePtr())
-            exceptionChanged = true;
+        if (newExceptionType.getAsOpaquePtr() != exceptionType.getAsOpaquePtr())
 
-          exceptionTypes.push_back(newExceptionType);
-        }
 
-        if (exceptionChanged) {
-          info.ExceptionSpec.Exceptions =
-              llvm::makeArrayRef(exceptionTypes).copy(ctx);
-        }
-      }
+            llvm::makeArrayRef(exceptionTypes).copy(Ctx);
 
-      if (returnType.getAsOpaquePtr()
-            == funcProtoType->getReturnType().getAsOpaquePtr() &&
-          !paramChanged && !exceptionChanged)
-        return type;
+    if (returnType.getAsOpaquePtr() ==
+            funcProtoType->getReturnType().getAsOpaquePtr() &&
+      return BaseType::VisitFunctionType(funcType);
 
-      return ctx.getFunctionType(returnType, paramTypes, info);
-    }
+    return Ctx.getFunctionType(returnType, paramTypes, info);
 
+  QualType VisitObjCObjectType(const ObjCObjectType *objcObjectType) {
     // Substitute into the type arguments of a specialized Objective-C object
     // type.
-    if (const auto *objcObjectType = dyn_cast<ObjCObjectType>(splitType.Ty)) {
-      if (objcObjectType->isSpecializedAsWritten()) {
-        SmallVector<QualType, 4> newTypeArgs;
-        bool anyChanged = false;
-        for (auto typeArg : objcObjectType->getTypeArgsAsWritten()) {
-          QualType newTypeArg = typeArg.substObjCTypeArgs(
-                                  ctx, typeArgs,
-                                  ObjCSubstitutionContext::Ordinary);
-          if (newTypeArg.isNull())
-            return {};
+            Ctx, TypeArgs, ObjCSubstitutionContext::Ordinary);
 
-          if (newTypeArg.getAsOpaquePtr() != typeArg.getAsOpaquePtr()) {
-            // If we're substituting based on an unspecialized context type,
-            // produce an unspecialized type.
-            ArrayRef<ObjCProtocolDecl *> protocols(
-                                           objcObjectType->qual_begin(),
-                                           objcObjectType->getNumProtocols());
-            if (typeArgs.empty() &&
-                context != ObjCSubstitutionContext::Superclass) {
-              return ctx.getObjCObjectType(
-                       objcObjectType->getBaseType(), {},
-                       protocols,
-                       objcObjectType->isKindOfTypeAsWritten());
-            }
+              objcObjectType->qual_begin(), objcObjectType->getNumProtocols());
+          if (TypeArgs.empty() &&
+              SubstContext != ObjCSubstitutionContext::Superclass) {
+            return Ctx.getObjCObjectType(
+                objcObjectType->getBaseType(), {}, protocols,
 
-            anyChanged = true;
-          }
 
-          newTypeArgs.push_back(newTypeArg);
-        }
 
-        if (anyChanged) {
-          ArrayRef<ObjCProtocolDecl *> protocols(
-                                         objcObjectType->qual_begin(),
-                                         objcObjectType->getNumProtocols());
-          return ctx.getObjCObjectType(objcObjectType->getBaseType(),
-                                       newTypeArgs, protocols,
-                                       objcObjectType->isKindOfTypeAsWritten());
-        }
-      }
+            objcObjectType->qual_begin(), objcObjectType->getNumProtocols());
+        return Ctx.getObjCObjectType(objcObjectType->getBaseType(), newTypeArgs,
+                                     protocols,
 
-      return type;
-    }
+    return BaseType::VisitObjCObjectType(objcObjectType);
+};
 
-    return type;
-  });
+struct StripObjCKindOfTypeVisitor
+    : public SimpleTransformVisitor<StripObjCKindOfTypeVisitor> {
+  using BaseType = SimpleTransformVisitor<StripObjCKindOfTypeVisitor>;
+
+  explicit StripObjCKindOfTypeVisitor(ASTContext &ctx) : BaseType(ctx) {}
+
+  QualType VisitObjCObjectType(const ObjCObjectType *objType) {
+    if (!objType->isKindOfType())
+      return BaseType::VisitObjCObjectType(objType);
+
+    QualType baseType = objType->getBaseType().stripObjCKindOfType(Ctx);
+    return Ctx.getObjCObjectType(baseType, objType->getTypeArgsAsWritten(),
+                                 objType->getProtocols(),
+                                 /*isKindOf=*/false);
+  }
+};
+
+} // namespace
+
+/// Substitute the given type arguments for Objective-C type
+/// parameters within the given type, recursively.
+QualType QualType::substObjCTypeArgs(ASTContext &ctx,
+                                     ArrayRef<QualType> typeArgs,
+                                     ObjCSubstitutionContext context) const {
+  SubstObjCTypeArgsVisitor visitor(ctx, typeArgs, context);
+  return visitor.recurse(*this);
 }
 
 QualType QualType::substObjCMemberType(QualType objectType,
                                        const DeclContext *dc,
                                        ObjCSubstitutionContext context) const {
   if (auto subs = objectType->getObjCSubstitutions(dc))
     return substObjCTypeArgs(dc->getParentASTContext(), *subs, context);
 
   return *this;
 }
 
 QualType QualType::stripObjCKindOfType(const ASTContext &constCtx) const {
   // FIXME: Because ASTContext::getAttributedType() is non-const.
   auto &ctx = const_cast<ASTContext &>(constCtx);
-  return simpleTransform(ctx, *this,
-           [&](QualType type) -> QualType {
-             SplitQualType splitType = type.split();
-             if (auto *objType = splitType.Ty->getAs<ObjCObjectType>()) {
-               if (!objType->isKindOfType())
-                 return type;
-
-               QualType baseType
-                 = objType->getBaseType().stripObjCKindOfType(ctx);
-               return ctx.getQualifiedType(
-                        ctx.getObjCObjectType(baseType,
-                                              objType->getTypeArgsAsWritten(),
-                                              objType->getProtocols(),
-                                              /*isKindOf=*/false),
-                        splitType.Quals);
-             }
-
-             return type;
-           });
+  StripObjCKindOfTypeVisitor visitor(ctx);
+  return visitor.recurse(*this);
 }
 
 QualType QualType::getAtomicUnqualifiedType() const {
   if (const auto AT = getTypePtr()->getAs<AtomicType>())
     return AT->getValueType().getUnqualifiedType();
   return getUnqualifiedType();
 }
 

clang/test/SemaObjC/parameterized_classes_subst.m

 @interface NSControl : NSView
 - (void)toggle;
 @end
 
 @interface NSViewController<ViewType : NSView *> : NSObject
 @property (nonatomic,retain) ViewType view;
 @end
 
+@interface TypedefTypeParam<T> : NSObject
+typedef T AliasT;
+- (void)test:(AliasT)object;
+// expected-note@-1 {{parameter 'object' here}}
+@end
+
 // --------------------------------------------------------------------------
 // Nullability
 // --------------------------------------------------------------------------
 typedef NSControl * _Nonnull Nonnull_NSControl;
 
 @interface NSNullableTest<ViewType : NSView *> : NSObject
 - (ViewType)view;
 - (nullable ViewType)maybeView;

 
 void test_message_send_param(
        NSMutableSet<NSString *> *mutStringSet,
        WidgetSet *widgetSet,
        UntypedMutableSet *untypedMutSet,
        MutableSetOfArrays<NSString *> *mutStringArraySet,
        NSMutableSet *mutSet,
        MutableSetOfArrays *mutArraySet,
+       TypedefTypeParam<NSString *> *typedefTypeParam,
        void (^block)(void)) {
   Window *window;
 
   [mutStringSet addObject: window]; // expected-warning{{parameter of type 'NSString *'}}
   [widgetSet addObject: window]; // expected-warning{{parameter of type 'Widget *'}}
   [untypedMutSet addObject: window]; // expected-warning{{parameter of incompatible type 'id<NSCopying>'}}
   [mutStringArraySet addObject: window]; // expected-warning{{parameter of type 'NSArray<NSString *> *'}}
   [mutSet addObject: window]; // expected-warning{{parameter of incompatible type 'id<NSCopying>'}}
   [mutArraySet addObject: window]; // expected-warning{{parameter of incompatible type 'id<NSCopying>'}}
+  [typedefTypeParam test: window]; // expected-warning{{parameter of type 'NSString *'}}
   [block addObject: window]; // expected-warning{{parameter of incompatible type 'id<NSCopying>'}}
 }
 
 // --------------------------------------------------------------------------
 // Property accesses.
 // --------------------------------------------------------------------------
 void test_property_read(
        NSSet<NSString *> *stringSet,