This is an archive of the discontinued LLVM Phabricator instance.

[Sema][ObjC] Infer availability of +new from availability of -init
ClosedPublic

Authored by erik.pilkington on Aug 23 2018, 3:27 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Hmm, I don't think this solution is ideal, we'd rather have an attribute somewhere for other consumers of availability annotations. Does MyObject have an implicit decl of new, or are we referring to NSObjects new? Ideally we would an attribute on a particular new instead, but that might not work.

Hmm, I don't think this solution is ideal, we'd rather have an attribute somewhere for other consumers of availability annotations. Does MyObject have an implicit decl of new, or are we referring to NSObjects new? Ideally we would an attribute on a particular new instead, but that might not work.

We're referring to NSObject's new. I don't think it's unreasonable to ask users who override init to be unavailable also override new with the same annotation, but it seems like extra boilerplate for something that we can easily infer in clang. What other consumers are you concerned about?

Hmm, I don't think this solution is ideal, we'd rather have an attribute somewhere for other consumers of availability annotations. Does MyObject have an implicit decl of new, or are we referring to NSObjects new? Ideally we would an attribute on a particular new instead, but that might not work.

We're referring to NSObject's new. I don't think it's unreasonable to ask users who override init to be unavailable also override new with the same annotation, but it seems like extra boilerplate for something that we can easily infer in clang. What other consumers are you concerned about?

+ @ributzka
One consumer is TAPI. It looks at the declarations present in the header file, so it won't be able to reason about the availability of new with the current implementation. We could potentially implicitly declare unavailable new in the interface if init is unavailable, but that wouldn't really too work with class categories (since new might be explicitly declared there). Maybe it's worth it though.

TAPI mostly cares about linkable symbols, so this shouldn't be a problem.

I feel like this is a much tricky situation than just new and init. Following example is the same situation.

__attribute__((objc_root_class))
@interface NSObject
- (void) foo;
- (void) bar;
@end

@implementation NSObject
- (void) foo {}
- (void) bar { [self foo]; }
@end

@interface MyObject : NSObject
- (void) foo __attribute__((unavailable));
@end

void test(MyObject *obj) {
  [obj bar];
}

We can do something about [NSObject new] because we know it's implementation but we have to live with more general cases.

That's probably the best solution then, I don't think declaring implicit new just for availability attribute is sound.

Does this work with self new as well?

clang/lib/AST/DeclObjC.cpp
833 ↗(On Diff #162274)

const auto

836 ↗(On Diff #162274)

const auto

clang/lib/Sema/SemaDeclAttr.cpp
6951 ↗(On Diff #162274)

Please be consistent with the name, you are using ClassMessageReceiver, ClassReceiver and Receiver in different arguments in this patch.

6986 ↗(On Diff #162274)

Comparisons to nullptr are redundant, you can just say S.NSAPIObj && ClassMessageReceiver. Same below.

erik.pilkington marked 4 inline comments as done.

Address @arphaman's review comments.

I feel like this is a much tricky situation than just new and init. Following example is the same situation.

__attribute__((objc_root_class))
@interface NSObject
- (void) foo;
- (void) bar;
@end

@implementation NSObject
- (void) foo {}
- (void) bar { [self foo]; }
@end

@interface MyObject : NSObject
- (void) foo __attribute__((unavailable));
@end

void test(MyObject *obj) {
  [obj bar];
}

We can do something about [NSObject new] because we know it's implementation but we have to live with more general cases.

I agree that the general case is impossible to properly diagnose, but I think its totally reasonable for us to special case this pattern with NSObject.

clang/lib/Sema/SemaDeclAttr.cpp
6951 ↗(On Diff #162274)

Sure, sorry. I canonicalized on ClassReceiver.

This revision is now accepted and ready to land.Sep 10 2018, 1:49 PM
This revision was automatically updated to reflect the committed changes.