[PATCH] D51189: [Sema][ObjC] Infer availability of +new from availability of -init
This revision was automatically updated to reflect the committed changes.
Closed by commit rL341874: [Sema][ObjC] Infer availability of +new from
availability of -init. (authored by epilk, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D51189?vs=164316=164758#toc
Repository:
rL LLVM
https://reviews.llvm.org/D51189
Files:
cfe/trunk/include/clang/AST/ASTContext.h
cfe/trunk/include/clang/AST/DeclObjC.h
cfe/trunk/include/clang/AST/NSAPI.h
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/AST/DeclObjC.cpp
cfe/trunk/lib/AST/NSAPI.cpp
cfe/trunk/lib/Sema/SemaDeclAttr.cpp
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/lib/Sema/SemaExprObjC.cpp
cfe/trunk/test/SemaObjC/infer-availability-from-init.m
Index: cfe/trunk/lib/AST/NSAPI.cpp
===
--- cfe/trunk/lib/AST/NSAPI.cpp
+++ cfe/trunk/lib/AST/NSAPI.cpp
@@ -607,3 +607,11 @@
}
return Sel;
}
+
+Selector NSAPI::getOrInitNullarySelector(StringRef Id, Selector ) const {
+ if (Sel.isNull()) {
+IdentifierInfo *Ident = (Id);
+Sel = Ctx.Selectors.getSelector(0, );
+ }
+ return Sel;
+}
Index: cfe/trunk/lib/AST/DeclObjC.cpp
===
--- cfe/trunk/lib/AST/DeclObjC.cpp
+++ cfe/trunk/lib/AST/DeclObjC.cpp
@@ -829,6 +829,14 @@
hasAttr();
}
+bool ObjCMethodDecl::definedInNSObject(const ASTContext ) const {
+ if (const auto *PD = dyn_cast(getDeclContext()))
+return PD->getIdentifier() == Ctx.getNSObjectName();
+ if (const auto *ID = dyn_cast(getDeclContext()))
+return ID->getIdentifier() == Ctx.getNSObjectName();
+ return false;
+}
+
bool ObjCMethodDecl::isDesignatedInitializerForTheInterface(
const ObjCMethodDecl **InitMethod) const {
if (getMethodFamily() != OMF_init)
Index: cfe/trunk/lib/Sema/SemaExpr.cpp
===
--- cfe/trunk/lib/Sema/SemaExpr.cpp
+++ cfe/trunk/lib/Sema/SemaExpr.cpp
@@ -206,7 +206,8 @@
bool Sema::DiagnoseUseOfDecl(NamedDecl *D, ArrayRef Locs,
const ObjCInterfaceDecl *UnknownObjCClass,
bool ObjCPropertyAccess,
- bool AvoidPartialAvailabilityChecks) {
+ bool AvoidPartialAvailabilityChecks,
+ ObjCInterfaceDecl *ClassReceiver) {
SourceLocation Loc = Locs.front();
if (getLangOpts().CPlusPlus && isa(D)) {
// If there were any diagnostics suppressed by template argument deduction,
@@ -292,7 +293,7 @@
}
DiagnoseAvailabilityOfDecl(D, Locs, UnknownObjCClass, ObjCPropertyAccess,
- AvoidPartialAvailabilityChecks);
+ AvoidPartialAvailabilityChecks, ClassReceiver);
DiagnoseUnusedOfDecl(*this, D, Loc);
Index: cfe/trunk/lib/Sema/SemaExprObjC.cpp
===
--- cfe/trunk/lib/Sema/SemaExprObjC.cpp
+++ cfe/trunk/lib/Sema/SemaExprObjC.cpp
@@ -2471,7 +2471,8 @@
if (!Method)
Method = Class->lookupPrivateClassMethod(Sel);
-if (Method && DiagnoseUseOfDecl(Method, SelectorSlotLocs))
+if (Method && DiagnoseUseOfDecl(Method, SelectorSlotLocs,
+nullptr, false, false, Class))
return ExprError();
}
@@ -2784,14 +2785,19 @@
} else {
if (ObjCMethodDecl *CurMeth = getCurMethodDecl()) {
if (ObjCInterfaceDecl *ClassDecl = CurMeth->getClassInterface()) {
+// FIXME: Is this correct? Why are we assuming that a message to
+// Class will call a method in the current interface?
+
// First check the public methods in the class interface.
Method = ClassDecl->lookupClassMethod(Sel);
if (!Method)
Method = ClassDecl->lookupPrivateClassMethod(Sel);
+
+if (Method && DiagnoseUseOfDecl(Method, SelectorSlotLocs, nullptr,
+false, false, ClassDecl))
+ return ExprError();
}
- if (Method && DiagnoseUseOfDecl(Method, SelectorSlotLocs))
-return ExprError();
}
if (!Method) {
// If not messaging 'self', look for any factory method named 'Sel'.
Index: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
===
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp
@@ -6967,8 +6967,12 @@
/// \param D The declaration to check.
/// \param Message If non-null, this will be populated with the message from
/// the availability attribute that is selected.
+/// \param ClassReceiver If we're checking the the method of a class message
+/// send, the class. Otherwise nullptr.
static std::pair
-ShouldDiagnoseAvailabilityOfDecl(const NamedDecl *D, std::string *Message) {
[PATCH] D51189: [Sema][ObjC] Infer availability of +new from availability of -init
arphaman accepted this revision. arphaman added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D51189 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51189: [Sema][ObjC] Infer availability of +new from availability of -init
erik.pilkington added a comment.
In https://reviews.llvm.org/D51189#1211910, @steven_wu wrote:
> 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.
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6951
+ std::string *Message,
+ ObjCInterfaceDecl *ClassMessageReceiver) {
AvailabilityResult Result = D->getAvailability(Message);
arphaman wrote:
> Please be consistent with the name, you are using `ClassMessageReceiver`,
> `ClassReceiver` and `Receiver` in different arguments in this patch.
Sure, sorry. I canonicalized on ClassReceiver.
https://reviews.llvm.org/D51189
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51189: [Sema][ObjC] Infer availability of +new from availability of -init
erik.pilkington updated this revision to Diff 164316.
erik.pilkington marked 4 inline comments as done.
erik.pilkington added a comment.
Address @arphaman's review comments.
https://reviews.llvm.org/D51189
Files:
clang/include/clang/AST/ASTContext.h
clang/include/clang/AST/DeclObjC.h
clang/include/clang/AST/NSAPI.h
clang/include/clang/Sema/Sema.h
clang/lib/AST/DeclObjC.cpp
clang/lib/AST/NSAPI.cpp
clang/lib/Sema/SemaDeclAttr.cpp
clang/lib/Sema/SemaExpr.cpp
clang/lib/Sema/SemaExprObjC.cpp
clang/test/SemaObjC/infer-availability-from-init.m
Index: clang/test/SemaObjC/infer-availability-from-init.m
===
--- /dev/null
+++ clang/test/SemaObjC/infer-availability-from-init.m
@@ -0,0 +1,58 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx-10.9 -Wunguarded-availability -fblocks -fsyntax-only -verify %s
+
+__attribute__((objc_root_class))
+@interface NSObject
++(instancetype)new;
+-(instancetype)init;
+@end
+
+@interface MyObject : NSObject
+-(instancetype)init __attribute__((unavailable)); // expected-note{{'init' has been explicitly marked unavailable here}}
+@end
+
+void usemyobject() {
+ [MyObject new]; // expected-error{{'new' is unavailable}}
+}
+
+@interface MyOtherObject : NSObject
++(instancetype)init __attribute__((unavailable));
++(instancetype)new;
+@end
+
+void usemyotherobject() {
+ [MyOtherObject new]; // no error; new is overrideen.
+}
+
+@interface NotGoodOverride : NSObject
++(instancetype)init __attribute__((unavailable));
+-(instancetype)new;
++(instancetype)new: (int)x;
+@end
+
+void usenotgoodoverride() {
+ [NotGoodOverride new]; // no error
+}
+
+@interface NotNSObject
++(instancetype)new;
+-(instancetype)init;
+@end
+
+@interface NotMyObject : NotNSObject
+-(instancetype)init __attribute__((unavailable));
+@end
+
+void usenotmyobject() {
+ [NotMyObject new]; // no error; this isn't NSObject
+}
+
+@interface FromSelf : NSObject
+-(instancetype)init __attribute__((unavailable)); // expected-note {{'init' has been explicitly marked unavailable here}}
++(FromSelf*)another_one;
+@end
+
+@implementation FromSelf
++(FromSelf*)another_one {
+ [self new]; // expected-error{{'new' is unavailable}}
+}
+@end
Index: clang/lib/Sema/SemaExprObjC.cpp
===
--- clang/lib/Sema/SemaExprObjC.cpp
+++ clang/lib/Sema/SemaExprObjC.cpp
@@ -2471,7 +2471,8 @@
if (!Method)
Method = Class->lookupPrivateClassMethod(Sel);
-if (Method && DiagnoseUseOfDecl(Method, SelectorSlotLocs))
+if (Method && DiagnoseUseOfDecl(Method, SelectorSlotLocs,
+nullptr, false, false, Class))
return ExprError();
}
@@ -2784,14 +2785,19 @@
} else {
if (ObjCMethodDecl *CurMeth = getCurMethodDecl()) {
if (ObjCInterfaceDecl *ClassDecl = CurMeth->getClassInterface()) {
+// FIXME: Is this correct? Why are we assuming that a message to
+// Class will call a method in the current interface?
+
// First check the public methods in the class interface.
Method = ClassDecl->lookupClassMethod(Sel);
if (!Method)
Method = ClassDecl->lookupPrivateClassMethod(Sel);
+
+if (Method && DiagnoseUseOfDecl(Method, SelectorSlotLocs, nullptr,
+false, false, ClassDecl))
+ return ExprError();
}
- if (Method && DiagnoseUseOfDecl(Method, SelectorSlotLocs))
-return ExprError();
}
if (!Method) {
// If not messaging 'self', look for any factory method named 'Sel'.
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -206,7 +206,8 @@
bool Sema::DiagnoseUseOfDecl(NamedDecl *D, ArrayRef Locs,
const ObjCInterfaceDecl *UnknownObjCClass,
bool ObjCPropertyAccess,
- bool AvoidPartialAvailabilityChecks) {
+ bool AvoidPartialAvailabilityChecks,
+ ObjCInterfaceDecl *ClassReceiver) {
SourceLocation Loc = Locs.front();
if (getLangOpts().CPlusPlus && isa(D)) {
// If there were any diagnostics suppressed by template argument deduction,
@@ -292,7 +293,7 @@
}
DiagnoseAvailabilityOfDecl(D, Locs, UnknownObjCClass, ObjCPropertyAccess,
- AvoidPartialAvailabilityChecks);
+ AvoidPartialAvailabilityChecks, ClassReceiver);
DiagnoseUnusedOfDecl(*this, D, Loc);
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6959,8 +6959,12 @@
/// \param D The declaration to check.
/// \param
[PATCH] D51189: [Sema][ObjC] Infer availability of +new from availability of -init
arphaman added a comment.
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?
Comment at: clang/lib/AST/DeclObjC.cpp:833
+bool ObjCMethodDecl::definedInNSObject(const ASTContext ) const {
+ if (const ObjCProtocolDecl *PD =
+ dyn_cast(getDeclContext()))
`const auto`
Comment at: clang/lib/AST/DeclObjC.cpp:836
+return PD->getIdentifier() == Ctx.getNSObjectName();
+ if (const ObjCInterfaceDecl *ID =
+ dyn_cast(getDeclContext())) {
`const auto`
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6951
+ std::string *Message,
+ ObjCInterfaceDecl *ClassMessageReceiver) {
AvailabilityResult Result = D->getAvailability(Message);
Please be consistent with the name, you are using `ClassMessageReceiver`,
`ClassReceiver` and `Receiver` in different arguments in this patch.
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6986
+ if (const auto *MD = dyn_cast(D)) {
+if (S.NSAPIObj != nullptr && ClassMessageReceiver != nullptr) {
+ ObjCMethodDecl *Init = ClassMessageReceiver->lookupInstanceMethod(
Comparisons to `nullptr` are redundant, you can just say `S.NSAPIObj &&
ClassMessageReceiver`. Same below.
Repository:
rC Clang
https://reviews.llvm.org/D51189
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51189: [Sema][ObjC] Infer availability of +new from availability of -init
steven_wu added a comment.
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.
Repository:
rC Clang
https://reviews.llvm.org/D51189
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51189: [Sema][ObjC] Infer availability of +new from availability of -init
arphaman added a subscriber: ributzka. arphaman added a comment. In https://reviews.llvm.org/D51189#1211763, @erik.pilkington wrote: > In https://reviews.llvm.org/D51189#1211754, @arphaman wrote: > > > 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 `NSObject`s `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. Repository: rC Clang https://reviews.llvm.org/D51189 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51189: [Sema][ObjC] Infer availability of +new from availability of -init
erik.pilkington added a comment. In https://reviews.llvm.org/D51189#1211754, @arphaman wrote: > 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 `NSObject`s `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? Repository: rC Clang https://reviews.llvm.org/D51189 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51189: [Sema][ObjC] Infer availability of +new from availability of -init
arphaman added a comment. 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 `NSObject`s `new`? Ideally we would an attribute on a particular `new` instead, but that might not work. Repository: rC Clang https://reviews.llvm.org/D51189 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51189: [Sema][ObjC] Infer availability of +new from availability of -init
erik.pilkington created this revision.
erik.pilkington added a reviewer: arphaman.
Herald added a subscriber: dexonsmith.
rdar://18335828
Thanks!
Erik
Repository:
rC Clang
https://reviews.llvm.org/D51189
Files:
clang/include/clang/AST/ASTContext.h
clang/include/clang/AST/DeclObjC.h
clang/include/clang/AST/NSAPI.h
clang/include/clang/Sema/Sema.h
clang/lib/AST/DeclObjC.cpp
clang/lib/AST/NSAPI.cpp
clang/lib/Sema/SemaDeclAttr.cpp
clang/lib/Sema/SemaExpr.cpp
clang/lib/Sema/SemaExprObjC.cpp
clang/test/SemaObjC/infer-availability-from-init.m
Index: clang/test/SemaObjC/infer-availability-from-init.m
===
--- /dev/null
+++ clang/test/SemaObjC/infer-availability-from-init.m
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx-10.9 -Wunguarded-availability -fblocks -fsyntax-only -verify %s
+
+__attribute__((objc_root_class))
+@interface NSObject
++(instancetype)new;
+-(instancetype)init;
+@end
+
+@interface MyObject : NSObject
+-(instancetype)init __attribute__((unavailable)); // expected-note{{'init' has been explicitly marked unavailable here}}
+@end
+
+void usemyobject() {
+ [MyObject new]; // expected-error{{'new' is unavailable}}
+}
+
+@interface MyOtherObject : NSObject
++(instancetype)init __attribute__((unavailable));
++(instancetype)new;
+@end
+
+void usemyotherobject() {
+ [MyOtherObject new]; // no error; new is overrideen.
+}
+
+@interface NotGoodOverride : NSObject
++(instancetype)init __attribute__((unavailable));
+-(instancetype)new;
++(instancetype)new: (int)x;
+@end
+
+void usenotgoodoverride() {
+ [NotGoodOverride new]; // no error
+}
+
+@interface NotNSObject
++(instancetype)new;
+-(instancetype)init;
+@end
+
+@interface NotMyObject : NotNSObject
+-(instancetype)init __attribute__((unavailable));
+@end
+
+void usenotmyobject() {
+ [NotMyObject new]; // no error; this isn't NSObject
+}
Index: clang/lib/Sema/SemaExprObjC.cpp
===
--- clang/lib/Sema/SemaExprObjC.cpp
+++ clang/lib/Sema/SemaExprObjC.cpp
@@ -2471,7 +2471,8 @@
if (!Method)
Method = Class->lookupPrivateClassMethod(Sel);
-if (Method && DiagnoseUseOfDecl(Method, SelectorSlotLocs))
+if (Method && DiagnoseUseOfDecl(Method, SelectorSlotLocs,
+nullptr, false, false, Class))
return ExprError();
}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -206,7 +206,8 @@
bool Sema::DiagnoseUseOfDecl(NamedDecl *D, ArrayRef Locs,
const ObjCInterfaceDecl *UnknownObjCClass,
bool ObjCPropertyAccess,
- bool AvoidPartialAvailabilityChecks) {
+ bool AvoidPartialAvailabilityChecks,
+ ObjCInterfaceDecl *CD) {
SourceLocation Loc = Locs.front();
if (getLangOpts().CPlusPlus && isa(D)) {
// If there were any diagnostics suppressed by template argument deduction,
@@ -292,7 +293,7 @@
}
DiagnoseAvailabilityOfDecl(D, Locs, UnknownObjCClass, ObjCPropertyAccess,
- AvoidPartialAvailabilityChecks);
+ AvoidPartialAvailabilityChecks, CD);
DiagnoseUnusedOfDecl(*this, D, Loc);
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6943,8 +6943,12 @@
/// \param D The declaration to check.
/// \param Message If non-null, this will be populated with the message from
/// the availability attribute that is selected.
+/// \param ClassMessageReceiver If we're checking the the method of a class
+/// message send, the class. Otherwise nullptr.
static std::pair
-ShouldDiagnoseAvailabilityOfDecl(const NamedDecl *D, std::string *Message) {
+ShouldDiagnoseAvailabilityOfDecl(Sema , const NamedDecl *D,
+ std::string *Message,
+ ObjCInterfaceDecl *ClassMessageReceiver) {
AvailabilityResult Result = D->getAvailability(Message);
// For typedefs, if the typedef declaration appears available look
@@ -6977,6 +6981,20 @@
}
}
+ // For +new, infer availability from -init.
+ if (const auto *MD = dyn_cast(D)) {
+if (S.NSAPIObj != nullptr && ClassMessageReceiver != nullptr) {
+ ObjCMethodDecl *Init = ClassMessageReceiver->lookupInstanceMethod(
+ S.NSAPIObj->getInitSelector());
+ if (Init != nullptr && Result == AR_Available && MD->isClassMethod() &&
+ MD->getSelector() == S.NSAPIObj->getNewSelector() &&
+ MD->definedInNSObject(S.getASTContext())) {
+Result = Init->getAvailability(Message);
+D = Init;
+ }
+}
+ }
+
return {Result, D};
}
@@
