Sema: allow comparison between blocks & block-compatible objc types

[DHowett-MSFT]

Summary

This commit makes valid the following code:

// objective-c++
#define nil ((id)nullptr)
...
void (^f)() = ^{};
if (f == nil) {
}
…

Where it would previously fail with the error invalid operands to binary expression ('void (^)()' and 'id').

Comparisons are now allowed between block types and id, id<NSCopying>, id<NSObject>, and NSObject*. No other comparisons are changed.

[lebedev.ri]

Why not test/SemaObjC/block_compare.mm ?

[DHowett-MSFT]

In D44580#1040540, @lebedev.ri wrote:

Why not test/SemaObjC/block_compare.mm ?

I wasn't aware that it existed. It may even be a good fit for test/SemaObjC/block-type-safety.m, which already exists.

[DHowett-MSFT]

Moved files around slightly.

[theraven]

We seem to be missing tests for the assignment part of this patch.

lib/Sema/SemaExpr.cpp

7749

Do we want to allow implicit casts for all object types to block types for assignment, or only for null pointers? We definitely want to allow nil to be assigned to a block type, but I would lean slightly to requiring an implicit cast.

Ideally, I think we'd allow this but warn, because casting from an arbitrary ObjC type to a block incorrectly can cause exciting security vulnerabilities if it's done incorrectly and we should encourage people to check these casts (nil is always safe though - as long as somewhere else checks the nullability attributes).

[DHowett-MSFT]

lib/Sema/SemaExpr.cpp

7749

I don't actually have a compelling use case for widening assignability here. I'm dropping this part of the patch.

I do think it should be valid, perhaps with a warning. It feels incorrect for there to be valid comparison cases that are not valid assignment cases (here), but that position doesn't hold up under scrutiny.

[DHowett-MSFT]

Backed out changes to block type assignment.

[DHowett-MSFT]

Ran clang-format over changed lines. Reuploaded diff with full context.

[rjmccall]

Okay, LGTM with the reduced set of changes to the functionality.

[DHowett-MSFT]

Thank you for the review! I don't have the means to check this in myself.

[rjmccall]

Committed as r329508.

lib/Sema/SemaExpr.cpp

if (isa<BlockPointerType>(LHSType)) {
     }
 
     // int or null -> T^
     if (RHSType->isIntegerType()) {
       Kind = CK_IntegralToPointer; // FIXME: null
       return IntToBlockPointer;
     }
 
-    // id -> T^
-    if (getLangOpts().ObjC1 && RHSType->isObjCIdType()) {
+    // id (or strictly compatible object type) -> T^
+    if (getLangOpts().ObjC1 && RHSType->isBlockCompatibleObjCPointerType(Context)) {
       Kind = CK_AnyPointerToBlockPointerCast;
       return Compatible;
     }
 
     // void* -> T^
     if (const PointerType *RHSPT = RHSType->getAs<PointerType>())
       if (RHSPT->getPointeeType()->isVoidType()) {
         Kind = CK_AnyPointerToBlockPointerCast;
if (LHSType->isObjCObjectPointerType() &&
         diagnoseObjCLiteralComparison(*this, Loc, LHS, RHS, Opc);
 
       if (LHSIsNull && !RHSIsNull)
         LHS = ImpCastExprToType(LHS.get(), RHSType, CK_BitCast);
       else
         RHS = ImpCastExprToType(RHS.get(), LHSType, CK_BitCast);
       return ResultTy;
     }
+
+    if(!IsRelational &&
+       LHSType->isBlockPointerType() &&
+       RHSType->isBlockCompatibleObjCPointerType(Context)) {
+      LHS = ImpCastExprToType(LHS.get(), RHSType, CK_BlockPointerToObjCPointerCast);
+      return ResultTy;
+    } else if(!IsRelational &&
+              LHSType->isBlockCompatibleObjCPointerType(Context) &&
+              RHSType->isBlockPointerType()) {
+      RHS = ImpCastExprToType(RHS.get(), LHSType, CK_BlockPointerToObjCPointerCast);
+      return ResultTy;
+    }
   }
   if ((LHSType->isAnyPointerType() && RHSType->isIntegerType()) ||
       (LHSType->isIntegerType() && RHSType->isAnyPointerType())) {
     unsigned DiagID = 0;
     bool isError = false;
     if (LangOpts.DebuggerSupport) {
       // Under a debugger, allow the comparison of pointers to integers,
       // since users tend to want to compare addresses.

test/Sema/objc_block_compare.mm

+// RUN: %clang_cc1 -S -o - -triple i686-windows -verify -fblocks \
+// RUN:     -Wno-unused-comparison %s
+
+#pragma clang diagnostic ignored "-Wunused-comparison"
+
+#define nil ((id)nullptr)
+
+@protocol NSObject
+@end
+
+@protocol NSCopying
+@end
+
+@protocol OtherProtocol
+@end
+
+__attribute__((objc_root_class))
+@interface NSObject <NSObject, NSCopying>
+@end
+
+__attribute__((objc_root_class))
+@interface Test
+@end
+
+int main() {
+  void (^block)() = ^{};
+  NSObject *object;
+  id<NSObject, NSCopying> qualifiedId;
+
+  id<OtherProtocol> poorlyQualified1;
+  Test *objectOfWrongType;
+
+  block == nil;
+  block == object;
+  block == qualifiedId;
+
+  nil == block;
+  object == block;
+  qualifiedId == block;
+
+  // these are still not valid: blocks must be compared with id, NSObject*, or a protocol-qualified id
+  // conforming to NSCopying or NSObject.
+
+  block == poorlyQualified1; // expected-error {{invalid operands to binary expression ('void (^)()' and 'id<OtherProtocol>')}}
+  block == objectOfWrongType; // expected-error {{invalid operands to binary expression ('void (^)()' and 'Test *')}}
+
+  poorlyQualified1 == block; // expected-error {{invalid operands to binary expression ('id<OtherProtocol>' and 'void (^)()')}}
+  objectOfWrongType == block; // expected-error {{invalid operands to binary expression ('Test *' and 'void (^)()')}}
+
+  return 0;
+}