Redo ObjC Override Checking

The general problem with this approach is that the clang importer is part of the cache-fill for name lookup.  This means that qualified lookups it runs will be re-entrant and result in validation-order-dependent behaviors.

The next commit will restore the expected ordering behavior here.

Author: CodaFi

lib/ClangImporter/ImportDecl.cpp

@@ -2174,6 +2174,13 @@ namespace {
       return getVersion() == getActiveSwiftVersion();
     }
 
+    template <typename T>
+    T *recordMemberInContext(DeclContext *dc, T *member) {
+      assert(member && "Attempted to record null member!");
+      Impl.MembersForNominal[dc->getSelfNominalTypeDecl()].push_back(member);
+      return member;
+    }
+
     /// Import the name of the given entity.
     ///
     /// This version of importFullName introduces any context-specific
@@ -3734,7 +3741,7 @@ namespace {
       if (correctSwiftName)
         markAsVariant(result, *correctSwiftName);
 
-      return result;
+      return recordMemberInContext(dc, result);
     }
 
     void finishFuncDecl(const clang::FunctionDecl *decl,
@@ -4344,7 +4351,7 @@ namespace {
         }
       }
 
-      return result;
+      return recordMemberInContext(dc, result);
     }
 
   public:
@@ -5002,44 +5009,57 @@ namespace {
         return nullptr;
 
       VarDecl *overridden = nullptr;
-      if (dc->getSelfClassDecl()) {
-        // Check whether there is a function with the same name as this
-        // property. If so, suppress the property; the user will have to use
-        // the methods directly, to avoid ambiguities.
-        NominalTypeDecl *lookupContext = dc->getSelfNominalTypeDecl();
+      // Check whether there is a function with the same name as this
+      // property. If so, suppress the property; the user will have to use
+      // the methods directly, to avoid ambiguities.
+      if (auto *subject = dc->getSelfClassDecl()) {
+        bool foundMethod = false;
 
         if (auto *classDecl = dyn_cast<ClassDecl>(dc)) {
           // If we're importing into the primary @interface for something, as
           // opposed to an extension, make sure we don't try to load any
           // categories...by just looking into the super type.
-          lookupContext = classDecl->getSuperclassDecl();
+          subject = classDecl->getSuperclassDecl();
         }
+        
+        for (; subject; (subject = subject->getSuperclassDecl())) {
+          llvm::SmallVector<ValueDecl *, 8> lookup;
+          auto found = Impl.MembersForNominal.find(subject);
+          if (found != Impl.MembersForNominal.end()) {
+            lookup.append(found->second.begin(), found->second.end());
+            namelookup::pruneLookupResultSet(dc, NL_QualifiedDefault, lookup);
+          }
+
+          for (auto &result : lookup) {
+            // Skip declarations that don't match the name we're looking for.
+            if (result->getBaseName() != name)
+              continue;
+
+            if (auto *fd = dyn_cast<FuncDecl>(result)) {
+              if (fd->isInstanceMember() != decl->isInstanceProperty())
+                continue;
+
+              if (fd->getFullName().getArgumentNames().empty()) {
+                foundMethod = true;
+              }
+            } else {
+              auto *var = cast<VarDecl>(result);
+              if (var->isInstanceMember() != decl->isInstanceProperty())
+                continue;
 
-        if (lookupContext) {
-          SmallVector<ValueDecl *, 2> lookup;
-          dc->lookupQualified(lookupContext, DeclNameRef(name),
-                              NL_QualifiedDefault | NL_KnownNoDependency,
-                              lookup);
-          bool foundMethod = false;
-          for (auto result : lookup) {
-            if (isa<FuncDecl>(result) &&
-                result->isInstanceMember() == decl->isInstanceProperty() &&
-                result->getFullName().getArgumentNames().empty())
-              foundMethod = true;
-
-            if (auto var = dyn_cast<VarDecl>(result)) {
               // If the selectors of the getter match in Objective-C, we have an
               // override.
-              if (var->isInstanceMember() == decl->isInstanceProperty() &&
-                  var->getObjCGetterSelector() ==
-                    Impl.importSelector(decl->getGetterName()))
+              if (var->getObjCGetterSelector() ==
+                  Impl.importSelector(decl->getGetterName())) {
                 overridden = var;
+              }
             }
           }
-          if (foundMethod && !overridden)
-            return nullptr;
         }
 
+        if (foundMethod && !overridden)
+          return nullptr;
+
         if (overridden) {
           const DeclContext *overrideContext = overridden->getDeclContext();
           // It's okay to compare interface types directly because Objective-C
@@ -5130,7 +5150,7 @@ namespace {
       if (correctSwiftName)
         markAsVariant(result, *correctSwiftName);
 
-      return result;
+      return recordMemberInContext(dc, result);
     }
 
     Decl *

lib/ClangImporter/ImporterImpl.h

@@ -447,6 +447,10 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
   // Mapping from imported types to their raw value types.
   llvm::DenseMap<const NominalTypeDecl *, Type> RawTypes;
 
+  /// Keep track of all member declarations that have been imported into a nominal type.
+  llvm::DenseMap<const NominalTypeDecl *, std::vector<ValueDecl *>>
+      MembersForNominal;
+
   clang::CompilerInstance *getClangInstance() {
     return Instance.get();
   }

test/ClangImporter/objc_redeclared_properties_incompatible.swift

@@ -71,31 +71,40 @@ func testGenericWithoutInitializer(obj: PropertiesNoInitGeneric<Base>) {
   // CHECK-PUBLIC: [[@LINE-1]]:7: error: cannot assign to property: 'readwriteChange' is a get-only property
 }
 
+// N.B. We consider the category in the imported header Private.h as the
+// canonical declaration of these properties and drop the one in the framework.
+
 func testCategoryWithInitializer() {
   let obj = PropertiesInitCategory()
 
   let _: Int = obj.nullabilityChange
-  // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
+  // CHECK-PUBLIC: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
+  // CHECK-PRIVATE: [[@LINE-2]]:20: error: cannot convert value of type 'Base?' to specified type 'Int'
 
   let _: Int = obj.missingGenerics
-  // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'GenericClass<Base>' to specified type 'Int'
+  // CHECK-PUBLIC: [[@LINE-1]]:20: error: cannot convert value of type 'GenericClass<Base>' to specified type 'Int'
+  // CHECK-PRIVATE: [[@LINE-2]]:20: error: cannot convert value of type 'GenericClass<AnyObject>' to specified type 'Int'
 
   let _: Int = obj.typeChange
-  // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
+  // CHECK-PUBLIC: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
+  // CHECK-PRIVATE: [[@LINE-2]]:20: error: cannot convert value of type 'PrivateSubclass' to specified type 'Int'
 
   obj.readwriteChange = Base() // CHECK-PRIVATE-NOT: [[@LINE]]:{{.+}}: error
   // CHECK-PUBLIC: [[@LINE-1]]:7: error: cannot assign to property: 'readwriteChange' is a get-only property
 }
 
 func testCategoryWithoutInitializer(obj: PropertiesNoInitCategory) {
   let _: Int = obj.nullabilityChange
-  // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
+  // CHECK-PUBLIC: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
+  // CHECK-PRIVATE: [[@LINE-2]]:20: error: cannot convert value of type 'Base?' to specified type 'Int'
 
   let _: Int = obj.missingGenerics
-  // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'GenericClass<Base>' to specified type 'Int'
+  // CHECK-PUBLIC: [[@LINE-1]]:20: error: cannot convert value of type 'GenericClass<Base>' to specified type 'Int'
+  // CHECK-PRIVATE: [[@LINE-2]]:20: error: cannot convert value of type 'GenericClass<AnyObject>' to specified type 'Int'
 
   let _: Int = obj.typeChange
-  // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
+  // CHECK-PUBLIC: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
+  // CHECK-PRIVATE: [[@LINE-2]]:20: error: cannot convert value of type 'PrivateSubclass' to specified type 'Int'
 
   obj.readwriteChange = Base() // CHECK-PRIVATE-NOT: [[@LINE]]:{{.+}}: error
   // CHECK-PUBLIC: [[@LINE-1]]:7: error: cannot assign to property: 'readwriteChange' is a get-only property