[ClangImporter] Reject *all* attempts to redeclare properties

Add an algorithm to go search the override tables *and* the existing
loaded members of a class for a redeclaration point.  The idea is that
both mirrored protocol members and categories offer a way to convince
the Clang Importer to import a property twice. We support a limited form
of this multiple-imports behavior today by allowing the redeclared
property to refine a readonly property into a readwrite property.

To maintain both the refinement behavior and to disambiguate any
remaining cases of ambiguity caused by extensions, attempt to identify
a redeclaration point if we can't identify an override point. Then,
decline to import the redeclared member if we're successful.

Note that the algorithm as presented is subject to import ordering. That
is, if a framework declares both an overlay and a clang module unit, if
the overlay is not loaded or members from the overlay are not installed
in the class by the time we see the declaration we may fail to identify
an redeclaration point.

The regression tests are for rdar://59044692, rdar://59075988, and
rdar://59125907.

Author: CodaFi

lib/ClangImporter/ImportDecl.cpp

@@ -2208,6 +2208,50 @@ namespace {
     return {nullptr, foundMethod};
   }
 
+  // Attempt to identify the redeclaration of a property.
+  //
+  // Note that this function does not perform any additional member loading and
+  // is therefore subject to the relativistic effects of module import order.
+  // That is, suppose that a Clang Module and an Overlay module are in play.
+  // Depending on which module loads members first, a redeclaration point may
+  // or may not be identifiable.
+  VarDecl *
+  identifyPropertyRedeclarationPoint(ClangImporter::Implementation &Impl,
+                                     const clang::ObjCPropertyDecl *decl,
+                                     ClassDecl *subject, Identifier name) {
+    llvm::SetVector<Decl *> lookup;
+    // First, pull in all available members of the base class so we can catch
+    // redeclarations of APIs that are refined for Swift.
+    auto currentMembers = subject->getCurrentMembersWithoutLoading();
+    lookup.insert(currentMembers.begin(), currentMembers.end());
+
+    // Now pull in any just-imported members from the overrides table.
+    auto foundNames = Impl.MembersForNominal.find(subject);
+    if (foundNames != Impl.MembersForNominal.end()) {
+      auto foundDecls = foundNames->second.find(name);
+      if (foundDecls != foundNames->second.end()) {
+        lookup.insert(foundDecls->second.begin(), foundDecls->second.end());
+      }
+    }
+
+    for (auto *result : lookup) {
+      auto *var = dyn_cast<VarDecl>(result);
+      if (!var)
+        continue;
+
+      if (var->isInstanceMember() != decl->isInstanceProperty())
+        continue;
+
+      // If the selectors of the getter match in Objective-C, we have a
+      // redeclaration.
+      if (var->getObjCGetterSelector() ==
+          Impl.importSelector(decl->getGetterName())) {
+        return var;
+      }
+    }
+    return nullptr;
+  }
+
   /// Convert Clang declarations into the corresponding Swift
   /// declarations.
   class SwiftDeclConverter
@@ -5094,12 +5138,21 @@ namespace {
               overrideContext->getSelfNominalTypeDecl()
                 == dc->getSelfNominalTypeDecl()) {
             // We've encountered a redeclaration of the property.
-            // HACK: Just update the original declaration instead of importing a
-            // second property.
             handlePropertyRedeclaration(overridden, decl);
             return nullptr;
           }
         }
+
+        // Try searching the class for a property redeclaration. We can use
+        // the redeclaration to refine the already-imported property with a
+        // setter and also cut off any double-importing behavior.
+        auto *redecl
+            = identifyPropertyRedeclarationPoint(Impl, decl,
+                                                 dc->getSelfClassDecl(), name);
+        if (redecl) {
+          handlePropertyRedeclaration(redecl, decl);
+          return nullptr;
+        }
       }
 
       auto importedType = Impl.importPropertyType(decl, isInSystemModule(dc));

test/ClangImporter/Inputs/frameworks/CategoryOverrides.framework/Headers/CategoryOverrides.h

@@ -32,3 +32,19 @@ typedef enum {
 @interface Refinery : Base
 @property (nonatomic, readonly) RefinedSugar sugar /*NS_REFINED_FOR_SWIFT*/ __attribute__((swift_private));
 @end
+
+@protocol NullableProtocol
+@property (nonatomic, readonly, nullable) Base *requirement;
+@end
+
+@protocol NonNullProtocol <NullableProtocol>
+@property (nonatomic, readonly, nonnull) Base *requirement;
+@end
+
+@protocol ReadonlyProtocol
+@property (nonatomic, readonly) int answer;
+@end
+
+@protocol ReadwriteProtocol <ReadonlyProtocol>
+@property (nonatomic, readwrite) int answer;
+@end

test/ClangImporter/Inputs/frameworks/CategoryOverrides.framework/PrivateHeaders/Private.h

@@ -18,3 +18,9 @@
 @interface Refinery ()
 @property (nonatomic, readwrite) RefinedSugar sugar;
 @end
+
+@interface MyBaseClass () <NonNullProtocol>
+@end
+
+@interface MyDerivedClass () <ReadwriteProtocol>
+@end

test/ClangImporter/objc_redeclared_properties_categories.swift

@@ -11,7 +11,6 @@ import CategoryOverrides
 
 // Nail down some emergent behaviors of the Clang Importer's override checking:
 
-
 // A category declared in a (private) header can happen to double-import a property
 // and a function with the same name - both before and after omit-needless-words -
 // as long as they have different contextual types.
@@ -78,3 +77,20 @@ func takesARefinery(_ x: Refinery) {
   // CHECK: cannot assign to property: 'sugar' is a get-only property
   x.sugar = .caster
 }
+
+func nullabilityRefinementProto(_ x: MyBaseClass) {
+  // CHECK-PUBLIC: has no member 'requirement'
+  // CHECK-PRIVATE-NOT: has no member 'requirement'
+  // CHECK-PRIVATE-NOT: value of optional type 'Base?'
+  let _ : Base = x.requirement
+}
+
+func readwriteRefinementProto(_ x: MyDerivedClass) {
+  // CHECK-PUBLIC: has no member 'answer'
+  // CHECK-PRIVATE-NOT: has no member 'answer'
+  if x.answer == 0 {
+    // CHECK-PUBLIC: has no member 'answer'
+    // CHECK-PRIVATE-NOT: has no member 'answer'
+    x.answer = 42
+  }
+}