[ObjC generics] Fix not inheriting type bounds in categories/extensions.

[vsapsai]

Summary

When a category/extension doesn't repeat a type bound, corresponding
type parameter is substituted with id when used as a type argument. As
a result, in the added test case it was causing errors like

type argument 'T' (aka 'id') does not satisfy the bound ('id<NSCopying>') of type parameter 'T'

We are already checking that type parameters should be consistent
everywhere (see checkTypeParamListConsistency) and update
ObjCTypeParamDecl to have correct underlying type. And when we use the
type parameter as a method return type or a method parameter type, it is
substituted to the bounded type. But when we use the type parameter as a
type argument, we check ObjCTypeParamType that wasn't updated and
remains id.

Fix by updating not only ObjCTypeParamDecl UnderlyingType but also
TypeForDecl as we use the underlying type to create a canonical type for
ObjCTypeParamType (see ASTContext::getObjCTypeParamType).

This is a different approach to fixing the issue. The previous one was
02c2ab3d8872416589bd1a6ca3dfb96ba373a3b9 which was reverted in
4c539e8da1b3de38a53ef3f7497f5c45a3243b61. The problem with the previous
approach was that ObjCTypeParamType::desugar was returning underlying
type for ObjCTypeParamDecl without applying any protocols stored in
ObjCTypeParamType. It caused inconsistencies in comparing types before
and after desugaring.

rdar://problem/54329242

[merge_guards_bot]

Unit tests: pass. 61930 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[hans]

I don't have the context here. Was I added as a subscriber because it's related to the clang 10 release?

[vsapsai]

In D72872#1868989, @hans wrote:

I don't have the context here. Was I added as a subscriber because it's related to the clang 10 release?

It's not related to clang 10 release. I've added you because earlier you've found a problem with a previous approach https://github.com/llvm/llvm-project/commit/4c539e8da1b3de38a53ef3f7497f5c45a3243b61 So in case it breaks something else, you have extra visibility into the change.

[hans]

In D72872#1870256, @vsapsai wrote:

In D72872#1868989, @hans wrote:

I don't have the context here. Was I added as a subscriber because it's related to the clang 10 release?

It's not related to clang 10 release. I've added you because earlier you've found a problem with a previous approach https://github.com/llvm/llvm-project/commit/4c539e8da1b3de38a53ef3f7497f5c45a3243b61 So in case it breaks something else, you have extra visibility into the change.

Thanks! I applied the patch and was able to build large parts of Chromium without any problems.

[vsapsai]

In D72872#1872242, @hans wrote:

[...snip...]
Thanks! I applied the patch and was able to build large parts of Chromium without any problems.

Thanks for checking!

[vsapsai]

• Fix some clang-format checks. Ignore the check to add a space between a generic name and its type parameters as it's not the style the rest of the header uses and not what Apple SDK headers are using.

[vsapsai]

Ping.

[vsapsai]

Ping.

[vsapsai]

Ping.

[erik.pilkington]

LGTM, sorry for the delay in reviewing this.

[vsapsai]

Thanks for the review.

clang/include/clang/AST/ASTContext.h

public:
 
   QualType getObjCObjectType(QualType Base,
                              ArrayRef<QualType> typeArgs,
                              ArrayRef<ObjCProtocolDecl *> protocols,
                              bool isKindOf) const;
 
   QualType getObjCTypeParamType(const ObjCTypeParamDecl *Decl,
                                 ArrayRef<ObjCProtocolDecl *> protocols) const;
+  void adjustObjCTypeParamBoundType(const ObjCTypeParamDecl *Orig,
+                                    ObjCTypeParamDecl *New) const;
 
   bool ObjCObjectAdoptsQTypeProtocols(QualType QT, ObjCInterfaceDecl *Decl);
 
   /// QIdProtocolsAdoptObjCObjectProtocols - Checks that protocols in
   /// QT's qualified-id protocol list adopt all protocols in IDecl's list
   /// of protocols.
   bool QIdProtocolsAdoptObjCObjectProtocols(QualType QT,
                                             ObjCInterfaceDecl *IDecl);

clang/lib/AST/ASTContext.cpp

ASTContext::getObjCTypeParamType(const ObjCTypeParamDecl *Decl,
   void *mem = Allocate(size, TypeAlignment);
   auto *newType = new (mem) ObjCTypeParamType(Decl, Canonical, protocols);
 
   Types.push_back(newType);
   ObjCTypeParamTypes.InsertNode(newType, InsertPos);
   return QualType(newType, 0);
 }
 
+void ASTContext::adjustObjCTypeParamBoundType(const ObjCTypeParamDecl *Orig,
+                                              ObjCTypeParamDecl *New) const {
+  New->setTypeSourceInfo(getTrivialTypeSourceInfo(Orig->getUnderlyingType()));
+  // Update TypeForDecl after updating TypeSourceInfo.
+  auto NewTypeParamTy = cast<ObjCTypeParamType>(New->getTypeForDecl());
+  SmallVector<ObjCProtocolDecl *, 8> protocols;
+  protocols.append(NewTypeParamTy->qual_begin(), NewTypeParamTy->qual_end());
+  QualType UpdatedTy = getObjCTypeParamType(New, protocols);
+  New->setTypeForDecl(UpdatedTy.getTypePtr());
+}
+
 /// ObjCObjectAdoptsQTypeProtocols - Checks that protocols in IC's
 /// protocol list adopt all protocols in QT's qualified-id protocol
 /// list.
 bool ASTContext::ObjCObjectAdoptsQTypeProtocols(QualType QT,
                                                 ObjCInterfaceDecl *IC) {
   if (!QT->isObjCQualifiedIdType())
     return false;
 

clang/lib/AST/Type.cpp

Profile(ID, getBaseType(), getTypeArgsAsWritten(),
           llvm::makeArrayRef(qual_begin(), getNumProtocols()),
           isKindOfTypeAsWritten());
 }
 
 void ObjCTypeParamType::Profile(llvm::FoldingSetNodeID &ID,
                                 const ObjCTypeParamDecl *OTPDecl,
                                 ArrayRef<ObjCProtocolDecl *> protocols) {
   ID.AddPointer(OTPDecl);
+  ID.AddPointer(OTPDecl->getUnderlyingType().getAsOpaquePtr());
   ID.AddInteger(protocols.size());
   for (auto proto : protocols)
     ID.AddPointer(proto);
 }
 
 void ObjCTypeParamType::Profile(llvm::FoldingSetNodeID &ID) {
   Profile(ID, getDecl(),
           llvm::makeArrayRef(qual_begin(), getNumProtocols()));

clang/lib/Sema/SemaDeclObjC.cpp

if (newTypeParam->hasExplicitBound()) {
              prevTypeParam->getUnderlyingType().getAsString(
                S.Context.getPrintingPolicy()));
 
       S.Diag(prevTypeParam->getLocation(), diag::note_objc_type_param_here)
         << prevTypeParam->getDeclName();
 
       // Override the new type parameter's bound type with the previous type,
       // so that it's consistent.
-      newTypeParam->setTypeSourceInfo(
-        S.Context.getTrivialTypeSourceInfo(prevTypeParam->getUnderlyingType()));
+      S.Context.adjustObjCTypeParamBoundType(prevTypeParam, newTypeParam);
       continue;
     }
 
     // The new type parameter got the implicit bound of 'id'. That's okay for
     // categories and extensions (overwrite it later), but not for forward
     // declarations and @interfaces, because those must be standalone.
     if (newContext == TypeParamListContext::ForwardDeclaration ||
         newContext == TypeParamListContext::Definition) {
if (newContext == TypeParamListContext::ForwardDeclaration ||
         << (newContext == TypeParamListContext::ForwardDeclaration)
         << FixItHint::CreateInsertion(insertionLoc, newCode);
 
       S.Diag(prevTypeParam->getLocation(), diag::note_objc_type_param_here)
         << prevTypeParam->getDeclName();
     }
 
     // Update the new type parameter's bound to match the previous one.
-    newTypeParam->setTypeSourceInfo(
-      S.Context.getTrivialTypeSourceInfo(prevTypeParam->getUnderlyingType()));
+    S.Context.adjustObjCTypeParamBoundType(prevTypeParam, newTypeParam);
   }
 
   return false;
 }
 
 Decl *Sema::ActOnStartClassInterface(
     Scope *S, SourceLocation AtInterfaceLoc, IdentifierInfo *ClassName,
     SourceLocation ClassLoc, ObjCTypeParamList *typeParamList,

clang/test/SemaObjC/parameterized_classes_collection_literal.m

 + (NSNumber *)numberWithInt:(int)value;
 @end
 
 @interface NSArray<T> : NSObject <NSCopying>
 + (instancetype)arrayWithObjects:(const T [])objects count:(NSUInteger)cnt;
 @end
 
 @interface NSDictionary<K, V> : NSObject <NSCopying>
-+ (instancetype)dictionaryWithObjects:(const V [])objects forKeys:(const K [])keys count:(NSUInteger)cnt;
++ (instancetype)dictionaryWithObjects:(const V [])objects
+                              forKeys:(const K <NSCopying> [])keys
+                                count:(NSUInteger)cnt;
 @end
 
 void testArrayLiteral(void) {
   NSArray<NSString *> *array1 = @[@"hello",
                                    @1, // expected-warning{{of type 'NSNumber *' is not compatible with array element type 'NSString *'}}
                                   @"world",
                                   @[@1, @2]]; // expected-warning{{of type 'NSArray *' is not compatible with array element type 'NSString *'}}
 
   NSArray<NSArray<NSString *> *> *array2 = @[@[@"hello", @"world"],
                                               @"blah", // expected-warning{{object of type 'NSString *' is not compatible with array element type 'NSArray<NSString *> *'}}
                                              @[@1]]; // expected-warning{{object of type 'NSNumber *' is not compatible with array element type 'NSString *'}}
 }
 
 void testDictionaryLiteral(void) {
   NSDictionary<NSString *, NSNumber *> *dict1 = @{
     @"hello" : @17,
     @18 : @18, // expected-warning{{object of type 'NSNumber *' is not compatible with dictionary key type 'NSString *'}}
     @"world" : @"blah" // expected-warning{{object of type 'NSString *' is not compatible with dictionary value type 'NSNumber *'}}
   };
 }
+
+void testCastingInDictionaryLiteral(NSString *arg) {
+  NSDictionary *dict = @{
+    (id)arg : @"foo",
+  };
+}

clang/test/SemaObjC/parameterized_classes_subst.m

 - (int)test2:(id)key{ // expected-warning{{conflicting return type in implementation}}
   return 0;
 }
 - (void)mapUsingBlock:(id (^)(id))block {
 }
 - (void)mapUsingBlock2:(id)block { // expected-warning{{conflicting parameter types in implementation}}
 }
 @end
+
+// --------------------------------------------------------------------------
+// Use a type parameter as a type argument.
+// --------------------------------------------------------------------------
+// Type bounds in a category/extension are omitted. rdar://problem/54329242
+@interface ParameterizedContainer<T : id<NSCopying>>
+- (ParameterizedContainer<T> *)inInterface;
+@end
+@interface ParameterizedContainer<T> (Cat)
+- (ParameterizedContainer<T> *)inCategory;
+@end
+@interface ParameterizedContainer<U> ()
+- (ParameterizedContainer<U> *)inExtension;
+@end