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

[teemperor]

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

[kastiglione]

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?

[teemperor]

• Expanded checks to also verify setters.
• Use class instead of protocol.
• Check all properties (before we only checked class properties by accident)

[teemperor]

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]

LGTM

clang/lib/AST/ASTImporter.cpp

if (Error Err = ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc))
     return std::move(Err);
   if (ToD)
     return ToD;
 
   // Check whether we have already imported this property.
   auto FoundDecls = Importer.findDeclsInToCtx(DC, Name);
   for (auto *FoundDecl : FoundDecls) {
     if (auto *FoundProp = dyn_cast<ObjCPropertyDecl>(FoundDecl)) {
+      // Instance and class properties can share the same name but are different
+      // declarations.
+      if (FoundProp->isInstanceProperty() != D->isInstanceProperty())
+        continue;
+
       // Check property types.
       if (!Importer.IsStructurallyEquivalent(D->getType(),
                                              FoundProp->getType())) {
         Importer.ToDiag(Loc, diag::warn_odr_objc_property_type_inconsistent)
           << Name << D->getType() << FoundProp->getType();
         Importer.ToDiag(FoundProp->getLocation(), diag::note_odr_value_here)
           << FoundProp->getType();
 

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase, ImplicitlyDeclareSelf) {
   auto ToMethod = Import(FromMethod, Lang_OBJCXX);
   ASSERT_TRUE(ToMethod);
 
   // Both methods should have their implicit parameters.
   EXPECT_TRUE(FromMethod->getSelfDecl() != nullptr);
   EXPECT_TRUE(ToMethod->getSelfDecl() != nullptr);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ObjPropertyNameConflict) {
+  // Tests that properties that share the same name are correctly imported.
+  // This is only possible with one instance and one class property.
+  Decl *FromTU = getTuDecl(R"(
+                           @interface DupProp{}
+                           @property (class) int prop;
+                           @property int prop;
+                           @end
+                           )",
+                           Lang_OBJCXX, "input.mm");
+  auto *FromClass = FirstDeclMatcher<ObjCInterfaceDecl>().match(
+      FromTU, namedDecl(hasName("DupProp")));
+  auto ToClass = Import(FromClass, Lang_OBJCXX);
+  ASSERT_TRUE(ToClass);
+  // We should have one class and one instance property.
+  ASSERT_EQ(
+      1, std::distance(ToClass->classprop_begin(), ToClass->classprop_end()));
+  ASSERT_EQ(1,
+            std::distance(ToClass->instprop_begin(), ToClass->instprop_end()));
+  for (clang::ObjCPropertyDecl *prop : ToClass->properties()) {
+    // All properties should have a getter and a setter.
+    ASSERT_TRUE(prop->getGetterMethodDecl());
+    ASSERT_TRUE(prop->getSetterMethodDecl());
+    // The getters/setters should be able to find the right associated property.
+    ASSERT_EQ(prop->getGetterMethodDecl()->findPropertyDecl(), prop);
+    ASSERT_EQ(prop->getSetterMethodDecl()->findPropertyDecl(), prop);
+  }
+}
+
 struct ImportAutoFunctions : ASTImporterOptionSpecificTestBase {};
 
 TEST_P(ImportAutoFunctions, ReturnWithTypedefDeclaredInside) {
   Decl *FromTU = getTuDecl(
       R"(
       auto X = [](long l) {
         using int_type = long;
         auto dur = 13;