[6.2] AST: Filter out some Obj-C overrides when MemberImportVisibility is enabled

lib/AST/NameLookup.cpp

@@ -323,23 +323,16 @@ bool swift::removeOverriddenDecls(SmallVectorImpl<ValueDecl*> &decls) {
     }
 
     while (auto overrides = decl->getOverriddenDecl()) {
-      overridden.insert(overrides);
-
-      // Because initializers from Objective-C base classes have greater
-      // visibility than initializers written in Swift classes, we can
-      // have a "break" in the set of declarations we found, where
-      // C.init overrides B.init overrides A.init, but only C.init and
-      // A.init are in the chain. Make sure we still remove A.init from the
-      // set in this case.
-      if (decl->getBaseName().isConstructor()) {
-        /// FIXME: Avoid the possibility of an infinite loop by fixing the root
-        ///        cause instead (incomplete circularity detection).
-        assert(decl != overrides && "Circular class inheritance?");
-        decl = overrides;
-        continue;
+      if (!overridden.insert(overrides).second) {
+        // If we've already seen a decl then there's no need to visit the decls
+        // that it overrides since they should already be in the set. This also
+        // prevents infinite loops in the case that the AST contains an
+        // override chain with a cycle due to circular inheritance.
+        break;
       }
 
-      break;
+      DEBUG_ASSERT(decl != overrides && "Circular class inheritance?");
+      decl = overrides;
     }
   }
 
@@ -2388,6 +2381,8 @@ static bool isAcceptableLookupResult(const DeclContext *dc, NLOptions options,
                                      ValueDecl *decl,
                                      bool onlyCompleteObjectInits,
                                      bool requireImport) {
+  auto &ctx = dc->getASTContext();
+
   // Filter out designated initializers, if requested.
   if (onlyCompleteObjectInits) {
     if (auto ctor = dyn_cast<ConstructorDecl>(decl)) {
@@ -2405,19 +2400,43 @@ static bool isAcceptableLookupResult(const DeclContext *dc, NLOptions options,
   }
 
   // Check access.
-  if (!(options & NL_IgnoreAccessControl) &&
-      !dc->getASTContext().isAccessControlDisabled()) {
+  if (!(options & NL_IgnoreAccessControl) && !ctx.isAccessControlDisabled()) {
     bool allowUsableFromInline = options & NL_IncludeUsableFromInline;
     if (!decl->isAccessibleFrom(dc, /*forConformance*/ false,
                                 allowUsableFromInline))
       return false;
   }
 
-  // Check that there is some import in the originating context that makes this
-  // decl visible.
-  if (requireImport && !(options & NL_IgnoreMissingImports))
-    if (!dc->isDeclImported(decl))
-      return false;
+  if (requireImport) {
+    // Check that there is some import in the originating context that makes
+    // this decl visible.
+    if (!(options & NL_IgnoreMissingImports)) {
+      if (!dc->isDeclImported(decl))
+        return false;
+    }
+
+    // Unlike in Swift, Obj-C allows method overrides to be declared in
+    // extensions (categories), even outside of the module that defines the
+    // type that is being extended. When MemberImportVisibility is enabled,
+    // if these overrides are not filtered out they can hijack name
+    // lookup and cause the compiler to insist that the module that defines
+    // the extension be imported, contrary to developer expectations.
+    //
+    // Filter results belonging to these extensions out, even when ignoring
+    // missing imports, if we're in a context that requires imports to access
+    // member declarations.
+    if (decl->getOverriddenDecl()) {
+      if (auto *extension = dyn_cast<ExtensionDecl>(decl->getDeclContext())) {
+        if (auto *nominal = extension->getExtendedNominal()) {
+          auto extensionMod = extension->getModuleContext();
+          auto nominalMod = nominal->getModuleContext();
+          if (!extensionMod->isSameModuleLookingThroughOverlays(nominalMod) &&
+              !dc->isDeclImported(extension))
+            return false;
+        }
+      }
+    }
+  }
 
   // Check that it has the appropriate ABI role.
   if (!ABIRoleInfo(decl).matchesOptions(options))

lib/Sema/TypeCheckNameLookup.cpp

@@ -835,7 +835,7 @@ missingImportsForDefiningModule(ModuleDecl *owningModule, SourceFile &sf) {
   }
 
   std::sort(result.begin(), result.end(), [](ModuleDecl *LHS, ModuleDecl *RHS) {
-    return LHS->getNameStr() < LHS->getNameStr();
+    return LHS->getNameStr() < RHS->getNameStr();
   });
 
   return result;

test/CrossImport/member-import-visibility.swift

@@ -39,8 +39,8 @@ import Swift
 // expected-note@-1 2 {{add import of module 'DeclaringLibrary'}}
 
 private func test() {
-  returnsDeclaringTy().overlayMember() // expected-error {{instance method 'overlayMember()' is not available due to missing imports of defining modules 'DeclaringLibrary' and 'BystandingLibrary'}}
-  returnsBystandingTy().overlayMember() // expected-error {{instance method 'overlayMember()' is not available due to missing imports of defining modules 'DeclaringLibrary' and 'BystandingLibrary'}}
+  returnsDeclaringTy().overlayMember() // expected-error {{instance method 'overlayMember()' is not available due to missing imports of defining modules 'BystandingLibrary' and 'DeclaringLibrary'}}
+  returnsBystandingTy().overlayMember() // expected-error {{instance method 'overlayMember()' is not available due to missing imports of defining modules 'BystandingLibrary' and 'DeclaringLibrary'}}
 }
 
 //--- BothDeclaringAndBystanding.swift

test/NameLookup/Inputs/MemberImportVisibility/Categories_A.h

@@ -1,6 +1,13 @@
 @import Foundation;
 
-@interface X
+@interface Base
+- (void)overriddenInOverlayForA __attribute__((deprecated("Categories_A.h")));
+- (void)overriddenInOverlayForB __attribute__((deprecated("Categories_A.h")));
+- (void)overriddenInOverlayForC __attribute__((deprecated("Categories_A.h")));
+- (void)overriddenInSubclassInOverlayForC __attribute__((deprecated("Categories_A.h")));
+@end
+
+@interface X : Base
 @end
 
 @interface X (A)

test/NameLookup/Inputs/MemberImportVisibility/Categories_A.swift

@@ -3,4 +3,7 @@
 extension X {
   public func fromOverlayForA() {}
   @objc public func fromOverlayForAObjC() {}
+
+  @available(*, deprecated, message: "Categories_A.swift")
+  public override func overriddenInOverlayForA() {}
 }

test/NameLookup/Inputs/MemberImportVisibility/Categories_B.swift

@@ -3,4 +3,7 @@
 extension X {
   public func fromOverlayForB() {}
   @objc public func fromOverlayForBObjC() {}
+
+  @available(*, deprecated, message: "Categories_B.swift")
+  public override func overriddenInOverlayForB() {}
 }

test/NameLookup/Inputs/MemberImportVisibility/Categories_C.h

@@ -3,3 +3,7 @@
 @interface X (C)
 - (void)fromC;
 @end
+
+@interface SubclassFromC : X
+- (instancetype)init;
+@end

test/NameLookup/Inputs/MemberImportVisibility/Categories_C.swift

@@ -3,4 +3,12 @@
 extension X {
   public func fromOverlayForC() {}
   @objc public func fromOverlayForCObjC() {}
+
+  @available(*, deprecated, message: "Categories_C.swift")
+  public override func overriddenInOverlayForC() {}
+}
+
+extension SubclassFromC {
+  @available(*, deprecated, message: "Categories_C.swift")
+  public override func overriddenInSubclassInOverlayForC() {}
 }

test/NameLookup/Inputs/MemberImportVisibility/Categories_E.swift

@@ -1,2 +1,4 @@
 import Categories_C
 import Categories_D.Submodule
+
+public func makeSubclassFromC() -> SubclassFromC { SubclassFromC() }

test/NameLookup/Inputs/MemberImportVisibility/ObjCOverloads/Branch.h

@@ -0,0 +1,9 @@
+@import Root;
+
+@interface BranchObject : RootObject
+- (void)overridden1 __attribute__((deprecated("Branch.h")));
+@end
+
+@interface BranchObject (Branch)
+- (void)overridden3 __attribute__((deprecated("Branch.h")));
+@end

test/NameLookup/Inputs/MemberImportVisibility/ObjCOverloads/Fruit.h

@@ -0,0 +1,9 @@
+@import Branch;
+
+@interface FruitObject : BranchObject
+- (void)overridden1 __attribute__((deprecated("Fruit.h")));
+@end
+
+@interface FruitObject (Fruit)
+- (void)overridden4 __attribute__((deprecated("Fruit.h")));
+@end

test/NameLookup/Inputs/MemberImportVisibility/ObjCOverloads/Leaf.h

@@ -0,0 +1,15 @@
+@import Branch;
+
+@interface LeafObject : BranchObject
+- (void)overridden1 __attribute__((deprecated("Leaf.h")));
+@end
+
+@interface BranchObject (Leaf)
+- (void)overridden2 __attribute__((deprecated("Leaf.h")));
+- (void)overridden4 __attribute__((deprecated("Leaf.h")));
+@end
+
+@interface LeafObject (Leaf)
+- (void)overridden3 __attribute__((deprecated("Leaf.h")));
+@end
+

test/NameLookup/Inputs/MemberImportVisibility/ObjCOverloads/Root.h

@@ -0,0 +1,10 @@
+@interface RootObject
+- (instancetype)init;
+- (void)overridden1 __attribute__((deprecated("Root.h")));
+- (void)overridden2 __attribute__((deprecated("Root.h")));
+@end
+
+@interface RootObject (Root)
+- (void)overridden3 __attribute__((deprecated("Root.h")));
+- (void)overridden4 __attribute__((deprecated("Root.h")));
+@end

test/NameLookup/Inputs/MemberImportVisibility/ObjCOverloads/module.modulemap

@@ -0,0 +1,19 @@
+module Root {
+  header "Root.h"
+  export *
+}
+
+module Branch {
+  header "Branch.h"
+  export *
+}
+
+module Leaf {
+  header "Leaf.h"
+  export *
+}
+
+module Fruit {
+  header "Fruit.h"
+  export *
+}

test/NameLookup/Inputs/MemberImportVisibility/members_A.swift

@@ -35,8 +35,8 @@ public enum EnumInA {
 }
 
 open class BaseClassInA {
-  public init() {}
   open func methodInA() {}
+  open func overriddenMethod() {}
 }
 
 public protocol ProtocolInA {

test/NameLookup/Inputs/MemberImportVisibility/members_B.swift

@@ -37,6 +37,7 @@ package enum EnumInB_package {
 
 open class DerivedClassInB: BaseClassInA {
   open func methodInB() {}
+  open override func overriddenMethod() {}
 }
 
 extension ProtocolInA {

test/NameLookup/Inputs/MemberImportVisibility/members_C.swift

@@ -31,6 +31,8 @@ public enum EnumInC {
 
 open class DerivedClassInC: DerivedClassInB {
   open func methodInC() {}
+  open override func overriddenMethod() {}
+  public func asDerivedClassInB() -> DerivedClassInB { return self }
 }
 
 extension ProtocolInA {

test/NameLookup/members_transitive.swift

@@ -9,7 +9,7 @@
 // REQUIRES: swift_feature_MemberImportVisibility
 
 import members_C
-// expected-member-visibility-note 16{{add import of module 'members_B'}}{{1-1=internal import members_B\n}}
+// expected-member-visibility-note 18{{add import of module 'members_B'}}{{1-1=internal import members_B\n}}
 
 
 func testExtensionMembers(x: X, y: Y<Z>) {
@@ -96,8 +96,22 @@ class DerivedFromClassInC: DerivedClassInC {
 
 struct ConformsToProtocolInA: ProtocolInA {} // expected-member-visibility-error{{type 'ConformsToProtocolInA' does not conform to protocol 'ProtocolInA'}} expected-member-visibility-note {{add stubs for conformance}}
 
-func testDerivedMethodAccess() {
-  DerivedClassInC().methodInC()
-  DerivedClassInC().methodInB() // expected-member-visibility-error{{instance method 'methodInB()' is not available due to missing import of defining module 'members_B'}}
-  DerivedFromClassInC().methodInB()
+func testInheritedMethods(
+  a: BaseClassInA,
+  c: DerivedClassInC,
+) {
+  let b = c.asDerivedClassInB()
+
+  a.methodInA()
+  b.methodInA()
+  c.methodInA()
+
+  b.methodInB() // expected-member-visibility-error{{instance method 'methodInB()' is not available due to missing import of defining module 'members_B'}}
+  c.methodInB() // expected-member-visibility-error{{instance method 'methodInB()' is not available due to missing import of defining module 'members_B'}}
+
+  c.methodInC()
+
+  a.overriddenMethod()
+  b.overriddenMethod() // expected-member-visibility-error{{instance method 'overriddenMethod()' is not available due to missing import of defining module 'members_B'}}
+  c.overriddenMethod()
 }

test/NameLookup/members_transitive_objc.swift

@@ -3,8 +3,8 @@
 // RUN: %target-swift-frontend -emit-module -I %t -I %S/Inputs/MemberImportVisibility -o %t %S/Inputs/MemberImportVisibility/Categories_B.swift
 // RUN: %target-swift-frontend -emit-module -I %t -I %S/Inputs/MemberImportVisibility -o %t %S/Inputs/MemberImportVisibility/Categories_C.swift
 // RUN: %target-swift-frontend -emit-module -I %t -I %S/Inputs/MemberImportVisibility -o %t %S/Inputs/MemberImportVisibility/Categories_E.swift
-// RUN: %target-swift-frontend -typecheck %s -I %t -I %S/Inputs/MemberImportVisibility -import-objc-header %S/Inputs/MemberImportVisibility/Bridging.h -verify -swift-version 5
-// RUN: %target-swift-frontend -typecheck %s -I %t -I %S/Inputs/MemberImportVisibility -import-objc-header %S/Inputs/MemberImportVisibility/Bridging.h -verify -swift-version 6
+// RUN: %target-swift-frontend -typecheck %s -I %t -I %S/Inputs/MemberImportVisibility -import-objc-header %S/Inputs/MemberImportVisibility/Bridging.h -verify -swift-version 5 -verify-additional-prefix no-member-visibility-
+// RUN: %target-swift-frontend -typecheck %s -I %t -I %S/Inputs/MemberImportVisibility -import-objc-header %S/Inputs/MemberImportVisibility/Bridging.h -verify -swift-version 6 -verify-additional-prefix no-member-visibility-
 // RUN: %target-swift-frontend -typecheck %s -I %t -I %S/Inputs/MemberImportVisibility -import-objc-header %S/Inputs/MemberImportVisibility/Bridging.h -verify -swift-version 5 -enable-upcoming-feature MemberImportVisibility -verify-additional-prefix member-visibility-
 
 // REQUIRES: objc_interop
@@ -13,30 +13,43 @@
 import Categories_B
 import Categories_E
 
-// expected-member-visibility-note@-1 2 {{add import of module 'Categories_C'}}{{1-1=internal import Categories_C\n}}
+// expected-member-visibility-note@-1 3 {{add import of module 'Categories_C'}}{{1-1=internal import Categories_C\n}}
 // expected-member-visibility-note@-2 {{add import of module 'Categories_D'}}{{1-1=internal import Categories_D\n}}
 func test(x: X) {
   x.fromA()
   x.fromOverlayForA()
+  x.overriddenInOverlayForA() // expected-warning {{'overriddenInOverlayForA()' is deprecated: Categories_A.swift}}
   x.fromB()
   x.fromOverlayForB()
+  x.overriddenInOverlayForB() // expected-warning {{'overriddenInOverlayForB()' is deprecated: Categories_B.swift}}
   x.fromC() // expected-member-visibility-error {{instance method 'fromC()' is not available due to missing import of defining module 'Categories_C'}}
   x.fromOverlayForC() // expected-member-visibility-error {{instance method 'fromOverlayForC()' is not available due to missing import of defining module 'Categories_C'}}
+  x.overriddenInOverlayForC()
+  // expected-no-member-visibility-warning@-1 {{'overriddenInOverlayForC()' is deprecated: Categories_C.swift}}
+  // expected-member-visibility-warning@-2 {{'overriddenInOverlayForC()' is deprecated: Categories_A.h}}
   x.fromSubmoduleOfD() // expected-member-visibility-error {{instance method 'fromSubmoduleOfD()' is not available due to missing import of defining module 'Categories_D'}}
   x.fromBridgingHeader()
   x.overridesCategoryMethodOnNSObject()
+
+  let subclassFromC = makeSubclassFromC()
+  subclassFromC.overriddenInSubclassInOverlayForC()
+  // expected-warning@-1 {{'overriddenInSubclassInOverlayForC()' is deprecated: Categories_C.swift}}
+  // expected-member-visibility-error@-2 {{instance method 'overriddenInSubclassInOverlayForC()' is not available due to missing import of defining module 'Categories_C'}}
 }
 
 func testAnyObject(a: AnyObject) {
   a.fromA()
   a.fromOverlayForAObjC()
+  a.overriddenInOverlayForA() // expected-warning {{'overriddenInOverlayForA()' is deprecated: Categories_A.h}}
   a.fromB()
   a.fromOverlayForBObjC()
+  a.overriddenInOverlayForB() // expected-warning {{'overriddenInOverlayForB()' is deprecated: Categories_A.h}}
   // FIXME: Better diagnostics?
   // Name lookup for AnyObject already ignored transitive imports, so
   // `MemberImportVisibility` has no effect on these diagnostics.
   a.fromC() // expected-error {{value of type 'AnyObject' has no member 'fromC'}}
   a.fromOverlayForCObjC() // expected-error {{value of type 'AnyObject' has no member 'fromOverlayForCObjC'}}
+  a.overriddenInOverlayForC() // expected-warning {{'overriddenInOverlayForC()' is deprecated: Categories_A.h}}
   a.fromBridgingHeader()
   a.overridesCategoryMethodOnNSObject()
 }