Merge pull request #72060 from DougGregor/import-extension-visibility

Implement proper visibility rules for imported extensions

Author: DougGregor

include/swift/AST/DiagnosticsSema.def

@@ -165,6 +165,12 @@ ERROR(init_candidate_inaccessible,none,
       "'%select{private|fileprivate|internal|package|@_spi|@_spi}1' protection level",
       (Type, AccessLevel))
 
+ERROR(candidate_from_missing_import,none,
+      "%0 %1 is not available due to missing import of defining module %2",
+      (DescriptiveDeclKind, DeclName, DeclName))
+NOTE(candidate_add_import,none,
+     "add import of module %0", (DeclName))
+
 ERROR(cannot_pass_rvalue_mutating_subelement,none,
       "cannot use mutating member on immutable value: %0",
       (StringRef))

include/swift/AST/NameLookup.h

@@ -628,6 +628,15 @@ SelfBounds getSelfBoundsFromWhereClause(
 /// given protocol or protocol extension.
 SelfBounds getSelfBoundsFromGenericSignature(const ExtensionDecl *extDecl);
 
+/// Determine whether the given declaration is visible to name lookup when
+/// found from the given module scope context.
+///
+/// Note that this routine does not check ASTContext::isAccessControlDisabled();
+/// that's left for the caller.
+bool declIsVisibleToNameLookup(
+    const ValueDecl *decl, const DeclContext *moduleScopeContext,
+    NLOptions options);
+
 namespace namelookup {
 
 /// The bridge between the legacy UnqualifiedLookupFactory and the new ASTScope

include/swift/Basic/Features.def

@@ -365,6 +365,10 @@ EXPERIMENTAL_FEATURE(ClosureIsolation, true)
 // Enable isolated(any) attribute on function types.
 CONDITIONALLY_SUPPRESSIBLE_EXPERIMENTAL_FEATURE(IsolatedAny, true)
 
+
+// Whether members of extensions respect the enclosing file's imports.
+EXPERIMENTAL_FEATURE(ExtensionImportVisibility, true)
+
 // Alias for IsolatedAny
 EXPERIMENTAL_FEATURE(IsolatedAny2, true)
 
@@ -377,6 +381,7 @@ EXPERIMENTAL_FEATURE(ObjCImplementation, true)
 // Enable @implementation on @_cdecl functions.
 EXPERIMENTAL_FEATURE(CImplementation, true)
 
+
 #undef EXPERIMENTAL_FEATURE_EXCLUDED_FROM_MODULE_INTERFACE
 #undef EXPERIMENTAL_FEATURE
 #undef UPCOMING_FEATURE

lib/AST/FeatureSet.cpp

@@ -683,6 +683,7 @@ static bool usesFeatureIsolatedAny(Decl *decl) {
   });
 }
 
+UNINTERESTING_FEATURE(ExtensionImportVisibility)
 UNINTERESTING_FEATURE(IsolatedAny2)
 
 static bool usesFeatureGlobalActorIsolatedTypesUsability(Decl *decl) {

lib/AST/ModuleNameLookup.cpp

@@ -126,6 +126,22 @@ class LookupVisibleDecls : public ModuleNameLookup<LookupVisibleDecls> {
 
 } // end anonymous namespace
 
+bool swift::declIsVisibleToNameLookup(
+    const ValueDecl *decl, const DeclContext *moduleScopeContext,
+    NLOptions options) {
+  // NL_IgnoreAccessControl only applies to the current module. If
+  // it applies here, the declaration is visible.
+  if ((options & NL_IgnoreAccessControl) &&
+      moduleScopeContext &&
+      moduleScopeContext->getParentModule() ==
+          decl->getDeclContext()->getParentModule())
+    return true;
+
+  bool includeUsableFromInline = options & NL_IncludeUsableFromInline;
+  return decl->isAccessibleFrom(moduleScopeContext, false,
+                                includeUsableFromInline);
+}
+
 template <typename LookupStrategy>
 void ModuleNameLookup<LookupStrategy>::lookupInModule(
     SmallVectorImpl<ValueDecl *> &decls,
@@ -151,7 +167,6 @@ void ModuleNameLookup<LookupStrategy>::lookupInModule(
 
   const size_t initialCount = decls.size();
   size_t currentCount = decls.size();
-  bool includeUsableFromInline = options & NL_IncludeUsableFromInline;
 
   auto updateNewDecls = [&](const DeclContext *moduleScopeContext) {
     if (decls.size() == currentCount)
@@ -165,13 +180,7 @@ void ModuleNameLookup<LookupStrategy>::lookupInModule(
         if (resolutionKind == ResolutionKind::MacrosOnly && !isa<MacroDecl>(VD))
           return true;
         if (respectAccessControl &&
-            // NL_IgnoreAccessControl applies only to the current module.
-            !((options & NL_IgnoreAccessControl) &&
-                moduleScopeContext &&
-                moduleScopeContext->getParentModule() ==
-                    VD->getDeclContext()->getParentModule()) &&
-            !VD->isAccessibleFrom(moduleScopeContext, false,
-                                  includeUsableFromInline))
+            !declIsVisibleToNameLookup(VD, moduleScopeContext, options))
           return true;
         return false;
       });

lib/AST/NameLookup.cpp

@@ -2285,8 +2285,17 @@ static bool isAcceptableLookupResult(const DeclContext *dc,
   if (!(options & NL_IgnoreAccessControl) &&
       !dc->getASTContext().isAccessControlDisabled()) {
     bool allowUsableFromInline = options & NL_IncludeUsableFromInline;
-    return decl->isAccessibleFrom(dc, /*forConformance*/ false,
-                                  allowUsableFromInline);
+    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 (decl->getDeclContext()->getParentModule() != dc->getParentModule() &&
+        dc->getASTContext().LangOpts.hasFeature(
+            Feature::ExtensionImportVisibility) &&
+        !decl->findImport(dc))
+      return false;
   }
 
   return true;

lib/Sema/CSDiagnostics.cpp

@@ -6187,7 +6187,65 @@ bool InaccessibleMemberFailure::diagnoseAsError() {
 
   auto loc = nameLoc.isValid() ? nameLoc.getStartLoc() : ::getLoc(anchor);
   auto accessLevel = Member->getFormalAccessScope().accessLevelForDiagnostics();
-  if (auto *CD = dyn_cast<ConstructorDecl>(Member)) {
+  bool suppressDeclHereNote = false;
+  if (accessLevel == AccessLevel::Public &&
+      !Member->findImport(getDC())) {
+    auto definingModule = Member->getDeclContext()->getParentModule();
+    emitDiagnosticAt(loc, diag::candidate_from_missing_import,
+                     Member->getDescriptiveKind(), Member->getName(),
+                     definingModule->getName());
+
+    auto enclosingSF = getDC()->getParentSourceFile();
+    SourceLoc bestLoc;
+    SourceManager &srcMgr = Member->getASTContext().SourceMgr;
+    for (auto item : enclosingSF->getTopLevelItems()) {
+      // If we found an import declaration, we want to insert after it.
+      if (auto importDecl =
+              dyn_cast_or_null<ImportDecl>(item.dyn_cast<Decl *>())) {
+        SourceLoc loc = importDecl->getEndLoc();
+        if (loc.isValid()) {
+          bestLoc = Lexer::getLocForEndOfLine(srcMgr, loc);
+        }
+
+        // Keep looking for more import declarations.
+        continue;
+      }
+
+      // If we got a location based on import declarations, we're done.
+      if (bestLoc.isValid())
+        break;
+
+      // For any other item, we want to insert before it.
+      SourceLoc loc = item.getStartLoc();
+      if (loc.isValid()) {
+        bestLoc = Lexer::getLocForStartOfLine(srcMgr, loc);
+        break;
+      }
+    }
+
+    if (bestLoc.isValid()) {
+      llvm::SmallString<64> importText;
+
+      // @_spi imports.
+      if (Member->isSPI()) {
+        auto spiGroups = Member->getSPIGroups();
+        if (!spiGroups.empty()) {
+          importText += "@_spi(";
+          importText += spiGroups[0].str();
+          importText += ") ";
+        }
+      }
+
+      importText += "import ";
+      importText += definingModule->getName().str();
+      importText += "\n";
+      emitDiagnosticAt(bestLoc, diag::candidate_add_import,
+                       definingModule->getName())
+        .fixItInsert(bestLoc, importText);
+    }
+
+    suppressDeclHereNote = true;
+  } else if (auto *CD = dyn_cast<ConstructorDecl>(Member)) {
     emitDiagnosticAt(loc, diag::init_candidate_inaccessible,
                      CD->getResultInterfaceType(), accessLevel)
         .highlight(nameLoc.getSourceRange());
@@ -6197,7 +6255,8 @@ bool InaccessibleMemberFailure::diagnoseAsError() {
         .highlight(nameLoc.getSourceRange());
   }
 
-  emitDiagnosticAt(Member, diag::decl_declared_here, Member);
+  if (!suppressDeclHereNote)
+    emitDiagnosticAt(Member, diag::decl_declared_here, Member);
   return true;
 }
 

test/NameLookup/Inputs/extensions_A.swift

@@ -0,0 +1,9 @@
+public struct X { }
+
+public protocol P { }
+
+public struct Y<T> { }
+
+extension Y: P where T: P { }
+
+public struct Z: P { }

test/NameLookup/Inputs/extensions_B.swift

@@ -0,0 +1,10 @@
+import extensions_A
+
+public extension X {
+  public func XinB() { }
+}
+
+public extension Y {
+  public func YinB() { }
+}
+

test/NameLookup/Inputs/extensions_C.swift

@@ -0,0 +1,11 @@
+import extensions_A
+import extensions_B
+
+public extension X {
+  public func XinC() { }
+}
+
+public extension Y {
+  public func YinC() { }
+}
+

test/NameLookup/extensions_transitive.swift

@@ -0,0 +1,16 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -o %t %S/Inputs/extensions_A.swift
+// RUN: %target-swift-frontend -emit-module -I %t -o %t %S/Inputs/extensions_B.swift
+// RUN: %target-swift-frontend -emit-module -I %t -o %t %S/Inputs/extensions_C.swift
+// RUN: %target-swift-frontend -typecheck %s -I %t -verify -enable-experimental-feature ExtensionImportVisibility
+
+import extensions_A
+import extensions_C
+// expected-note 2{{add import of module 'extensions_B'}}{{1-1=import extensions_B\n}}
+func test(x: X, y: Y<Z>) {
+  x.XinB() // expected-error{{instance method 'XinB()' is not available due to missing import of defining module 'extensions_B'}}
+  y.YinB() // expected-error{{instance method 'YinB()' is not available due to missing import of defining module 'extensions_B'}}
+
+  x.XinC()
+  y.YinC()
+}

test/SPI/client_reuse_spi.swift

@@ -6,7 +6,7 @@
 // RUN: %target-swift-frontend -typecheck -verify -verify-ignore-unknown -DLIB_C %s -I %t
 
 #if LIB_A
-
+// expected-note@-1{{add import of module 'A'}}{{1-1=@_spi(A) import A\n}}
 @_spi(A) public struct SecretStruct {
   @_spi(A) public func bar() {}
 }
@@ -22,7 +22,7 @@
 @_spi(B) import B
 
 var a = foo() // OK
-a.bar() // expected-error{{'bar' is inaccessible due to '@_spi' protection level}}
+a.bar() // expected-error{{instance method 'bar()' is not available due to missing import of defining module 'A'}}
 
 var b = SecretStruct() // expected-error{{cannot find 'SecretStruct' in scope}}