Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

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

Authored by DHowett-MSFT on Mar 16 2018, 1:27 PM.



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.

Diff Detail

rC Clang

Event Timeline

DHowett-MSFT created this revision.Mar 16 2018, 1:27 PM

Why not test/SemaObjC/ ?

DHowett-MSFT added a subscriber: lebedev.ri.EditedMar 16 2018, 1:38 PM

Why not test/SemaObjC/ ?

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 removed a subscriber: lebedev.ri.

Moved files around slightly.

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


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 planned changes to this revision.Mar 18 2018, 3:08 PM
DHowett-MSFT marked an inline comment as done.
DHowett-MSFT added inline comments.

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 marked an inline comment as done.

Backed out changes to block type assignment.

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

rjmccall accepted this revision.Apr 3 2018, 9:25 PM

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

This revision is now accepted and ready to land.Apr 3 2018, 9:25 PM

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

rjmccall closed this revision.Apr 7 2018, 10:45 AM

Committed as r329508.