[ObjC] Fix type checking for qualified id block parameters.

[vsapsai]

Summary

When checking if block types are compatible, we are checking for
compatibility their return types and parameters' types. As these types
have different variance, we need to check them in different order.

rdar://problem/52788423

[vsapsai]

clang/test/SemaObjC/block-type-safety.m

141 ↗

(On Diff #217500)

This is an existing error, just added a test for it.

148–149 ↗

(On Diff #217500)

And this is a new error. Earlier we would emit an error on blockWithParam = genericBlockWithParam assignment instead.

[arphaman]

LGTM

[vsapsai]

Thanks for the fast review.

[thakis]

We're getting a bunch of errors looking like ../../AppsListViewController.m:11:37: error: incompatible block pointer types assigning to 'void (^)(__strong id<AppCellProtocol>)' from 'void (^)(AppCollectionViewCell *__strong)' on code that looks fairly harmless to me. It looks something like this:

@protocol AppCellProtocol <NSObject> ... @end
@interface AppCollectionViewCell : NSObject <AppCellProtocol> ...@enderby 

@interface Cell : NSObject
@property(nonatomic, copy) void (^buttonPressed)(id<AppCellProtocol> cell);
@end

@implementation Bar
- (void) f {
  __weak __typeof(self) weakSelf = self;
  cell.buttonPressed = ^(AppCollectionViewCell *pressedCell) {
    // ...
  };
}
@end

The code doesn't say __strong anywhere as far as I can tell; it looks like regular protocol code.

Is this expected?

[vsapsai]

In D66831#1650015, @thakis wrote:

We're getting a bunch of errors looking like ../../AppsListViewController.m:11:37: error: incompatible block pointer types assigning to 'void (^)(__strong id<AppCellProtocol>)' from 'void (^)(AppCollectionViewCell *__strong)' on code that looks fairly harmless to me. It looks something like this:

@protocol AppCellProtocol <NSObject> ... @end
@interface AppCollectionViewCell : NSObject <AppCellProtocol> ...@enderby 

@interface Cell : NSObject
@property(nonatomic, copy) void (^buttonPressed)(id<AppCellProtocol> cell);
@end

@implementation Bar
- (void) f {
  __weak __typeof(self) weakSelf = self;
  cell.buttonPressed = ^(AppCollectionViewCell *pressedCell) {
    // ...
  };
}
@end

The code doesn't say __strong anywhere as far as I can tell; it looks like regular protocol code.

Is this expected?

That's the intention of the change. When you call void (^buttonPressed)(id<AppCellProtocol> cell), it is only enforced that the argument conforms to AppCellProtocol. But you can do

cell.buttonPressed = ^(AppCollectionViewCell *pressedCell) {
  // call some AppCollectionViewCell method not present in AppCellProtocol
};

and get unrecognized selector error. The change should help to avoid such situations.

Does my reasoning look correct to you?

[thakis]

Yes, that makes sense, thanks.

[smeenai]

Sorry for the late feedback here – we're in the process of testing with Clang 10 and noticed a behavior change caused by this commit.

Consider the following piece of code:

@protocol P
@end

@protocol Q
@end

@interface I <P>
@end

@interface J : I <Q>
@end

void f(void (^)(id<P>));
void g() {
  f(^(J *j){});
}

Clang 9 would accept it, whereas Clang 10 complains about incompatible block pointer types. If I change the declaration of J to instead be:

@interface J : I

Both Clang 9 and Clang 10 accept it.

I'm not very familiar with Objective-C semantics. Does saying @interface J : I <Q> mean we're overriding the conformance being specified by @interface I <P>? In that case, the Clang 10 error makes sense to me.

In practice though, classes like UIImage are declared as inheriting from NSObject<NSSecureCoding>, so passing a UIImage * to a block with a parameter of type id<NSObject> gives an error with Clang 10.

[vsapsai]

In D66831#1901972, @smeenai wrote:

I'm not very familiar with Objective-C semantics. Does saying @interface J : I <Q> mean we're overriding the conformance being specified by @interface I <P>? In that case, the Clang 10 error makes sense to me.

@interface J : I <Q> isn't overriding the conformance specified by @interface I <P>, it adds conformance to Q.

In D66831#1901972, @smeenai wrote:

In practice though, classes like UIImage are declared as inheriting from NSObject<NSSecureCoding>, so passing a UIImage * to a block with a parameter of type id<NSObject> gives an error with Clang 10.

I didn't try your example in a debugger but I believe we should warn here because code invoking the block is not guaranteed to call it with UIImage *, only with id<NSObject>. And using UIImage * inside the block is dangerous. To expand on your example

@protocol P
@end

@protocol Q
- (void)specificMethod;
@end

@interface I <P>
@end

@interface J : I <Q>
- (void)specificMethod;
@end

void f(void (^block)(id<P>)) {
  I *i = [I new];
  block(i);
}

void g() {
  f(^(J *j){
    [j specificMethod];  // not recognized selector
  });
}

According to this explanation @interface J : I should also cause errors but I think it works because assigning to/from id<Protocol> is somewhat hand-wavy and not very strict.

Also consider specifying protocols the other way around

@protocol P
@end

@protocol Q
@end

@interface I <P>
@end

@interface J : I <Q>
@end

void f(void (^)(J *));
void g() {
  f(^(id<P> p){});
  f(^(id<Q> q){});
}

I think it shouldn't be an error but clang-9 complains.

[smeenai]

Thanks for that explanation; that makes sense. Do you think that the @interface J : I would also ideally be an error then?

[vsapsai]

In D66831#1902176, @smeenai wrote:

Thanks for that explanation; that makes sense. Do you think that the @interface J : I would also ideally be an error then?

Theoretically, I think @interface J : I should also trigger an error (or at least a warning) because in a block we can call J-specific methods that aren't declared in any protocols. And that can potentially lead to not recognized selector.

Practically, if I remember correctly, that is done on purpose to make id-with-protocols more like id that is compatible with anything. Here my memory gets really hazy but it is possible there is common and idiomatic code relying on less strict type checking. But don't take my word for granted, I might be mistaken. If you are interested, the relevant code is in ASTContext::ObjCQualifiedIdTypesAreCompatible. And for the case like

void test(id<P> p) {
  J *j = p;
}

the block with protocol checking can be particularly interesting.

[• chatwyn]

Hi:
we're in the process of testing with Clang 10 and noticed a behavior change caused by this commit.

Consider the following piece of code:

@protocol P
@end

@protocol Q
@end

@interface I <P,Q>
@end

void a() {
    void (^genericBlockWithParam)(id <P>);
    void (^blockWithParam)(I *);
    blockWithParam = genericBlockWithParam;
    genericBlockWithParam = blockWithParam;
}

when I use clang 9, it will be below error

incompatible block pointer types assigning to 'void (^__strong)(I *__strong)' from 'void (^__strong)(__strong id<P>)'
    blockWithParam = genericBlockWithParam;
                   ^ ~~~~~~~~~~~~~~~~~~~~~

but in clang 10, the error is that

incompatible block pointer types assigning to 'void (^__strong)(__strong id<P>)' from 'void (^__strong)(I *__strong)'
genericBlockWithParam = blockWithParam;
                   ^ ~~~~~~~~~~~~~~~~~~~~~

I think clang 9 is reasonable.

[vsapsai]

There should be no error for blockWithParam = genericBlockWithParam; because blockWithParam is called with I * and it is safe to substitute genericBlockWithParam. Basically you have

I *blockArg;
id<P> genericParam = blockArg;

And for genericBlockWithParam = blockWithParam; you have

id<P> blockArg;
I *blockParam = blockArg;
// Or as a concrete example
// id<NSCopying> blockArg_thatHappensToBeNumber = @42;
// NSString *blockParam = blockArg_thatHappensToBeNumber;

It is not safe to make such assignments, that's why there is an error now.

[jyknight]

Hi, it seems that this change is incompatible with the [NSItemProvider loadItemForTypeIdentifier:options:completionHandler:] method's expected (but extremely weird and unusual!) usage.

Per https://developer.apple.com/documentation/foundation/nsitemprovider/1403900-loaditemfortypeidentifier?language=objc it takes an argument of type NSItemProviderCompletionHandler aka void (^)(id<NSSecureCoding> item, NSError *error);. However, loadItemForTypeIdentifier:options:completionHandler expects users to provide blocks of other types, e.g. ^(NSURL *url, NSError *error) { ... } and introspects at runtime the resulting block object to determine the declared signature, and then provides the expected type.

This is, of course, an extremely weird API (who'd ever expect a function to do runtime introspection of the callback's function signature and change the types passed to the function accordingly?). But, the expected usage previously type-checked, because of the bug fixed in this change (NSURL* is a subtype of id<NSSecureCoding>, so it was accepted before), and now throws an error.

Should this change be reverted? Or have an exemption added for that one weird API? Or maybe that API needs to be modified to say it takes an argument of type "id" instead of NSItemProviderCompletionHandler?

[vsapsai]

Agree that is a mistake in NSItemProvider API. The solution I offer is not to revert the change but to add cc1 flag -fcompatibility-qualified-id-block-type-checking and to make the driver for Darwin platforms to add this flag. Thus developers using Apple SDKs will see the old behavior and won't need to change their code. While everybody else will use clang with correct type checking. If any other platforms provide APIs relying on the old type checking, the solution for them is to tweak the driver.

The right NSItemProvider fix is to use __kindof, like void (^)(__kindof id<NSSecureCoding> item, NSError *error); It's purpose is to flip substitution principle, so that's the way to express API requirements in the type system. When we have a fix available in SDK, we can change the driver not to add the compatibility flag anymore and NSItemProvider should keep working without developers changing their code.

I'm going to work on the patch to add the compatibility flag, if you have any scenarios not covered by the offered solution, please let me know.

[jyknight]

In D66831#2019125, @vsapsai wrote:

Agree that is a mistake in NSItemProvider API. The solution I offer is not to revert the change but to add cc1 flag -fcompatibility-qualified-id-block-type-checking and to make the driver for Darwin platforms to add this flag. Thus developers using Apple SDKs will see the old behavior and won't need to change their code. While everybody else will use clang with correct type checking. If any other platforms provide APIs relying on the old type checking, the solution for them is to tweak the driver.

The right NSItemProvider fix is to use __kindof, like void (^)(__kindof id<NSSecureCoding> item, NSError *error); It's purpose is to flip substitution principle, so that's the way to express API requirements in the type system. When we have a fix available in SDK, we can change the driver not to add the compatibility flag anymore and NSItemProvider should keep working without developers changing their code.

I'm going to work on the patch to add the compatibility flag, if you have any scenarios not covered by the offered solution, please let me know.

Sounds like a fine plan.

I'm not sure whether there's anyone using objective-c blocks other than with the Apple SDKs on Darwin platforms, so your proposal seems basically equivalent to just entirely disabling the check, temporarily. But that's ok.

[vsapsai]

Patch for the suggested compatibility flag is available at https://reviews.llvm.org/D79511

cfe/trunk/lib/AST/ASTContext.cpp

if (RHSOPT->isObjCBuiltinType() || LHSOPT->isObjCIdType())
     return true;
 
   if (LHSOPT->isObjCBuiltinType()) {
     return finish(RHSOPT->isObjCBuiltinType() ||
                   RHSOPT->isObjCQualifiedIdType());
   }
 
   if (LHSOPT->isObjCQualifiedIdType() || RHSOPT->isObjCQualifiedIdType())
-    return finish(ObjCQualifiedIdTypesAreCompatible(QualType(LHSOPT,0),
-                                                    QualType(RHSOPT,0),
-                                                    false));
+    return finish(ObjCQualifiedIdTypesAreCompatible(
+        QualType(BlockReturnType ? LHSOPT : RHSOPT, 0),
+        QualType(BlockReturnType ? RHSOPT : LHSOPT, 0), false));
 
   const ObjCInterfaceType* LHS = LHSOPT->getInterfaceType();
   const ObjCInterfaceType* RHS = RHSOPT->getInterfaceType();
   if (LHS && RHS)  { // We have 2 user-defined types.
     if (LHS != RHS) {
       if (LHS->getDecl()->isSuperClassOf(RHS->getDecl()))
         return finish(BlockReturnType);
       if (RHS->getDecl()->isSuperClassOf(LHS->getDecl()))

cfe/trunk/test/SemaObjC/block-type-safety.m

 
 @interface NSAllArray <NSCopying>
 @end
 
 @interface NSAllArray (FooConformance) <Foo>
 @end
 
 int test5() {
+    // Returned value is used outside of a block, so error on changing
+    // a return type to a more general than expected.
     NSAllArray *(^block)(id);
     id <Foo> (^genericBlock)(id);
     genericBlock = block;
+    block = genericBlock; // expected-error {{incompatible block pointer types assigning to 'NSAllArray *(^)(id)' from 'id<Foo> (^)(id)'}}
+
+    // A parameter is used inside a block, so error on changing a parameter type
+    // to a more specific than an argument type it will be called with.
+    // rdar://problem/52788423
+    void (^blockWithParam)(NSAllArray *);
+    void (^genericBlockWithParam)(id<Foo>);
+    genericBlockWithParam = blockWithParam; // expected-error {{incompatible block pointer types assigning to 'void (^)(id<Foo>)' from 'void (^)(NSAllArray *)'}}
+    blockWithParam = genericBlockWithParam;
     return 0;
 }
 
 // rdar://10798770
 typedef int NSInteger;
 
 typedef enum : NSInteger {NSOrderedAscending = -1L, NSOrderedSame, NSOrderedDescending} NSComparisonResult;