Page MenuHomePhabricator

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

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

Details

Summary

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;
@end

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!

clang/unittests/AST/ASTImporterTest.cpp
5646

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?

5662

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
clang/unittests/AST/ASTImporterTest.cpp
5646

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

5662

Done, thanks!

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

LGTM

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.