[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 updated this revision to Diff 161684.
QF5690 added a comment.
Using `isObjCARCImplicitlyUnretainedType` instead of `isObjCClassType`
Tests in `property-in-class-extension-1.m` relaxed to
`-Wproperty-attribute-mismatch` (was `-Weverything`)
Repository:
rC Clang
https://reviews.llvm.org/D44539
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaObjCProperty.cpp
test/SemaObjC/property-assign-on-object-type.m
test/SemaObjC/property-in-class-extension-1.m
Index: test/SemaObjC/property-in-class-extension-1.m
===
--- test/SemaObjC/property-in-class-extension-1.m
+++ test/SemaObjC/property-in-class-extension-1.m
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-darwin11
-fobjc-runtime-has-weak -fobjc-weak -verify -Weverything %s
-// RUN: %clang_cc1 -x objective-c++ -triple x86_64-apple-darwin11
-fobjc-runtime-has-weak -fobjc-weak -fsyntax-only -verify -Weverything %s
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-darwin11
-fobjc-runtime-has-weak -fobjc-weak -verify -Wproperty-attribute-mismatch %s
+// RUN: %clang_cc1 -x objective-c++ -triple x86_64-apple-darwin11
-fobjc-runtime-has-weak -fobjc-weak -fsyntax-only -verify
-Wproperty-attribute-mismatch %s
// rdar://12103400
@class NSString;
Index: test/SemaObjC/property-assign-on-object-type.m
===
--- /dev/null
+++ test/SemaObjC/property-assign-on-object-type.m
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wobjc-property-assign-on-object-type
%s
+
+@interface Foo @end
+@protocol Prot @end
+
+@interface Bar
+@property(assign, readonly) Foo* o1; // expected-warning {{'assign' property
of object type may become a dangling reference; consider using
'unsafe_unretained'}}
+@property(unsafe_unretained, readonly) Foo* o2;
+
+@property(assign) Class classProperty;
+@property(assign) Class classWithProtocolProperty;
+@property(assign) int s1;
+@property(assign) int* s2;
+@end
+
+@interface Bar ()
+@property(readwrite) Foo* o1;
+@property(readwrite) Foo* o2;
+@end
Index: lib/Sema/SemaObjCProperty.cpp
===
--- lib/Sema/SemaObjCProperty.cpp
+++ lib/Sema/SemaObjCProperty.cpp
@@ -2547,6 +2547,14 @@
PropertyDecl->setInvalidDecl();
}
+ // Check for assign on object types.
+ if ((Attributes & ObjCDeclSpec::DQ_PR_assign) &&
+ !(Attributes & ObjCDeclSpec::DQ_PR_unsafe_unretained) &&
+ PropertyTy->isObjCRetainableType() &&
+ !PropertyTy->isObjCARCImplicitlyUnretainedType()) {
+Diag(Loc, diag::warn_objc_property_assign_on_object);
+ }
+
// Check for more than one of { assign, copy, retain }.
if (Attributes & ObjCDeclSpec::DQ_PR_assign) {
if (Attributes & ObjCDeclSpec::DQ_PR_copy) {
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -1014,6 +1014,9 @@
"property attributes '%0' and '%1' are mutually exclusive">;
def err_objc_property_requires_object : Error<
"property with '%0' attribute must be of object type">;
+def warn_objc_property_assign_on_object : Warning<
+ "'assign' property of object type may become a dangling reference; consider
using 'unsafe_unretained'">,
+ InGroup, DefaultIgnore;
def warn_objc_property_no_assignment_attribute : Warning<
"no 'assign', 'retain', or 'copy' attribute is specified - "
"'assign' is assumed">,
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -367,6 +367,7 @@
def BadFunctionCast : DiagGroup<"bad-function-cast">;
def ObjCPropertyImpl : DiagGroup<"objc-property-implementation">;
def ObjCPropertyNoAttribute : DiagGroup<"objc-property-no-attribute">;
+def ObjCPropertyAssignOnObjectType :
DiagGroup<"objc-property-assign-on-object-type">;
def ObjCProtocolQualifiers : DiagGroup<"objc-protocol-qualifiers">;
def ObjCMissingSuperCalls : DiagGroup<"objc-missing-super-calls">;
def ObjCDesignatedInit : DiagGroup<"objc-designated-initializers">;
Index: test/SemaObjC/property-in-class-extension-1.m
===
--- test/SemaObjC/property-in-class-extension-1.m
+++ test/SemaObjC/property-in-class-extension-1.m
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fobjc-weak -verify -Weverything %s
-// RUN: %clang_cc1 -x objective-c++ -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fobjc-weak -fsyntax-only -verify -Weverything %s
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fobjc-weak -verify -Wproperty-attri
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added a comment. In https://reviews.llvm.org/D44539#1207982, @rjmccall wrote: > LGTM. Thanks! What I should do next? Haven't found any info in docs about it :) Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added a comment. > you can ask for commit privileges yourself. Ok, how do I do it? Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added a comment. In https://reviews.llvm.org/D44539#1208838, @rjmccall wrote: > Please read the developer policy: https://llvm.org/docs/DeveloperPolicy.html > > The information is on that page. > We grant commit access to contributors with a track record of submitting high > quality patches. I don't think I'm quite fitting these criteria yet :) Can you please commit this patch for me? Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added a comment. It's been a couple of months now, @rjmccall any ETA's? Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1018 +def warn_objc_property_assign_on_object : Warning< + "'assign' attribute must not be of object type, use 'unsafe_unretained' instead">, + InGroup, DefaultIgnore; rjmccall wrote: > "must" is rather strong for a warning. Maybe something more like "'assign' > attribute on property of object type could be 'unsafe_unretained'"? But "could be" is rather weak :) May be "Prefer using explicit 'unsafe_unretained' attribute instead of 'assign' for object types", or "Using explicit 'unsafe_unretained' attribute instead of 'assign' for object types is preferred" (if passive voice is preferred) Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 created this revision.
QF5690 added a reviewer: rsmith.
QF5690 added a project: clang.
Herald added a subscriber: cfe-commits.
There is a problem, that `assign` attribute very often getting out of
attention. For example, consider this code:
@property(nonatomic, strong, readonly, nullable) SomeObjcClassType1* foo;
@property(nonatomic, strong, readwrite) SomeObjcClassType2* bar;
@property(nonatomic, assign, readonly) SomeObjcClassType* property;
@property(nonatomic, assign, readonly) SomeCStructType state;
It is very easy to miss that `assign` keyword, and it leads to hard to find and
reproduce bugs. Most of the time, we found such bugs in crash reports from
already in production code.
Now, consider this code:
@property(nonatomic, strong, readonly, nullable) SomeObjcClassType1* foo;
@property(nonatomic, strong, readwrite) SomeObjcClassType2* bar;
@property(nonatomic, unsafe_unretained, readonly) SomeObjcClassType* property;
@property(nonatomic, assign, readonly) SomeCStructType state;
It is now much harder to even make that mistake and it will be much obvious
during code review.
As there is no difference in behaviour between `assign` and `unsafe_unretained`
attribute, but second is much more verbose, saying "think twice when doing
this", I suggest to have, at least, optional warning, that will catch such
constructs.
This is my first revision in llvm, so any help would be very much appreciated.
Repository:
rC Clang
https://reviews.llvm.org/D44539
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaObjCProperty.cpp
test/SemaObjC/property-assign-on-object-type.m
test/SemaObjC/property-in-class-extension-1.m
Index: test/SemaObjC/property-in-class-extension-1.m
===
--- test/SemaObjC/property-in-class-extension-1.m
+++ test/SemaObjC/property-in-class-extension-1.m
@@ -15,12 +15,12 @@
@property (readonly) NSString* none;
@property (readonly) NSString* none1;
-@property (assign, readonly) NSString* changeMemoryModel; // expected-note
{{property declared here}}
+@property (unsafe_unretained, readonly) NSString* changeMemoryModel; //
expected-note {{property declared here}}
@property (readonly) __weak id weak_prop;
@property (readonly) __weak id weak_prop1;
-@property (assign, readonly) NSString* assignProperty;
+@property (unsafe_unretained, readonly) NSString* unsafeUnretainedProperty;
@property (readonly) NSString* readonlyProp;
@@ -43,8 +43,8 @@
@property () __weak id weak_prop;
@property (readwrite) __weak id weak_prop1;
-@property (assign, readwrite) NSString* assignProperty;
-@property (assign) NSString* readonlyProp;
+@property (unsafe_unretained, readwrite) NSString* unsafeUnretainedProperty;
+@property (unsafe_unretained) NSString* readonlyProp;
@end
// rdar://12214070
Index: test/SemaObjC/property-assign-on-object-type.m
===
--- /dev/null
+++ test/SemaObjC/property-assign-on-object-type.m
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wobjc-property-assign-on-object-type
+
+@interface Foo @end
+
+@interface Bar
+@property(assign, readonly) Foo* o1; // expected-warning {{'assign' attribute
must not be of object type, use 'unsafe_unretained' instead}}
+@property(unsafe_unretained, readonly) Foo* o2;
+
+@property(assign) int s1;
+@property(assign) int* s2;
+@end
+
+@interface Bar ()
+@property(readwrite) Foo* o1;
+@property(readwrite) Foo* o2;
+@end
Index: lib/Sema/SemaObjCProperty.cpp
===
--- lib/Sema/SemaObjCProperty.cpp
+++ lib/Sema/SemaObjCProperty.cpp
@@ -2547,6 +2547,13 @@
PropertyDecl->setInvalidDecl();
}
+ // Check for assign on object types.
+ if ((Attributes & ObjCDeclSpec::DQ_PR_assign) &&
+ !(Attributes & ObjCDeclSpec::DQ_PR_unsafe_unretained) &&
+ PropertyTy->isObjCRetainableType()) {
+Diag(Loc, diag::warn_objc_property_assign_on_object);
+ }
+
// Check for more than one of { assign, copy, retain }.
if (Attributes & ObjCDeclSpec::DQ_PR_assign) {
if (Attributes & ObjCDeclSpec::DQ_PR_copy) {
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -1014,6 +1014,9 @@
"property attributes '%0' and '%1' are mutually exclusive">;
def err_objc_property_requires_object : Error<
"property with '%0' attribute must be of object type">;
+def warn_objc_property_assign_on_object : Warning<
+ "'assign' attribute must not be of object type, use 'unsafe_unretained'
instead">,
+ InGroup, DefaultIgnore;
def warn_objc_property_no_assignment_attribute : Warning<
"no 'assign', 'retain', or 'copy' attribute is specified - "
"'assign' is assumed">,
Index: include/clang/Basic/Diagn
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added reviewers: aaron.ballman, benhamilton. QF5690 added a comment. Hey, saw many revisions around obj-c and sema that you are reviewing. Can you help me with this one? Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 updated this revision to Diff 150487.
QF5690 added a comment.
Remove warning for `Class` type, change warning message.
Repository:
rC Clang
https://reviews.llvm.org/D44539
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaObjCProperty.cpp
test/SemaObjC/property-assign-on-object-type.m
test/SemaObjC/property-in-class-extension-1.m
Index: test/SemaObjC/property-in-class-extension-1.m
===
--- test/SemaObjC/property-in-class-extension-1.m
+++ test/SemaObjC/property-in-class-extension-1.m
@@ -15,12 +15,12 @@
@property (readonly) NSString* none;
@property (readonly) NSString* none1;
-@property (assign, readonly) NSString* changeMemoryModel; // expected-note
{{property declared here}}
+@property (unsafe_unretained, readonly) NSString* changeMemoryModel; //
expected-note {{property declared here}}
@property (readonly) __weak id weak_prop;
@property (readonly) __weak id weak_prop1;
-@property (assign, readonly) NSString* assignProperty;
+@property (unsafe_unretained, readonly) NSString* unsafeUnretainedProperty;
@property (readonly) NSString* readonlyProp;
@@ -43,8 +43,8 @@
@property () __weak id weak_prop;
@property (readwrite) __weak id weak_prop1;
-@property (assign, readwrite) NSString* assignProperty;
-@property (assign) NSString* readonlyProp;
+@property (unsafe_unretained, readwrite) NSString* unsafeUnretainedProperty;
+@property (unsafe_unretained) NSString* readonlyProp;
@end
// rdar://12214070
Index: test/SemaObjC/property-assign-on-object-type.m
===
--- /dev/null
+++ test/SemaObjC/property-assign-on-object-type.m
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wobjc-property-assign-on-object-type
%s
+
+@interface Foo @end
+
+@interface Bar
+@property(assign, readonly) Foo* o1; // expected-warning {{'assign' property
of object type may become a dangling reference; consider using
'unsafe_unretained'}}
+@property(unsafe_unretained, readonly) Foo* o2;
+
+@property(assign) Class classProperty;
+@property(assign) int s1;
+@property(assign) int* s2;
+@end
+
+@interface Bar ()
+@property(readwrite) Foo* o1;
+@property(readwrite) Foo* o2;
+@end
Index: lib/Sema/SemaObjCProperty.cpp
===
--- lib/Sema/SemaObjCProperty.cpp
+++ lib/Sema/SemaObjCProperty.cpp
@@ -2547,6 +2547,14 @@
PropertyDecl->setInvalidDecl();
}
+ // Check for assign on object types.
+ if ((Attributes & ObjCDeclSpec::DQ_PR_assign) &&
+ !(Attributes & ObjCDeclSpec::DQ_PR_unsafe_unretained) &&
+ PropertyTy->isObjCRetainableType() &&
+ !PropertyTy->isObjCClassType()) {
+Diag(Loc, diag::warn_objc_property_assign_on_object);
+ }
+
// Check for more than one of { assign, copy, retain }.
if (Attributes & ObjCDeclSpec::DQ_PR_assign) {
if (Attributes & ObjCDeclSpec::DQ_PR_copy) {
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -1014,6 +1014,9 @@
"property attributes '%0' and '%1' are mutually exclusive">;
def err_objc_property_requires_object : Error<
"property with '%0' attribute must be of object type">;
+def warn_objc_property_assign_on_object : Warning<
+ "'assign' property of object type may become a dangling reference; consider
using 'unsafe_unretained'">,
+ InGroup, DefaultIgnore;
def warn_objc_property_no_assignment_attribute : Warning<
"no 'assign', 'retain', or 'copy' attribute is specified - "
"'assign' is assumed">,
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -367,6 +367,7 @@
def BadFunctionCast : DiagGroup<"bad-function-cast">;
def ObjCPropertyImpl : DiagGroup<"objc-property-implementation">;
def ObjCPropertyNoAttribute : DiagGroup<"objc-property-no-attribute">;
+def ObjCPropertyAssignOnObjectType :
DiagGroup<"objc-property-assign-on-object-type">;
def ObjCProtocolQualifiers : DiagGroup<"objc-protocol-qualifiers">;
def ObjCMissingSuperCalls : DiagGroup<"objc-missing-super-calls">;
def ObjCDesignatedInit : DiagGroup<"objc-designated-initializers">;
Index: test/SemaObjC/property-in-class-extension-1.m
===
--- test/SemaObjC/property-in-class-extension-1.m
+++ test/SemaObjC/property-in-class-extension-1.m
@@ -15,12 +15,12 @@
@property (readonly) NSString* none;
@property (readonly) NSString* none1;
-@property (assign, readonly) NSString* changeMemoryModel; // expected-note {{property declared here}}
+@property (unsafe_unretained, readonly) NSString* changeMemoryModel; // expected-note {{pr
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added inline comments.
Comment at: lib/Sema/SemaObjCProperty.cpp:2554
+ PropertyTy->isObjCRetainableType() &&
+ !PropertyTy->isObjCClassType()) {
+Diag(Loc, diag::warn_objc_property_assign_on_object);
rjmccall wrote:
> Please use `isObjCARCImplicitlyUnretainedType` here.
Thanks, didn't notice it.
Comment at: test/SemaObjC/property-in-class-extension-1.m:18
-@property (assign, readonly) NSString* changeMemoryModel; // expected-note
{{property declared here}}
+@property (unsafe_unretained, readonly) NSString* changeMemoryModel; //
expected-note {{property declared here}}
rjmccall wrote:
> Whoa, why is this test case using `-Weverything`? That seems unnecessarily
> fragile.
Do you think it should be relaxed only to warnings that are appearing here?
Repository:
rC Clang
https://reviews.llvm.org/D44539
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added a comment. In https://reviews.llvm.org/D44539#1041993, @benhamilton wrote: > I wonder if this wouldn't be better as a clang-tidy check: > > https://github.com/llvm-mirror/clang-tools-extra/tree/master/clang-tidy/objc Border between clang-tidy checks and compiler diagnostics is not entirely clear for me, but, as I understand, clang-tidy is more about codestyle and some internal team agreements. The check that I'm adding is intended to prevent crashes, and it is much better to catch such things in compile time. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added a comment. > I think the problem is there are valid places to use assign on object types > (especially delegates). Isn't it better to have unsafe_unretained there? I thought unsafe_unretained keyword is introduced specifically for that kinds of things. Ok, here is my last point :) Attributes like `strong`, `retain`, `copy` is commonly reffered as "ownership attribute". But `assign` does not tells anything about ownership, and from that point of view, it is semantically wrong to have `assign` on object types, and having `unsafe_unretained` that actually tells something about ownership – correct. If that's not the case, I can't understand the reason why `unsafe_unreatined` even exists. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
