This is an archive of the discontinued LLVM Phabricator instance.

[ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer types.
ClosedPublic

Authored by jyknight on Sep 24 2019, 1:58 PM.

Details

Summary

For example, in Objective-C mode, the initialization of 'x' in:

@implementation MyType
+ (void)someClassMethod {
  MyType *x = self;
}
@end

is correctly diagnosed with an incompatible-pointer-types warning, but
in Objective-C++ mode, it is not diagnosed at all -- even though
incompatible pointer conversions generally become an error in C++.

This patch fixes that oversight, allowing implicit conversions
involving Class only to/from unqualified-id, and between qualified and
unqualified Class, where the protocols are compatible.

Note that this does change some behaviors in Objective-C, as well, as
shown by the modified tests.

Of particular note is that assignment from from 'Class<MyProtocol>' to
'id<MyProtocol>' now warns. (Despite appearances, those are not
compatible types. 'Class<MyProtocol>' is not expected to have instance
methods defined by 'MyProtocol', while 'id<MyProtocol>' is.)

Diff Detail

Event Timeline

jyknight created this revision.Sep 24 2019, 1:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2019, 1:58 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Note that the test-case diffs are on top of https://reviews.llvm.org/D67982, which I split out to make the actual change in behavior in this commit clearer.

rjmccall accepted this revision.Sep 25 2019, 2:53 PM

LGTM.

This revision is now accepted and ready to land.Sep 25 2019, 2:53 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Oct 28 2019, 7:17 AM

After this, Class can no longer be used as a key type in an Obj-C dictionary literal. Is that intentional?

After this, Class can no longer be used as a key type in an Obj-C dictionary literal. Is that intentional?

Such code was already an on by default incompatible-pointer-types warning in ObjC mode. That it worked in ObjC++ was accidental.

For example:

foo.m:

#import <Foundation/Foundation.h>

int main() {
  NSDictionary* d = @{[NSArray class] : @"bar"};
}

Compiling:

$ clang -framework Foundation -o foo foo.m
foo.m:4:23: warning: incompatible pointer types passing 'Class' to parameter of type 'id<NSCopying> _Nonnull' [-Wincompatible-pointer-types]
  NSDictionary* d = @{[NSArray class] : @"bar"};
                      ^~~~~~~~~~~~~~~
1 warning generated.

While the default metaclass does in fact implement the one method NSCopying declares, it's not possible within the language to declare that the Class -- itself, as an instance -- implements the instance methods from the NSCopying protocol.

You can fix the code just by doing @{(id)clz : val}, since id is freely castable to anything.

While the default metaclass does in fact implement the one method NSCopying declares, it's not possible within the language to declare that the Class -- itself, as an instance -- implements the instance methods from the NSCopying protocol.

Should the compiler just know that then?

You can fix the code just by doing @{(id)clz : val}, since id is freely castable to anything.

We could probably do a quick check to see if the class informally conforms to the protocol. +copyWithZone seems to be marked unavailable in ARC; not sure if that would cause problems for such a check.

We could probably do a quick check to see if the class informally conforms to the protocol. +copyWithZone seems to be marked unavailable in ARC; not sure if that would cause problems for such a check.

What kind of check did you have in mind? We might hard-code the compiler to think that the "Class" type "implements" NSObject/NSCopying and thus is implicitly convertible to id<NSObject> and id<NSCopying>. That usually would be OK since the default metaclass in the normal runtime in fact does so. However, there's no such guarantee, and that kind of hardcoding seems generally kinda sketchy.

Given that this code was already being diagnosed for a long time in ObjC mode, I'm not sure that adding such a hack is really warranted. I'll add a bit to the release notes, though.

I was thinking that we could walk through the instance methods of the protocol and check whether the class has compatible declarations of class methods. But you're right, of course: we don't actually know what class we're working with, and we'd have to look through [NSFoo self] / [NSFoo class] just to cover the common case of a "direct" reference to a class, which would be a novel step in assuming things about standard methods. So the only way we could do this in general would be to just assume that all classes conform to certain special protocols.

Just doing nothing is fine with me.

I'll note that this change required several changes in our Obj-C++ codebase, and we don't have all that much Obj-C++ code in Chromium. Most of the changes made the code better, but I'd imagine that folks with more Obj-C++ code might need some amount of time to adopt their code. (Our changes: https://chromium-review.googlesource.com/c/chromium/src/+/1878075 https://chromium-review.googlesource.com/c/chromium/src/+/1884671 )

arphaman added a subscriber: arphaman.EditedFeb 6 2020, 5:34 PM

@jyknight @rjmccall I'm not sure this change is 100% fine. For example, the following code no longer compiles with ARC:

@protocol Delegate
@end

@interface X <Delegate>

@end

@interface Y
@property id<Delegate> d;
@end

@implementation X

+ (void)foo:(Y *)y with:(X*)x {
 y.d = self; // error: assigning to 'id<Delegate>' from incompatible type 'const Class'
 y.d = x;     // fine
}

@end

@jyknight @rjmccall I'm not sure this change is 100% fine. For example, the following code no longer compiles with ARC:

@protocol Delegate
@end

@interface X <Delegate>

@end

@interface Y
@property id<Delegate> d;
@end

@implementation X

+ (void)foo:(Y *)y with:(X*)x {
 y.d = self; // error: assigning to 'id<Delegate>' from incompatible type 'const Class'
 y.d = x;     // fine
}

@end

Your error looks correct to me -- "self" in a classmethod is not an instance, but the class itself. And while instances of X implement "Delegate", the Class does not.

@jyknight @rjmccall I'm not sure this change is 100% fine. For example, the following code no longer compiles with ARC:

@protocol Delegate
@end

@interface X <Delegate>

@end

@interface Y
@property id<Delegate> d;
@end

@implementation X

+ (void)foo:(Y *)y with:(X*)x {
 y.d = self; // error: assigning to 'id<Delegate>' from incompatible type 'const Class'
 y.d = x;     // fine
}

@end

Your error looks correct to me -- "self" in a classmethod is not an instance, but the class itself. And while instances of X implement "Delegate", the Class does not.

Got it, thanks! We might need to add a flag to allow the old behavior temporarily to accommodate our codebase while it's being updated.

Your error looks correct to me -- "self" in a classmethod is not an instance, but the class itself. And while instances of X implement "Delegate", the Class does not.

Got it, thanks! We might need to add a flag to allow the old behavior temporarily to accommodate our codebase while it's being updated.

Adding an explicit cast would be the simplest way to allow your code to continue being broken in the same way it was previously broken. E.g.

// TODO: fix the types here -- self is _not_ actually an id<Delegate>!
y.d = static_cast<id<Delegate>>(self);