[ClangImporter] Always load a class's members before any categories.

This ensures that any @property redeclarations that appear in class
extensions (a special kind of category in ObjC) do not affect the
primary type of the property as declared in the class itself.

To accomplish this, lookups in importDecl that are checking for
conflicts now no longer pull in new categories/extensions if the
current context is a ClassDecl.

rdar://problem/30785976

lib/AST/NameLookup.cpp

@@ -1181,6 +1181,8 @@ void NominalTypeDecl::makeMemberVisible(ValueDecl *member) {
 TinyPtrVector<ValueDecl *> NominalTypeDecl::lookupDirect(
                                                   DeclName name,
                                                   bool ignoreNewExtensions) {
+  (void)getMembers();
+
   // Make sure we have the complete list of members (in this nominal and in all
   // extensions).
   if (!ignoreNewExtensions) {
@@ -1188,8 +1190,6 @@ TinyPtrVector<ValueDecl *> NominalTypeDecl::lookupDirect(
       (void)E->getMembers();
   }
 
-  (void)getMembers();
-
   prepareLookupTable(ignoreNewExtensions);
 
   // Look for the declarations with this name.

lib/ClangImporter/ImportDecl.cpp

@@ -4325,25 +4325,36 @@ namespace {
       // 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.
-      auto containerTy = dc->getDeclaredTypeInContext();
-      VarDecl *overridden = nullptr;
-      SmallVector<ValueDecl *, 2> lookup;
-      dc->lookupQualified(containerTy, name,
-                          NL_QualifiedDefault | NL_KnownNoDependency,
-                          Impl.getTypeResolver(), lookup);
-      for (auto result : lookup) {
-        if (isa<FuncDecl>(result) &&
-            result->isInstanceMember() == decl->isInstanceProperty() &&
-            result->getFullName().getArgumentNames().empty())
-          return nullptr;
+      Type containerTy;
+      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.
+        containerTy = classDecl->getSuperclass();
+      } else {
+        containerTy = dc->getDeclaredTypeInContext();
+      }
 
-        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()))
-            overridden = var;
+      VarDecl *overridden = nullptr;
+      if (containerTy) {
+        SmallVector<ValueDecl *, 2> lookup;
+        dc->lookupQualified(containerTy, name,
+                            NL_QualifiedDefault | NL_KnownNoDependency,
+                            Impl.getTypeResolver(), lookup);
+        for (auto result : lookup) {
+          if (isa<FuncDecl>(result) &&
+              result->isInstanceMember() == decl->isInstanceProperty() &&
+              result->getFullName().getArgumentNames().empty())
+            return nullptr;
+
+          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()))
+              overridden = var;
+          }
         }
       }
 
@@ -5531,7 +5542,9 @@ ConstructorDecl *SwiftDeclConverter::importConstructor(
                             ->getResult()
                             ->castTo<AnyFunctionType>()
                             ->getInput();
-  for (auto other : ownerNominal->lookupDirect(importedName.getDeclName())) {
+  bool ignoreNewExtensions = isa<ClassDecl>(dc);
+  for (auto other : ownerNominal->lookupDirect(importedName.getDeclName(),
+                                               ignoreNewExtensions)) {
     auto ctor = dyn_cast<ConstructorDecl>(other);
     if (!ctor || ctor->isInvalid() ||
         ctor->getAttrs().isUnavailable(Impl.SwiftContext) ||
@@ -7571,6 +7584,23 @@ ClangImporter::Implementation::loadAllMembers(Decl *D, uint64_t extra) {
     auto ext = cast<ExtensionDecl>(D);
     DC = ext;
     IDC = ext;
+
+
+    // If the base is also imported from Clang, load its members first.
+    const NominalTypeDecl *base = ext->getExtendedType()->getAnyNominal();
+    if (auto *clangBase = base->getClangDecl()) {
+      base->loadAllMembers();
+
+      // Sanity check: make sure we don't jump over to a category /while/
+      // loading the original class's members. Right now we only check if this
+      // happens on the first member.
+      if (auto *clangContainer = dyn_cast<clang::ObjCContainerDecl>(clangBase)){
+        if (!clangContainer->decls_empty()) {
+          assert(!base->getMembers().empty() &&
+                 "can't load extension members before base has finished");
+        }
+      }
+    }
   }
 
   ImportingEntityRAII Importing(*this);

test/ClangImporter/Inputs/custom-modules/ObjCParseExtras.h

@@ -26,6 +26,20 @@ __attribute__((objc_root_class))
 + (void)classRef:(id)obj doSomething:(SEL)selector;
 @end
 
+@interface PropertyAndMethodCollisionInOneClass
+- (void)object;
++ (void)classRef;
+@property (getter=getObject) id object;
+@property (class,getter=getClassRef) id classRef;
+@end
+
+@interface PropertyAndMethodReverseCollisionInOneClass
+@property (getter=getObject) id object;
+@property (class,getter=getClassRef) id classRef;
+- (void)object;
++ (void)classRef;
+@end
+
 @protocol PropertyProto
 @property id protoProp;
 @property(readonly) id protoPropRO;

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

@@ -0,0 +1,37 @@
+__attribute__((objc_root_class))
+@interface Base
+- (nonnull instancetype)init;
+@end
+
+@interface GenericClass<T>: Base
+@end
+
+@interface PropertiesInit : Base
+@property (readonly, nonnull) Base *nullabilityChange;
+@property (readonly, nonnull) GenericClass<Base *> *missingGenerics;
+@property (readonly, nonnull) Base *typeChange;
+@end
+
+@interface PropertiesNoInit : Base
+@property (readonly, nonnull) Base *nullabilityChange;
+@property (readonly, nonnull) GenericClass<Base *> *missingGenerics;
+@property (readonly, nonnull) Base *typeChange;
+@end
+
+@interface PropertiesInitCategory : Base
+@end
+
+@interface PropertiesInitCategory (Category)
+@property (readonly, nonnull) Base *nullabilityChange;
+@property (readonly, nonnull) GenericClass<Base *> *missingGenerics;
+@property (readonly, nonnull) Base *typeChange;
+@end
+
+@interface PropertiesNoInitCategory : Base
+@end
+
+@interface PropertiesNoInitCategory (Category)
+@property (readonly, nonnull) Base *nullabilityChange;
+@property (readonly, nonnull) GenericClass<Base *> *missingGenerics;
+@property (readonly, nonnull) Base *typeChange;
+@end

test/ClangImporter/Inputs/frameworks/PrivatelyReadwrite.framework/Modules/module.modulemap

@@ -0,0 +1,7 @@
+framework module PrivatelyReadwrite {
+  umbrella header "PrivatelyReadwrite.h"
+  module * {
+    export *
+  }
+  exclude header "Private.h"
+}

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

@@ -0,0 +1,28 @@
+#import <PrivatelyReadwrite/PrivatelyReadwrite.h>
+
+@interface PrivateSubclass : Base
+@end
+
+@interface PropertiesInit ()
+@property (readwrite, nullable) Base *nullabilityChange;
+@property (readwrite, nonnull) GenericClass *missingGenerics;
+@property (readwrite, nonnull) PrivateSubclass *typeChange;
+@end
+
+@interface PropertiesNoInit ()
+@property (readwrite, nullable) Base *nullabilityChange;
+@property (readwrite, nonnull) GenericClass *missingGenerics;
+@property (readwrite, nonnull) PrivateSubclass *typeChange;
+@end
+
+@interface PropertiesInitCategory ()
+@property (readwrite, nullable) Base *nullabilityChange;
+@property (readwrite, nonnull) GenericClass *missingGenerics;
+@property (readwrite, nonnull) PrivateSubclass *typeChange;
+@end
+
+@interface PropertiesNoInitCategory ()
+@property (readwrite, nullable) Base *nullabilityChange;
+@property (readwrite, nonnull) GenericClass *missingGenerics;
+@property (readwrite, nonnull) PrivateSubclass *typeChange;
+@end

test/ClangImporter/objc_diags.swift

@@ -6,5 +6,5 @@ import ObjectiveC
 
 func instanceMethod(_ b: B) {
   b.method(1, 2.5) // expected-error {{argument labels '(_:, _:)' do not match any available overloads}}
-  // expected-note @-1 {{overloads for 'method' exist with these partially matching parameter lists: (Int32, onExtB: Double), (Int32, with: Float), (Int32, withFloat: Float), (Int32, onExtA: Double), (Int32, onCat1: Double), (Int32, with: Double), (Int32, withDouble: Double)}}
+  // expected-note @-1 {{overloads for 'method' exist with these partially matching parameter lists: (Int32, with: Float), (Int32, withFloat: Float), (Int32, onExtB: Double), (Int32, with: Double), (Int32, withDouble: Double), (Int32, onExtA: Double), (Int32, onCat1: Double)}}
 }

test/ClangImporter/objc_parse.swift

@@ -403,6 +403,23 @@ func testPropertyAndMethodCollision(_ obj: PropertyAndMethodCollision,
   _ = value
 }
 
+func testPropertyAndMethodCollisionInOneClass(
+  obj: PropertyAndMethodCollisionInOneClass,
+  rev: PropertyAndMethodReverseCollisionInOneClass
+) {
+  obj.object = nil
+  obj.object()
+
+  type(of: obj).classRef = nil
+  type(of: obj).classRef()
+
+  rev.object = nil
+  rev.object()
+
+  type(of: rev).classRef = nil
+  type(of: rev).classRef()
+}
+
 func testSubscriptAndPropertyRedeclaration(_ obj: SubscriptAndProperty) {
   _ = obj.x
   obj.x = 5

test/ClangImporter/objc_redeclared_properties_incompatible.swift

@@ -0,0 +1,61 @@
+// RUN: not %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -F %S/Inputs/frameworks %s 2>&1 | %FileCheck %s
+
+// RUN: echo '#import <PrivatelyReadwrite/Private.h>' > %t.h
+// RUN: not %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -F %S/Inputs/frameworks -import-objc-header %t.h %s 2>&1 | %FileCheck %s
+
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -emit-pch -F %S/Inputs/frameworks -o %t.pch %t.h
+// RUN: not %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -F %S/Inputs/frameworks -import-objc-header %t.pch %s 2>&1 | %FileCheck %s
+
+// REQUIRES: objc_interop
+
+import PrivatelyReadwrite
+
+// In the original bug, it made a difference whether the type was instantiated;
+// it resulted in members being imported in a different order.
+func testWithInitializer() {
+  let obj = PropertiesInit()
+
+  let _: Int = obj.nullabilityChange
+  // CHECK: [[@LINE-1]]: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'
+
+  let _: Int = obj.typeChange
+  // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
+}
+
+func testWithoutInitializer(obj: PropertiesNoInit) {
+  let _: Int = obj.nullabilityChange
+  // CHECK: [[@LINE-1]]: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'
+
+  let _: Int = obj.typeChange
+  // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
+}
+
+func testCategoryWithInitializer() {
+  let obj = PropertiesInitCategory()
+
+  let _: Int = obj.nullabilityChange
+  // CHECK: [[@LINE-1]]: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'
+
+  let _: Int = obj.typeChange
+  // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
+}
+
+func testCategoryWithoutInitializer(obj: PropertiesNoInitCategory) {
+  let _: Int = obj.nullabilityChange
+  // CHECK: [[@LINE-1]]: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'
+
+  let _: Int = obj.typeChange
+  // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
+}