Implement proper visibility rules for imported extensions

If an extension isn't imported either directly or via a transitive
(`@_exported`) import, its members should not be visible to name
lookup. Implement this behavior behind the experimental flag
ExtensionImportVisibility.

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

@@ -625,6 +625,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

@@ -337,6 +337,9 @@ EXPERIMENTAL_FEATURE(BorrowingSwitch, 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)
+
 #undef EXPERIMENTAL_FEATURE_EXCLUDED_FROM_MODULE_INTERFACE
 #undef EXPERIMENTAL_FEATURE
 #undef UPCOMING_FEATURE

lib/AST/FeatureSet.cpp

@@ -649,6 +649,8 @@ static bool usesFeatureIsolatedAny(Decl *decl) {
   });
 }
 
+UNINTERESTING_FEATURE(ExtensionImportVisibility)
+
 // ----------------------------------------------------------------------------
 // MARK: - FeatureSet
 // ----------------------------------------------------------------------------

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

@@ -2271,8 +2271,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

@@ -6163,7 +6163,54 @@ 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) {
+    auto definingModule = Member->getDeclContext()->getParentModule();
+    emitDiagnosticAt(loc, diag::candidate_from_missing_import,
+                     Member->getDescriptiveKind(), Member->getName(),
+                     definingModule->getName());
+
+    if (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;
+        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());

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()
+}