Get class property selectors from property decl if it exists

[herzka]

Summary

Before this fix, trying to get or set a class property using dot syntax would always use the constructed name (x or setX:), which might not match the real selector if the getter or setter is specified via property attributes. Now, the selectors in the declaration have priority over the constructed names, which is consistent with instance properties.

[herzka]

This issue applies to getters too. I'll make this fix broader.

I think the real fix for both is to construct the ObjCPropertyRefExpr using the ObjCPropertyDecl so that it's considered an explicit property. Currently, it still gets considered implicit, which is not true. I don't know what implications changing that may have, though. This fix seems safer.

[arphaman]

Can you please post the patch with full context (http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)?

[herzka]

Added full context. Sorry about that! This is my first contribution, and I don't know how I completely missed that line while going through the steps.

[compnerd]

Please add a test for this.

lib/Sema/SemaExprObjC.cpp

1989

Use auto.

[herzka]

Added test, used auto

[herzka]

Rename selectors

[compnerd]

I think Im misunderstanding something. How does the test actually test what you are changing?

[herzka]

Currently, A.customGetterProperty would be turned into [A customGetterProperty], which would fail to compile because that selector isn't declared anywhere. With my fix, it does compile because it (correctly) resolves to [A customGet], which does exist. Similarly, A.customSetterProperty = 1 got turned into [A setCustomSetterProperty:1] (another nonexistent selector), but now becomes [A customSet:1].

Here are the errors I get when I run the test without my fix:

error: 'error' diagnostics seen but not expected:
  File /Users/herzka/dev/OSS/llvm/tools/clang/test/SemaObjC/objc-class-property.m Line 45: no setter method 'setCustomSetterProperty:' for assignment to property
  File /Users/herzka/dev/OSS/llvm/tools/clang/test/SemaObjC/objc-class-property.m Line 46: no getter method for read from property
2 errors generated.

[compnerd]

Ah, I had missed the -verify option on the test. Yes, that makes sense. Ternary may have flowed the conditional code better. Do you need someone to commit this on your behalf?

[herzka]

@compnerd, yes please. I don't have commit access. Thanks for the review!

[compnerd]

SVN r295683

lib/Sema/SemaExprObjC.cpp

if (!IFace) {
 
     if (!IFace) {
       Diag(receiverNameLoc, diag::err_expected_either) << tok::identifier
                                                        << tok::l_paren;
       return ExprError();
     }
   }
 
+  Selector GetterSel;
+  Selector SetterSel;
+  if (auto PD = IFace->FindPropertyDeclaration(
+          &propertyName, ObjCPropertyQueryKind::OBJC_PR_query_class)) {
+    GetterSel = PD->getGetterName();
+    SetterSel = PD->getSetterName();
+  } else {
+    GetterSel = PP.getSelectorTable().getNullarySelector(&propertyName);
+    SetterSel =
+      SelectorTable::constructSetterSelector(PP.getIdentifierTable(),
+                                             PP.getSelectorTable(),
+                                             &propertyName);
+  }
+
   // Search for a declared property first.
-  Selector Sel = PP.getSelectorTable().getNullarySelector(&propertyName);
-  ObjCMethodDecl *Getter = IFace->lookupClassMethod(Sel);
+  ObjCMethodDecl *Getter = IFace->lookupClassMethod(GetterSel);
 
   // If this reference is in an @implementation, check for 'private' methods.
   if (!Getter)
-    Getter = IFace->lookupPrivateClassMethod(Sel);
+    Getter = IFace->lookupPrivateClassMethod(GetterSel);
 
   if (Getter) {
     // FIXME: refactor/share with ActOnMemberReference().
     // Check if we can reference this property.
     if (DiagnoseUseOfDecl(Getter, propertyNameLoc))
       return ExprError();
   }
 
   // Look for the matching setter, in case it is needed.
-  Selector SetterSel =
-    SelectorTable::constructSetterSelector(PP.getIdentifierTable(),
-                                            PP.getSelectorTable(),
-                                           &propertyName);
-
   ObjCMethodDecl *Setter = IFace->lookupClassMethod(SetterSel);
   if (!Setter) {
     // If this reference is in an @implementation, also check for 'private'
     // methods.
     Setter = IFace->lookupPrivateClassMethod(SetterSel);
   }
   // Look through local category implementations associated with the class.
   if (!Setter)

test/SemaObjC/objc-class-property.m

 @property int x;
 @property int y;
 @property int z;
 @property(readonly) int ro, ro2;
 @property (class) int c;
 @property (class) int c2; // expected-note {{property declared here}} \
                           // expected-note {{property declared here}}
 @property (class) int x;
+@property (class, setter=customSet:) int customSetterProperty;
+@property (class, getter=customGet) int customGetterProperty;
 @end
 
 @implementation A // expected-warning {{class property 'c2' requires method 'c2' to be defined}} \
                   // expected-warning {{class property 'c2' requires method 'setC2:' to be defined}}
 @dynamic x; // refers to the instance property
 @dynamic (class) x; // refers to the class property
 @synthesize z, c2; // expected-error {{@synthesize not allowed on a class property 'c2'}}
 @dynamic c; // refers to the class property
+@dynamic customSetterProperty;
+@dynamic customGetterProperty;
 @end
 
 int test() {
   A *a = [[A alloc] init];
   a.c; // expected-error {{property 'c' is a class property; did you mean to access it with class 'A'}}
   return a.x + A.c;
 }
 
+void customSelectors() {
+  A.customSetterProperty = 1;
+  (void)A.customGetterProperty;
+}
+
 void message_id(id me) {
   [me y];
 }
 
 void message_class(Class me) {
   [me c2];
 }