[Sema][Objective-C] Add check to warn when property of objc type has assign attribute

[QF5690]

Summary

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.

[QF5690]

Hey, saw many revisions around obj-c and sema that you are reviewing. Can you help me with this one?

[benhamilton]

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

[aaron.ballman]

I don't know enough about ObjC to feel comfortable reviewing this, but I've added @rjmccall who may have opinions.

[QF5690]

In 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.

[benhamilton]

I think the problem is there are valid places to use assign on object types (especially delegates).

There are a lot of special cases to deal with here, and it's really up to each team to decide the rules for when assign on object types are OK, so I don't think a compiler warning is appropriate.

[QF5690]

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.

[benhamilton]

Isn't it better to have unsafe_unretained there?

That is a good question for the author of that property attribute. I'm not sure of the entire history.

[rjmccall]

We added the unsafe_unretained property attribute as part of ARC because we were introducing __unsafe_retained as a type qualifier and we wanted all the type qualifiers to have corresponding attribute spellings. assign is the much-older attribute, and its non-owning behavior was widely understood. In fact, we briefly considered naming the qualifier __assign, but we quickly decided that that we wanted a more explicit name for the ARC age.

I like the idea of this warning, but I need to float it to our internal Objective-C language group before we can accept it.

[QF5690]

It's been a couple of months now, @rjmccall any ETA's?

[rjmccall]

It's waiting on a discussion that's happening pretty slowly, sorry. I know this is frustrating.

[Wizard]

Is there any case for property of ObjC types that we should use unsafe_unretained or assign rather than weak? In my understanding, weak is for properties of ObjC types as the replacement of unsafe_unretained and assign is for properties of primitive types.

[rjmccall]

This isn't really an Objective-C user forum, but I'll try to summarize briefly. unsafe_unretained is an unsafe version of weak — it's unsafe because it can be left dangling if the object is deallocated. It's necessary for a small (and getting smaller every year) set of classes that don't support true weak references, and it can be useful as an optimization to avoid the performance overhead of reference counting.

[rjmccall]

This was approved by the Objective-C language group as a default-off warning.

include/clang/Basic/DiagnosticSemaKinds.td

1018

"must" is rather strong for a warning. Maybe something more like "'assign' attribute on property of object type could be 'unsafe_unretained'"?

[aaron.ballman]

In D44539#1106152, @rjmccall wrote:

This was approved by the Objective-C language group as a default-off warning.

We usually do not expose new default-off warnings because experience shows that they rarely ever get enabled by users. If the ObjC group doesn't think this should be on by default, I wonder if it should be included in Clang at all.

[QF5690]

include/clang/Basic/DiagnosticSemaKinds.td

1018

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)

[rjmccall]

In D44539#1106443, @aaron.ballman wrote:

In D44539#1106152, @rjmccall wrote:

This was approved by the Objective-C language group as a default-off warning.

We usually do not expose new default-off warnings because experience shows that they rarely ever get enabled by users. If the ObjC group doesn't think this should be on by default, I wonder if it should be included in Clang at all.

That's a fair question to ask. In this case, I'm in favor of adding it because we have evidence of there being a substantial set of users who would enable it enthusiastically.

include/clang/Basic/DiagnosticSemaKinds.td

1018

Neither of those is quite in the standard diagnostic "voice". Maybe something like "'assign' property of object type may become a dangling reference; consider using 'unsafe_unretained'"?

Oh, you should probably not warn about Class types.

[aaron.ballman]

In D44539#1106802, @rjmccall wrote:

In D44539#1106443, @aaron.ballman wrote:

In D44539#1106152, @rjmccall wrote:

This was approved by the Objective-C language group as a default-off warning.

We usually do not expose new default-off warnings because experience shows that they rarely ever get enabled by users. If the ObjC group doesn't think this should be on by default, I wonder if it should be included in Clang at all.

That's a fair question to ask. In this case, I'm in favor of adding it because we have evidence of there being a substantial set of users who would enable it enthusiastically.

Okay, that works for me. Thank you for verifying.

[QF5690]

Remove warning for Class type, change warning message.

[rjmccall]

lib/Sema/SemaObjCProperty.cpp

2554

Please use isObjCARCImplicitlyUnretainedType here.

test/SemaObjC/property-in-class-extension-1.m

18

Whoa, why is this test case using -Weverything? That seems unnecessarily fragile.

[QF5690]

lib/Sema/SemaObjCProperty.cpp

2554

Thanks, didn't notice it.

test/SemaObjC/property-in-class-extension-1.m

18

Do you think it should be relaxed only to warnings that are appearing here?

[rjmccall]

test/SemaObjC/property-in-class-extension-1.m

18

Yeah, tests should generally only be testing specific categories. Opt-out warnings are different, of course, but it's understood that adding a new opt-out warning is an ambitious change.

[rjmccall]

Hey, still working on this?

[QF5690]

Using isObjCARCImplicitlyUnretainedType instead of isObjCClassType
Tests in property-in-class-extension-1.m relaxed to -Wproperty-attribute-mismatch (was -Weverything)

[rjmccall]

LGTM.

[QF5690]

In D44539#1207982, @rjmccall wrote:

LGTM.

Thanks! What I should do next? Haven't found any info in docs about it :)

[rjmccall]

In D44539#1208780, @QF5690 wrote:

In D44539#1207982, @rjmccall wrote:

LGTM.

Thanks! What I should do next? Haven't found any info in docs about it :)

https://llvm.org/docs/Contributing.html

It's up to you. We can certainly commit it for you, but if you're likely to work on more patches, you can ask for commit privileges yourself.

[QF5690]

you can ask for commit privileges yourself.

Ok, how do I do it?

[rjmccall]

Please read the developer policy: https://llvm.org/docs/DeveloperPolicy.html

The information is on that page.

[QF5690]

In 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?

[rjmccall]

Committed as r341489.

include/clang/Basic/DiagnosticGroups.td

 def OverlengthStrings : DiagGroup<"overlength-strings">;
 def OverloadedVirtual : DiagGroup<"overloaded-virtual">;
 def PrivateExtern : DiagGroup<"private-extern">;
 def SelTypeCast : DiagGroup<"cast-of-sel-type">;
 def FunctionDefInObjCContainer : DiagGroup<"function-def-in-objc-container">;
 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">;
 def ObjCRetainBlockProperty : DiagGroup<"objc-noncopy-retain-block-property">;
 def ObjCReadonlyPropertyHasSetter : DiagGroup<"objc-readonly-with-setter-property">;
 def ObjCInvalidIBOutletProperty : DiagGroup<"invalid-iboutlet">;
 def ObjCRootClass : DiagGroup<"objc-root-class">;
 def ObjCPointerIntrospectPerformSelector : DiagGroup<"deprecated-objc-pointer-introspection-performSelector">;

include/clang/Basic/DiagnosticSemaKinds.td

 def err_objc_var_decl_inclass : 
     Error<"cannot declare variable inside @interface or @protocol">;
 def err_missing_method_context : Error<
   "missing context for method declaration">;
 def err_objc_property_attr_mutually_exclusive : Error<
   "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<ObjCPropertyAssignOnObjectType>, DefaultIgnore;
 def warn_objc_property_no_assignment_attribute : Warning<
   "no 'assign', 'retain', or 'copy' attribute is specified - "
   "'assign' is assumed">,
   InGroup<ObjCPropertyNoAttribute>;
 def warn_objc_isa_use : Warning<
   "direct access to Objective-C's isa is deprecated in favor of "
   "object_getClass()">, InGroup<DeprecatedObjCIsaUsage>;
 def warn_objc_isa_assign : Warning<

lib/Sema/SemaObjCProperty.cpp

if ((Attributes & (ObjCDeclSpec::DQ_PR_weak | ObjCDeclSpec::DQ_PR_copy |
     Diag(Loc, diag::err_objc_property_requires_object)
       << (Attributes & ObjCDeclSpec::DQ_PR_weak ? "weak" :
           Attributes & ObjCDeclSpec::DQ_PR_copy ? "copy" : "retain (or strong)");
     Attributes &= ~(ObjCDeclSpec::DQ_PR_weak   | ObjCDeclSpec::DQ_PR_copy |
                     ObjCDeclSpec::DQ_PR_retain | ObjCDeclSpec::DQ_PR_strong);
     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) {
       Diag(Loc, diag::err_objc_property_attr_mutually_exclusive)
         << "assign" << "copy";
       Attributes &= ~ObjCDeclSpec::DQ_PR_copy;
     }
     if (Attributes & ObjCDeclSpec::DQ_PR_retain) {

test/SemaObjC/property-assign-on-object-type.m

+// 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<Prot> classWithProtocolProperty;
+@property(assign) int s1;
+@property(assign) int* s2;
+@end
+
+@interface Bar ()
+@property(readwrite) Foo* o1;
+@property(readwrite) Foo* o2;
+@end

test/SemaObjC/property-in-class-extension-1.m

-// 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;
 
 @interface MyClass
 
 @property (nonatomic, readonly) NSString* addingMemoryModel;
 
 @property (nonatomic, copy, readonly) NSString* matchingMemoryModel;
 
 @property (atomic, retain, readonly) NSString* addingNoNewMemoryModel;
 
 @property (readonly) NSString* none;
 @property (readonly) NSString* none1;
 
 @property (assign, 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 (readonly) NSString* readonlyProp;