[ObjC generics] Fix applying `__kindof` to the type parameter.

[vsapsai]

Summary

Fixes the warning about incompatible pointer types on assigning to a
subclass of type argument an expression of type __kindof TypeParam.

We already have a mechanism in ASTContext::canAssignObjCInterfaces
that handles ObjCObjectType with __kindof. But it wasn't triggered
because during type substitution __kindof TypeParam was represented as
AttributedType with attribute ObjCKindOf and equivalent type
TypeArg. For assignment type checking we use canonical types so
attributed type was desugared and the attribute was ignored.

The fix is in checking transformed AttributedType and pushing
__kindof down into ObjCObjectType when necessary.

rdar://problem/38514910

[jordan_rose]

I think this is reasonable but Doug was the one who worked on this. I wonder if it also helps with the test cases in rdar://problem/24619481.

[vsapsai]

In D57076#1368407, @jordan_rose wrote:

I think this is reasonable but Doug was the one who worked on this. I wonder if it also helps with the test cases in rdar://problem/24619481.

Yep, it helps with the test cases in rdar://problem/24619481. As soon as ObjCObjectType has IsKindOf, ASTContext::canAssignObjCInterfaces takes care of everything. It checks that LHS is a superclass of RHS or when RHS has __kindof that LHS is a subclass of RHS. My change doesn't deal with inheritance, so I didn't add those test cases.

[vsapsai]

• Add a test that __kindof works with type sugar such as typedef.
• Use early return to reduce nesting. Also it's consistent with the previous early returns.

[vsapsai]

Update diff to ignore parent patch.

[doug.gregor]

One minor request about regarding a dyn_cast, but this looks like the right approach. Thank you!

clang/lib/AST/Type.cpp

1291

Either this should be a cast<AttributedType> or the next if should check for !newAttrType. I suspect the latter is safer.

[vsapsai]

• Address review comment: don't crash when AttributedType is transformed to another type class.

[vsapsai]

clang/lib/AST/Type.cpp

1291

You are right. Thanks for the catch. Currently it shouldn't happen, as far as I can tell, so no test for the case when AttributedType is transformed to something else.

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;
-  });
+  QualType VisitAttributedType(const AttributedType *attrType) {
+    QualType newType = BaseType::VisitAttributedType(attrType);
+    if (newType.isNull())
+      return {};
+
+    const auto *newAttrType = dyn_cast<AttributedType>(newType.getTypePtr());
+    if (newAttrType->getAttrKind() != attr::ObjCKindOf)
+      return newType;
+
+    // Find out if it's an Objective-C object or object pointer type;
+    QualType newEquivType = newAttrType->getEquivalentType();
+    const ObjCObjectPointerType *ptrType =
+        newEquivType->getAs<ObjCObjectPointerType>();
+    const ObjCObjectType *objType = ptrType
+                                        ? ptrType->getObjectType()
+                                        : newEquivType->getAs<ObjCObjectType>();
+    if (!objType)
+      return newType;
+
+    // Rebuild the "equivalent" type, which pushes __kindof down into
+    // the object type.
+    newEquivType = Ctx.getObjCObjectType(
+        objType->getBaseType(), objType->getTypeArgsAsWritten(),
+        objType->getProtocols(),
+        // There is no need to apply kindof on an unqualified id type.
+        /*isKindOf=*/objType->isObjCUnqualifiedId() ? false : true);
+
+    // If we started with an object pointer type, rebuild it.
+    if (ptrType)
+      newEquivType = Ctx.getObjCObjectPointerType(newEquivType);
+
+    // Rebuild the attributed type.
+    return Ctx.getAttributedType(newAttrType->getAttrKind(),
+                                 newAttrType->getModifiedType(), newEquivType);
+  }
+};
+
+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/kindof.m

 @interface A2 : NSObject
 - (void)test:(__kindof id)T;
 @end
 @implementation A2
 - (void)test:(id)T {
 }
 @end
 
+// ---------------------------------------------------------------------------
+// __kindof on type parameters
+// ---------------------------------------------------------------------------
+
 @interface NSGeneric<ObjectType> : NSObject
 - (void)test:(__kindof ObjectType)T; // expected-note{{passing argument to parameter 'T' here}}
 - (void)mapUsingBlock:(id (^)(__kindof ObjectType))block;
+@property (copy) ObjectType object;
+@property (copy) __kindof ObjectType kindof_object;
+
+@property (copy) __kindof ObjectType _Nonnull nonnull_kindof_object;
 @end
 @implementation NSGeneric
 - (void)test:(id)T {
 }
 - (void)mapUsingBlock:(id (^)(id))block {
 }
 @end
 
+@interface NSDefaultGeneric<ObjectType : NSString *> : NSObject
+@property (copy) ObjectType object;
+@property (copy) __kindof ObjectType kindof_object;
+@end
+
 void testGeneric(NSGeneric<NSString*> *generic) {
   NSObject *NSObject_obj;
   // Assign from NSObject_obj to __kindof NSString*.
   [generic test:NSObject_obj]; // expected-warning{{incompatible pointer types sending 'NSObject *' to parameter of type '__kindof NSString *'}}
   NSString *NSString_str;
   [generic test:NSString_str];
 }
 
+void testGenericAssignment() {
+  NSMutableString *NSMutableString_str;
+  NSNumber *NSNumber_obj;
+
+  NSGeneric<NSString*> *generic;
+  NSMutableString_str = generic.object; // expected-warning{{incompatible pointer types}}
+  NSNumber_obj = generic.object; // expected-warning{{incompatible pointer types}}
+  NSMutableString_str = generic.kindof_object;
+  NSNumber_obj = generic.kindof_object; // expected-warning{{incompatible pointer types assigning to 'NSNumber *' from '__kindof NSString *'}}
+
+  NSGeneric<__kindof NSString*> *kindof_generic;
+  NSMutableString_str = kindof_generic.object;
+  NSNumber_obj = kindof_generic.object; // expected-warning{{incompatible pointer types assigning to 'NSNumber *' from '__kindof NSString *'}}
+  NSMutableString_str = kindof_generic.kindof_object;
+  NSNumber_obj = kindof_generic.kindof_object; // expected-warning{{incompatible pointer types assigning to 'NSNumber *' from '__kindof __kindof NSString *'}}
+
+  NSDefaultGeneric *default_generic;
+  NSMutableString_str = default_generic.object;
+  NSNumber_obj = default_generic.object; // expected-warning{{incompatible pointer types}}
+  NSMutableString_str = default_generic.kindof_object;
+  NSNumber_obj = default_generic.kindof_object; // expected-warning{{incompatible pointer types assigning to 'NSNumber *' from '__kindof __kindof NSString *'}}
+
+  typedef NSString *Typedef_NSString;
+  NSGeneric<Typedef_NSString> *typedef_generic;
+  NSMutableString_str = typedef_generic.object; // expected-warning{{incompatible pointer types}}
+  NSNumber_obj = typedef_generic.object; // expected-warning{{incompatible pointer types}}
+  NSMutableString_str = typedef_generic.kindof_object;
+  NSNumber_obj = typedef_generic.kindof_object; // expected-warning{{incompatible pointer types assigning to 'NSNumber *' from '__kindof Typedef_NSString'}}
+}
+
+void testKindofNonObjectType() {
+  typedef void (^BlockType)(int);
+  NSGeneric<BlockType> *generic;
+}
+
+void testKindofNullability(NSGeneric<NSString*> *generic) {
+  generic.nonnull_kindof_object = 0; // expected-warning{{null passed to a callee that requires a non-null argument}}
+}
+
 // Check that clang doesn't crash when a type parameter is illegal.
 @interface Array1<T> : NSObject
 @end
 
 @interface I1 : NSObject
 @end
 
 @interface Array1<__kindof I1*>(extensions1) // expected-error{{expected type parameter name}}
 @end
 
 @interface Array2<T1, T2, T3> : NSObject
 @end
 
 @interface Array2<T, T, __kindof I1*>(extensions2) // expected-error{{expected type parameter name}} expected-error{{redeclaration of type parameter 'T'}}
 @end