Merge pull request #70566 from xymus/spi-reexport-swift-only

xymus · web-flow · commit 91c62de72d7f · 2024-01-03T09:33:34.000-08:00
Sema: Reexport SPI via Swift exported imports but not clang's
diff --git a/include/swift/AST/ImportCache.h b/include/swift/AST/ImportCache.h
@@ -56,10 +56,12 @@ class ImportSet final :
   unsigned HasHeaderImportModule : 1;
   unsigned NumTopLevelImports : 31;
   unsigned NumTransitiveImports;
+  unsigned NumTransitiveSwiftOnlyImports;
 
   ImportSet(bool hasHeaderImportModule,
             ArrayRef<ImportedModule> topLevelImports,
-            ArrayRef<ImportedModule> transitiveImports);
+            ArrayRef<ImportedModule> transitiveImports,
+            ArrayRef<ImportedModule> transitiveSwiftOnlyImports);
 
   ImportSet(const ImportSet &) = delete;
   void operator=(const ImportSet &) = delete;
@@ -73,7 +75,8 @@ class ImportSet final :
       ArrayRef<ImportedModule> topLevelImports);
 
   size_t numTrailingObjects(OverloadToken<ImportedModule>) const {
-    return NumTopLevelImports + NumTransitiveImports;
+    return NumTopLevelImports + NumTransitiveImports +
+           NumTransitiveSwiftOnlyImports;
   }
 
   /// This is a bit of a hack to make module name lookup work properly.
@@ -94,6 +97,12 @@ class ImportSet final :
             NumTransitiveImports};
   }
 
+  ArrayRef<ImportedModule> getTransitiveSwiftOnlyImports() const {
+    return {getTrailingObjects<ImportedModule>() +
+              NumTopLevelImports + NumTransitiveImports,
+            NumTransitiveSwiftOnlyImports};
+  }
+
   ArrayRef<ImportedModule> getAllImports() const {
       return {getTrailingObjects<ImportedModule>(),
               NumTopLevelImports + NumTransitiveImports};
@@ -115,6 +124,9 @@ class alignas(ImportedModule) ImportCache {
                             const ModuleDecl *,
                             const DeclContext *>,
                  ArrayRef<ImportPath::Access>> ShadowCache;
+  llvm::DenseMap<std::tuple<const ModuleDecl *,
+                            const DeclContext *>,
+                 bool> SwiftOnlyCache;
 
   ImportPath::Access EmptyAccessPath;
 
@@ -142,6 +154,12 @@ class alignas(ImportedModule) ImportCache {
     return !getAllVisibleAccessPaths(mod, dc).empty();
   }
 
+  /// Is `mod` imported from `dc` via a purely Swift access path?
+  /// Always returns false if `dc` is a non-Swift module and only takes
+  /// into account re-exports declared from Swift modules for transitive imports.
+  bool isImportedByViaSwiftOnly(const ModuleDecl *mod,
+                                const DeclContext *dc);
+
   /// Returns all access paths in 'mod' that are visible from 'dc' if we
   /// subtract imports of 'other'.
   ArrayRef<ImportPath::Access>
diff --git a/include/swift/AST/Module.h b/include/swift/AST/Module.h
@@ -1087,10 +1087,6 @@ class ModuleDecl
   /// Returns the associated clang module if one exists.
   const clang::Module *findUnderlyingClangModule() const;
 
-  /// Does this module or the underlying clang module defines export_as with
-  /// a value corresponding to the \p other module?
-  bool isExportedAs(const ModuleDecl *other) const;
-
   /// Returns a generator with the components of this module's full,
   /// hierarchical name.
   ///
diff --git a/lib/AST/ImportCache.cpp b/lib/AST/ImportCache.cpp
@@ -27,15 +27,21 @@ using namespace namelookup;
 
 ImportSet::ImportSet(bool hasHeaderImportModule,
                      ArrayRef<ImportedModule> topLevelImports,
-                     ArrayRef<ImportedModule> transitiveImports)
+                     ArrayRef<ImportedModule> transitiveImports,
+                     ArrayRef<ImportedModule> transitiveSwiftOnlyImports)
   : HasHeaderImportModule(hasHeaderImportModule),
     NumTopLevelImports(topLevelImports.size()),
-    NumTransitiveImports(transitiveImports.size()) {
+    NumTransitiveImports(transitiveImports.size()),
+    NumTransitiveSwiftOnlyImports(transitiveSwiftOnlyImports.size()) {
   auto buffer = getTrailingObjects<ImportedModule>();
   std::uninitialized_copy(topLevelImports.begin(), topLevelImports.end(),
                           buffer);
   std::uninitialized_copy(transitiveImports.begin(), transitiveImports.end(),
                           buffer + topLevelImports.size());
+  std::uninitialized_copy(transitiveSwiftOnlyImports.begin(),
+                          transitiveSwiftOnlyImports.end(),
+                          buffer + topLevelImports.size() +
+                          transitiveImports.size());
 
 #ifndef NDEBUG
   llvm::SmallDenseSet<ImportedModule, 8> unique;
@@ -47,6 +53,15 @@ ImportSet::ImportSet(bool hasHeaderImportModule,
     auto result = unique.insert(import).second;
     assert(result && "Duplicate imports in import set");
   }
+
+  unique.clear();
+  for (auto import : topLevelImports) {
+    unique.insert(import);
+  }
+  for (auto import : transitiveSwiftOnlyImports) {
+    auto result = unique.insert(import).second;
+    assert(result && "Duplicate imports in import set");
+  }
 #endif
 }
 
@@ -82,10 +97,14 @@ void ImportSet::dump() const {
 }
 
 static void collectExports(ImportedModule next,
-                           SmallVectorImpl<ImportedModule> &stack) {
+                           SmallVectorImpl<ImportedModule> &stack,
+                           bool onlySwiftExports) {
   SmallVector<ImportedModule, 4> exports;
   next.importedModule->getImportedModulesForLookup(exports);
   for (auto exported : exports) {
+    if (onlySwiftExports && exported.importedModule->isNonSwiftModule())
+      continue;
+
     if (next.accessPath.empty())
       stack.push_back(exported);
     else if (exported.accessPath.empty()) {
@@ -135,7 +154,7 @@ ImportCache::getImportSet(ASTContext &ctx,
 
   SmallVector<ImportedModule, 4> stack;
   for (auto next : topLevelImports) {
-    collectExports(next, stack);
+    collectExports(next, stack, /*onlySwiftExports*/false);
   }
 
   while (!stack.empty()) {
@@ -148,7 +167,26 @@ ImportCache::getImportSet(ASTContext &ctx,
     if (next.importedModule == headerImportModule)
       hasHeaderImportModule = true;
 
-    collectExports(next, stack);
+    collectExports(next, stack, /*onlySwiftExports*/false);
+  }
+
+  // Now collect transitive imports through Swift reexported imports only.
+  SmallVector<ImportedModule, 4> transitiveSwiftOnlyImports;
+  visited.clear();
+  stack.clear();
+  for (auto next : topLevelImports) {
+    if (!visited.insert(next).second)
+      continue;
+    collectExports(next, stack, /*onlySwiftExports*/true);
+  }
+
+  while (!stack.empty()) {
+    auto next = stack.pop_back_val();
+    if (!visited.insert(next).second)
+      continue;
+
+    transitiveSwiftOnlyImports.push_back(next);
+    collectExports(next, stack, /*onlySwiftExports*/true);
   }
 
   // Find the insert position again, in case the above traversal invalidated
@@ -157,12 +195,15 @@ ImportCache::getImportSet(ASTContext &ctx,
   if (ImportSet *result = ImportSets.FindNodeOrInsertPos(ID, InsertPos))
     return *result;
   
-  size_t bytes = ImportSet::totalSizeToAlloc<ImportedModule>(topLevelImports.size() + transitiveImports.size());
+  size_t bytes = ImportSet::totalSizeToAlloc<ImportedModule>(
+                            topLevelImports.size() + transitiveImports.size() +
+                            transitiveSwiftOnlyImports.size());
   void *mem = ctx.Allocate(bytes, alignof(ImportSet), AllocationArena::Permanent);
 
   auto *result = new (mem) ImportSet(hasHeaderImportModule,
                                      topLevelImports,
-                                     transitiveImports);
+                                     transitiveImports,
+                                     transitiveSwiftOnlyImports);
   ImportSets.InsertNode(result, InsertPos);
 
   return *result;
@@ -249,6 +290,36 @@ ImportCache::getAllVisibleAccessPaths(const ModuleDecl *mod,
   return result;
 }
 
+bool ImportCache::isImportedByViaSwiftOnly(const ModuleDecl *mod,
+                                           const DeclContext *dc) {
+  dc = dc->getModuleScopeContext();
+  if (dc->getParentModule()->isNonSwiftModule())
+    return false;
+
+  auto &ctx = mod->getASTContext();
+  auto key = std::make_pair(mod, dc);
+  auto found = SwiftOnlyCache.find(key);
+  if (found != SwiftOnlyCache.end()) {
+    if (ctx.Stats)
+      ++ctx.Stats->getFrontendCounters().ModuleVisibilityCacheHit;
+    return found->second;
+  }
+
+  if (ctx.Stats)
+    ++ctx.Stats->getFrontendCounters().ModuleVisibilityCacheMiss;
+
+  bool result = false;
+  for (auto next : getImportSet(dc).getTransitiveSwiftOnlyImports()) {
+    if (next.importedModule == mod) {
+      result = true;
+      break;
+    }
+  }
+
+  SwiftOnlyCache[key] = result;
+  return result;
+}
+
 ArrayRef<ImportPath::Access>
 ImportCache::getAllAccessPathsNotShadowedBy(const ModuleDecl *mod,
                                             const ModuleDecl *other,
@@ -307,7 +378,7 @@ ImportCache::getAllAccessPathsNotShadowedBy(const ModuleDecl *mod,
         accessPaths.push_back(next.accessPath);
     }
 
-    collectExports(next, stack);
+    collectExports(next, stack, /*onlySwiftExports*/false);
   }
 
   auto result = allocateArray(ctx, accessPaths);
diff --git a/lib/AST/Module.cpp b/lib/AST/Module.cpp
@@ -2716,14 +2716,6 @@ const clang::Module *ModuleDecl::findUnderlyingClangModule() const {
   return nullptr;
 }
 
-bool ModuleDecl::isExportedAs(const ModuleDecl *other) const {
-  auto clangModule = findUnderlyingClangModule();
-  if (!clangModule)
-    return false;
-
-  return other->getRealName().str() == clangModule->ExportAsModule;
-}
-
 void ModuleDecl::collectBasicSourceFileInfo(
     llvm::function_ref<void(const BasicSourceFileInfo &)> callback) const {
   for (const FileUnit *fileUnit : getFiles()) {
@@ -3519,8 +3511,8 @@ void SourceFile::lookupImportedSPIGroups(
   for (auto &import : *Imports) {
     if (import.options.contains(ImportFlags::SPIAccessControl) &&
         (importedModule == import.module.importedModule ||
-         (imports.isImportedBy(importedModule, import.module.importedModule) &&
-          importedModule->isExportedAs(import.module.importedModule)))) {
+         imports.isImportedByViaSwiftOnly(importedModule,
+                                       import.module.importedModule))) {
       spiGroups.insert(import.spiGroups.begin(), import.spiGroups.end());
     }
   }
diff --git a/lib/Serialization/SerializedModuleLoader.cpp b/lib/Serialization/SerializedModuleLoader.cpp
@@ -1725,7 +1725,8 @@ void SerializedASTFile::lookupImportedSPIGroups(
 
     if (dep.Import->importedModule == importedModule ||
         (imports.isImportedBy(importedModule, dep.Import->importedModule) &&
-         importedModule->isExportedAs(dep.Import->importedModule))) {
+         imports.isImportedByViaSwiftOnly(importedModule,
+                                          dep.Import->importedModule))) {
       spiGroups.insert(dep.spiGroups.begin(), dep.spiGroups.end());
     }
   }
diff --git a/test/SPI/accidental-reexport.swift b/test/SPI/accidental-reexport.swift
@@ -0,0 +1,51 @@
+/// Guard that we don't reexport the SPIs accidentally through the common
+/// reexported imports between clang modules.
+
+// RUN: %empty-directory(%t)
+// RUN: split-file %s %t
+
+// RUN: %target-swift-frontend -emit-module %t/Overlayed.swift -I %t -o %t
+// RUN: %target-swift-frontend -typecheck -verify %t/Client.swift -I %t
+// RUN: %target-swift-frontend -typecheck -verify %t/LowerClient.swift -I %t
+
+//--- module.modulemap
+module Overlayed {
+    header "Overlayed.h"
+}
+
+module Middle {
+    header "Middle.h"
+    export *
+}
+
+module LowerMiddle {
+    header "LowerMiddle.h"
+    export *
+}
+
+//--- Overlayed.h
+//--- Overlayed.swift
+@_exported import Overlayed
+
+public func funcPublic() {}
+
+@_spi(X)
+public func funcSPI() {}
+
+//--- Middle.h
+#include <Overlayed.h>
+
+//--- LowerMiddle.h
+#include <Middle.h>
+
+//--- Client.swift
+@_spi(X) import Middle
+
+funcPublic()
+funcSPI() // expected-error {{cannot find 'funcSPI' in scope}}
+
+//--- LowerClient.swift
+@_spi(X) import LowerMiddle
+
+funcPublic()
+funcSPI() // expected-error {{cannot find 'funcSPI' in scope}}
diff --git a/test/SPI/reexported-spi-groups.swift b/test/SPI/reexported-spi-groups.swift
@@ -55,7 +55,8 @@
 // RUN:   %t/Client_FileA.swift %t/Client_FileB.swift\
 // RUN:   -swift-version 5 -I %t -verify
 
-/// Test that SPIs aren't avaible from a reexport without export_as
+/// SPIs are now reexported by any `@_exported` from Swift modules, no matter
+/// if `export_as` is present or not.
 // RUN: %target-swift-frontend -typecheck \
 // RUN:   %t/NonExportedAsClient.swift \
 // RUN:   -swift-version 5 -I %t -verify
@@ -92,6 +93,8 @@ public func exportedPublicFunc() {}
 
 @_spi(X) public struct ExportedSpiType {}
 
+@_spi(O) public func exportedSpiFuncOtherGroup() {}
+
 //--- Exporter.swift
 
 @_exported import Exported
@@ -130,6 +133,7 @@ public func clientA() {
     exportedPublicFunc()
     exportedSpiFunc()
     exporterSpiFunc()
+    exportedSpiFuncOtherGroup() // expected-error {{cannot find 'exportedSpiFuncOtherGroup' in scope}}
 }
 
 @inlinable
@@ -159,7 +163,7 @@ public func clientB() {
 
 public func client() {
     exportedPublicFunc()
-    exportedSpiFunc() // expected-error {{cannot find 'exportedSpiFunc' in scope}}
+    exportedSpiFunc()
     exporterSpiFunc()
 }
 

Original file line number	Diff line number	Diff line change
`@@ -1725,7 +1725,8 @@ void SerializedASTFile::lookupImportedSPIGroups(`
`1725`	`1725`
`1726`	`1726`	`if (dep.Import->importedModule == importedModule \|\|`
`1727`	`1727`	`(imports.isImportedBy(importedModule, dep.Import->importedModule) &&`
`1728`		`- importedModule->isExportedAs(dep.Import->importedModule))) {`
	`1728`	`+ imports.isImportedByViaSwiftOnly(importedModule,`
	`1729`	`+ dep.Import->importedModule))) {`
`1729`	`1730`	`spiGroups.insert(dep.spiGroups.begin(), dep.spiGroups.end());`
`1730`	`1731`	`}`
`1731`	`1732`	`}`