Merge pull request #72974 from tshortli/extension-import-visibility-6.0

[6.0] Implement proper visibility rules for imported extensions

Author: tshortli

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/AST/SourceFile.h

@@ -418,6 +418,11 @@ class SourceFile final : public FileUnit {
     ImportedUnderlyingModule = module;
   }
 
+  /// Finds the import declaration that effectively imports a given module in
+  /// this source file.
+  std::optional<AttributedImport<ImportedModule>>
+  findImport(const ModuleDecl *mod) const;
+
   /// Whether the given import has used @preconcurrency.
   bool hasImportUsedPreconcurrency(
       AttributedImport<ImportedModule> import) const;

include/swift/AST/TypeCheckRequests.h

@@ -4845,6 +4845,27 @@ class ObjCRequirementMapRequest
   bool isCached() const { return true; }
 };
 
+/// Finds the import declaration that effectively imports a given module in a
+/// source file.
+class ImportDeclRequest
+    : public SimpleRequest<ImportDeclRequest,
+                           std::optional<AttributedImport<ImportedModule>>(
+                               const SourceFile *sf, const ModuleDecl *mod),
+                           RequestFlags::Cached> {
+public:
+  using SimpleRequest::SimpleRequest;
+
+private:
+  friend SimpleRequest;
+
+  std::optional<AttributedImport<ImportedModule>>
+  evaluate(Evaluator &evaluator, const SourceFile *sf,
+           const ModuleDecl *mod) const;
+
+public:
+  bool isCached() const { return true; }
+};
+
 #define SWIFT_TYPEID_ZONE TypeChecker
 #define SWIFT_TYPEID_HEADER "swift/AST/TypeCheckerTypeIDZone.def"
 #include "swift/Basic/DefineTypeIDZone.h"

include/swift/AST/TypeCheckerTypeIDZone.def

@@ -562,4 +562,7 @@ SWIFT_REQUEST(TypeChecker, LocalTypeDeclsRequest,
 SWIFT_REQUEST(TypeChecker, ObjCRequirementMapRequest,
               ObjCRequirementMap(const ProtocolDecl *proto),
               Cached, NoLocationInfo)
-
+SWIFT_REQUEST(TypeChecker, ImportDeclRequest,
+              std::optional<AttributedImport<ImportedModule>>(
+                  const SourceFile *sf, const ModuleDecl *mod),
+              Cached, NoLocationInfo)

include/swift/Basic/Features.def

@@ -364,6 +364,9 @@ 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_EXCLUDED_FROM_MODULE_INTERFACE(ExtensionImportVisibility, true)
+
 // Alias for IsolatedAny
 EXPERIMENTAL_FEATURE(IsolatedAny2, true)
 

lib/AST/Decl.cpp

@@ -3853,24 +3853,7 @@ ValueDecl::findImport(const DeclContext *fromDC) {
   if (!fromSourceFile)
     return std::nullopt;
 
-  // Look to see if the owning module was directly imported.
-  for (const auto &import : fromSourceFile->getImports()) {
-    if (import.module.importedModule == module)
-      return import;
-  }
-
-  // Now look for transitive imports.
-  auto &importCache = getASTContext().getImportCache();
-  for (const auto &import : fromSourceFile->getImports()) {
-    auto &importSet = importCache.getImportSet(import.module.importedModule);
-    for (const auto &transitive : importSet.getTransitiveImports()) {
-      if (transitive.importedModule == module) {
-        return import;
-      }
-    }
-  }
-
-  return std::nullopt;
+  return fromSourceFile->findImport(module);
 }
 
 bool ValueDecl::isProtocolRequirement() const {

lib/AST/FeatureSet.cpp

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

lib/AST/Module.cpp

@@ -2580,6 +2580,38 @@ SourceFile::setImports(ArrayRef<AttributedImport<ImportedModule>> imports) {
   Imports = getASTContext().AllocateCopy(imports);
 }
 
+std::optional<AttributedImport<ImportedModule>>
+SourceFile::findImport(const ModuleDecl *module) const {
+  return evaluateOrDefault(getASTContext().evaluator,
+                           ImportDeclRequest{this, module}, std::nullopt);
+}
+
+std::optional<AttributedImport<ImportedModule>>
+ImportDeclRequest::evaluate(Evaluator &evaluator, const SourceFile *sf,
+                            const ModuleDecl *module) const {
+  auto &ctx = sf->getASTContext();
+  auto imports = sf->getImports();
+
+  // Look to see if the owning module was directly imported.
+  for (const auto &import : imports) {
+    if (import.module.importedModule == module)
+      return import;
+  }
+
+  // Now look for transitive imports.
+  auto &importCache = ctx.getImportCache();
+  for (const auto &import : imports) {
+    auto &importSet = importCache.getImportSet(import.module.importedModule);
+    for (const auto &transitive : importSet.getTransitiveImports()) {
+      if (transitive.importedModule == module) {
+        return import;
+      }
+    }
+  }
+
+  return std::nullopt;
+}
+
 bool SourceFile::hasImportUsedPreconcurrency(
     AttributedImport<ImportedModule> import) const {
   return PreconcurrencyImportsUsed.count(import) != 0;

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;
 }
 

stdlib/cmake/modules/SwiftSource.cmake

@@ -628,6 +628,8 @@ function(_compile_swift_files
     list(APPEND swift_flags "-enable-experimental-feature" "NonescapableTypes")
   endif()
 
+  list(APPEND swift_flags "-enable-experimental-feature" "ExtensionImportVisiblity")
+
   if (SWIFT_STDLIB_ENABLE_STRICT_CONCURRENCY_COMPLETE)
     list(APPEND swift_flags "-strict-concurrency=complete")
   endif()

test/NameLookup/Inputs/Categories/Categories_A.h

@@ -0,0 +1,8 @@
+@import Foundation;
+
+@interface X
+@end
+
+@interface X (A)
+- (void)fromA;
+@end

test/NameLookup/Inputs/Categories/Categories_A.swift

@@ -0,0 +1,6 @@
+@_exported import Categories_A
+
+extension X {
+  public func fromOverlayForA() {}
+  @objc public func fromOverlayForAObjC() {}
+}

test/NameLookup/Inputs/Categories/Categories_B.h

@@ -0,0 +1,5 @@
+@import Categories_A;
+
+@interface X (B)
+- (void)fromB;
+@end

test/NameLookup/Inputs/Categories/Categories_B.swift

@@ -0,0 +1,6 @@
+@_exported import Categories_B
+
+extension X {
+  public func fromOverlayForB() {}
+  @objc public func fromOverlayForBObjC() {}
+}

test/NameLookup/Inputs/Categories/Categories_C.h

@@ -0,0 +1,5 @@
+@import Categories_A;
+
+@interface X (C)
+- (void)fromC;
+@end

test/NameLookup/Inputs/Categories/Categories_C.swift

@@ -0,0 +1,6 @@
+@_exported import Categories_C
+
+extension X {
+  public func fromOverlayForC() {}
+  @objc public func fromOverlayForCObjC() {}
+}

test/NameLookup/Inputs/Categories/Categories_D.h

@@ -0,0 +1 @@
+// Intentionally empty

test/NameLookup/Inputs/Categories/Categories_E.swift

@@ -0,0 +1,2 @@
+import Categories_C
+import Categories_D.Submodule

test/NameLookup/Inputs/Categories/Submodule.h

@@ -0,0 +1,5 @@
+@import Categories_A;
+
+@interface X (SubmoduleOfD)
+- (void)fromSubmoduleOfD;
+@end

test/NameLookup/Inputs/Categories/module.modulemap

@@ -0,0 +1,25 @@
+module Categories_A {
+  header "Categories_A.h"
+  export *
+}
+
+module Categories_B {
+  header "Categories_B.h"
+  export *
+}
+
+module Categories_C {
+  header "Categories_C.h"
+  export *
+}
+
+module Categories_D {
+  header "Categories_D.h"
+  export *
+
+  explicit module Submodule {
+    header "Submodule.h"
+    export *
+  }
+}
+