Merge pull request #30819 from brentdax/a-less-ambitious-crossover-event

Ignore transitive ObjC imports when cross-importing

Author: beccadax

include/swift/AST/ClangModuleLoader.h

@@ -128,7 +128,9 @@ class ClangModuleLoader : public ModuleLoader {
 
   /// Retrieves the Swift wrapper for the given Clang module, creating
   /// it if necessary.
-  virtual ModuleDecl *getWrapperForModule(const clang::Module *mod) const = 0;
+  virtual ModuleDecl *
+  getWrapperForModule(const clang::Module *mod,
+                      bool returnOverlayIfPossible = false) const = 0;
 
   /// Adds a new search path to the Clang CompilerInstance, as if specified with
   /// -I or -F.

include/swift/AST/Module.h

@@ -465,7 +465,7 @@ class ModuleDecl : public DeclContext, public TypeDecl {
   /// Retrieve the top-level module. If this module is already top-level, this
   /// returns itself. If this is a submodule such as \c Foo.Bar.Baz, this
   /// returns the module \c Foo.
-  ModuleDecl *getTopLevelModule();
+  ModuleDecl *getTopLevelModule(bool overlay = false);
 
   bool isResilient() const {
     return getResilienceStrategy() != ResilienceStrategy::Default;

include/swift/ClangImporter/ClangImporter.h

@@ -330,7 +330,9 @@ class ClangImporter final : public ClangModuleLoader {
 
   /// Retrieves the Swift wrapper for the given Clang module, creating
   /// it if necessary.
-  ModuleDecl *getWrapperForModule(const clang::Module *mod) const override;
+  ModuleDecl *
+  getWrapperForModule(const clang::Module *mod,
+                      bool returnOverlayIfPossible = false) const override;
 
   std::string getBridgingHeaderContents(StringRef headerPath, off_t &fileSize,
                                         time_t &fileModTime);

lib/AST/Module.cpp

@@ -514,15 +514,16 @@ SourceLookupCache &ModuleDecl::getSourceLookupCache() const {
   return *Cache;
 }
 
-ModuleDecl *ModuleDecl::getTopLevelModule() {
+ModuleDecl *ModuleDecl::getTopLevelModule(bool overlay) {
   // If this is a Clang module, ask the Clang importer for the top-level module.
   // We need to check isNonSwiftModule() to ensure we don't look through
   // overlays.
   if (isNonSwiftModule()) {
     if (auto *underlying = findUnderlyingClangModule()) {
       auto &ctx = getASTContext();
       auto *clangLoader = ctx.getClangModuleLoader();
-      return clangLoader->getWrapperForModule(underlying->getTopLevelModule());
+      return clangLoader->getWrapperForModule(underlying->getTopLevelModule(),
+                                              overlay);
     }
   }
   // Swift modules don't currently support submodules.

lib/ClangImporter/ClangImporter.cpp

@@ -1860,8 +1860,13 @@ ModuleDecl *ClangImporter::getImportedHeaderModule() const {
   return Impl.ImportedHeaderUnit->getParentModule();
 }
 
-ModuleDecl *ClangImporter::getWrapperForModule(const clang::Module *mod) const {
-  return Impl.getWrapperForModule(mod)->getParentModule();
+ModuleDecl *
+ClangImporter::getWrapperForModule(const clang::Module *mod,
+                                   bool returnOverlayIfPossible) const {
+  auto clangUnit = Impl.getWrapperForModule(mod);
+  if (returnOverlayIfPossible && clangUnit->getOverlayModule())
+    return clangUnit->getOverlayModule();
+  return clangUnit->getParentModule();
 }
 
 PlatformAvailability::PlatformAvailability(LangOptions &langOpts)

lib/Sema/ImportResolution.cpp

@@ -172,15 +172,13 @@ class ImportResolver final : public DeclVisitor<ImportResolver> {
   /// The list of fully bound imports.
   SmallVector<ImportedModuleDesc, 16> boundImports;
 
-  /// All imported modules, including by re-exports, and including submodules.
-  llvm::DenseSet<ImportedModuleDesc> visibleModules;
-
-  /// \c visibleModules but without the submodules.
+  /// All imported modules which should be considered when cross-importing.
+  /// This is basically the transitive import graph, but with only top-level
+  /// modules and without reexports from Objective-C modules.
   ///
   /// We use a \c SmallSetVector here because this doubles as the worklist for
   /// cross-importing, so we want to keep it in order; this is feasible
-  /// because this set is usually fairly small, while \c visibleModules is
-  /// often enormous.
+  /// because this set is usually fairly small.
   SmallSetVector<ImportedModuleDesc, 64> crossImportableModules;
 
   /// The subset of \c crossImportableModules which may declare cross-imports.
@@ -223,7 +221,7 @@ class ImportResolver final : public DeclVisitor<ImportResolver> {
 
   /// Adds \p desc and everything it re-exports to \c visibleModules using
   /// the settings from \c desc.
-  void addVisibleModules(ImportedModuleDesc desc);
+  void addCrossImportableModules(ImportedModuleDesc desc);
 
   /// * If \p I is a cross-import overlay, registers \p M as overlaying
   ///   \p I.underlyingModule in \c SF.
@@ -341,7 +339,7 @@ void ImportResolver::bindImport(UnboundImport &&I) {
 
 void ImportResolver::addImport(const UnboundImport &I, ModuleDecl *M) {
   auto importDesc = I.makeDesc(M);
-  addVisibleModules(importDesc);
+  addCrossImportableModules(importDesc);
   boundImports.push_back(importDesc);
 }
 
@@ -936,7 +934,7 @@ static bool isSubmodule(ModuleDecl* M) {
   return clangMod && clangMod->Parent;
 }
 
-void ImportResolver::addVisibleModules(ImportedModuleDesc importDesc) {
+void ImportResolver::addCrossImportableModules(ImportedModuleDesc importDesc) {
   // FIXME: namelookup::getAllImports() doesn't quite do what we need (mainly
   // w.r.t. scoped imports), but it seems like we could extend it to do so, and
   // then eliminate most of this.
@@ -953,19 +951,34 @@ void ImportResolver::addVisibleModules(ImportedModuleDesc importDesc) {
                                       nextImport.first))
       continue;
 
+    // If we are importing a submodule, treat it as though we imported its
+    // top-level module (or rather, the top-level module's clang overlay if it
+    // has one).
+    if (isSubmodule(nextImport.second)) {
+      nextImport.second =
+          nextImport.second->getTopLevelModule(/*overlay=*/true);
+
+      // If the rewritten import is now for our own parent module, this was an
+      // import of our own clang submodule in a mixed-language module. We don't
+      // want to process our own cross-imports.
+      if (nextImport.second == SF.getParentModule())
+        continue;
+    }
+
     // Drop this module into the ImportDesc so we treat it as imported with the
     // same options and scope as `I`.
     importDesc.module.second = nextImport.second;
 
-    // If we've already imported it, we've also already imported its
-    // imports.
-    if (!visibleModules.insert(importDesc).second)
+    // Add it to the list of cross-importable modules. If it's already there,
+    // we've already done the rest of the work of this loop iteration and can
+    // skip it.
+    if (!crossImportableModules.insert(importDesc))
       continue;
 
-    // We don't cross-import submodules, so we shouldn't add them to the
-    // visibility set. (However, we do consider their reexports.)
-    if(!isSubmodule(importDesc.module.second))
-      crossImportableModules.insert(importDesc);
+    // We don't consider the re-exports of ObjC modules because ObjC re-exports
+    // everything, so there isn't enough signal there to work from.
+    if (nextImport.second->isNonSwiftModule())
+      continue;
 
     // Add the module's re-exports to worklist.
     nextImport.second->getImportedModules(importsWorklist,

test/CrossImport/Inputs/lib-templates/include/core_mi6.h

@@ -0,0 +1 @@
+void fromCoreMI6();

test/CrossImport/Inputs/lib-templates/include/core_mi6.swiftcrossimport/ThinLibrary.swiftoverlay

@@ -0,0 +1,5 @@
+%YAML 1.2
+---
+version: 1
+modules:
+  - name: _NeverImportedOverlay

test/CrossImport/Inputs/lib-templates/include/module.modulemap

@@ -4,3 +4,11 @@ module ClangLibrary {
     header "clang_library_submodule.h"
   }
 }
+module core_mi6 {
+  header "core_mi6.h"
+  export *
+}
+module UniversalExports {
+  header "universal_exports.h"
+  export *
+}

test/CrossImport/Inputs/lib-templates/include/universal_exports.h

@@ -0,0 +1,2 @@
+#import <core_mi6.h>
+void fromUniversalExportsClang();

test/CrossImport/Inputs/lib-templates/lib/swift/UniversalExports.swiftinterface

@@ -2,6 +2,10 @@
 // swift-module-flags: -swift-version 5 -enable-library-evolution -module-name UniversalExports
 
 import Swift
+
+// This is also a clang overlay:
+@_exported import UniversalExports
+
 @_exported import AlwaysImported
 import NeverImported
 import MI6    // @_exported imports NeverImported

test/CrossImport/transitive.swift

@@ -14,6 +14,12 @@ import UniversalExports
 import ThinLibrary
 #endif
 
+// We should have loaded the underlying clang module.
+fromUniversalExportsClang() // no-error
+
+// We should have loaded core_mi6 too.
+fromCoreMI6() // no-error
+
 // We should have loaded _AlwaysImportedOverlay.
 fromAlwaysImportedOverlay() // no-error