Page MenuHomePhabricator

[ASTImporter] Fix import of ObjCPropertyDecl that share the same name

Authored by teemperor on Mar 22 2021, 7:38 AM.



Objective-C apparently allows name conflicts between instance and class properties, so this
is valid code:

@protocol DupProp
@property (class, readonly) int prop;
@property (readonly) int prop;

The ASTImporter however isn't aware of this and will consider the two properties as if
they are the same property because it just compares their name and types. This causes
that when importing both properties we only end up with one property (whatever is
imported first from what I can see).

Beside generating a different AST this also leads to a bunch of asserts and crashes as
we still correctly import the two different getters for both properties (the import code
for methods does the correct check where it differentiated between instance and
class methods). As one of the setters will not have its associated ObjCPropertyDecl
imported, any call to ObjCMethodDecl::findPropertyDecl will just lead to an assert
or crash.

Fixes rdar://74322659

Diff Detail

Event Timeline

teemperor created this revision.Mar 22 2021, 7:38 AM
teemperor requested review of this revision.Mar 22 2021, 7:38 AM
teemperor edited the summary of this revision. (Show Details)Mar 22 2021, 7:53 AM
kastiglione accepted this revision.Mar 22 2021, 8:08 AM

nice fix!


I'm a little surprised to see the test use a protocol and not a class. Is there any specific reason for that, or does the choice not matter?


should the test also verify read writer properties?

This revision is now accepted and ready to land.Mar 22 2021, 8:08 AM
teemperor updated this revision to Diff 332335.Mar 22 2021, 9:47 AM
  • Expanded checks to also verify setters.
  • Use class instead of protocol.
  • Check all properties (before we only checked class properties by accident)
teemperor added inline comments.Mar 22 2021, 9:50 AM

Declaring a protocol is 3 characters shorter!

(But it doesn't matter for the test itself and class seems like the traditional container, so I changed it.)


Done, thanks!

shafik accepted this revision.Mar 22 2021, 9:50 AM


This revision was landed with ongoing or failed builds.Mar 22 2021, 10:06 AM
This revision was automatically updated to reflect the committed changes.