[ExplicitModule] Change how @testable imports are handled

cachemeifyoucan · cachemeifyoucan · commit 26ac2e0dbd90 · 2024-02-19T16:22:49.000-08:00
Always prefer binary module when using @testable imports because the swiftmodule rebuilt from interface cannot be imported as testable. rdar://123120159
diff --git a/include/swift/Serialization/ScanningLoaders.h b/include/swift/Serialization/ScanningLoaders.h
@@ -34,8 +34,7 @@ class SwiftModuleScanner : public SerializedModuleLoaderBase {
 
   /// Scan the given interface file to determine dependencies.
   llvm::ErrorOr<ModuleDependencyInfo>
-  scanInterfaceFile(Twine moduleInterfacePath, bool isFramework,
-                    bool isTestableImport);
+  scanInterfaceFile(Twine moduleInterfacePath, bool isFramework);
 
   InterfaceSubContextDelegate &astDelegate;
 
diff --git a/lib/Serialization/ScanningLoaders.cpp b/lib/Serialization/ScanningLoaders.cpp
@@ -80,8 +80,7 @@ std::error_code SwiftModuleScanner::findModuleFilesInDirectory(
       InPath = PrivateInPath;
     }
   }
-  auto dependencies =
-      scanInterfaceFile(InPath, IsFramework, isTestableDependencyLookup);
+  auto dependencies = scanInterfaceFile(InPath, IsFramework);
   if (dependencies) {
     this->dependencies = std::move(dependencies.get());
     return std::error_code();
@@ -148,7 +147,7 @@ static std::vector<std::string> getCompiledCandidates(ASTContext &ctx,
 
 llvm::ErrorOr<ModuleDependencyInfo>
 SwiftModuleScanner::scanInterfaceFile(Twine moduleInterfacePath,
-                                      bool isFramework, bool isTestableImport) {
+                                      bool isFramework) {
   // Create a module filename.
   // FIXME: Query the module interface loader to determine an appropriate
   // name for the module, which includes an appropriate hash.
@@ -242,64 +241,6 @@ SwiftModuleScanner::scanInterfaceFile(Twine moduleInterfacePath,
                                   &alreadyAddedModules);
         }
 
-        // For a `@testable` direct dependency, read in the dependencies
-        // from an adjacent binary module, for completeness.
-        if (isTestableImport) {
-          auto adjacentBinaryModule = std::find_if(
-              compiledCandidates.begin(), compiledCandidates.end(),
-              [moduleInterfacePath](const std::string &candidate) {
-                return llvm::sys::path::parent_path(candidate) ==
-                       llvm::sys::path::parent_path(moduleInterfacePath.str());
-              });
-          if (adjacentBinaryModule != compiledCandidates.end()) {
-            // Required modules.
-            auto adjacentBinaryModuleRequiredImports = getImportsOfModule(
-                *adjacentBinaryModule, ModuleLoadingBehavior::Required,
-                isFramework, isRequiredOSSAModules(),
-                isRequiredNoncopyableGenerics(),
-                Ctx.LangOpts.SDKName,
-                Ctx.LangOpts.PackageName, Ctx.SourceMgr.getFileSystem().get(),
-                Ctx.SearchPathOpts.DeserializedPathRecoverer);
-            if (!adjacentBinaryModuleRequiredImports)
-              return adjacentBinaryModuleRequiredImports.getError();
-            auto adjacentBinaryModuleRequiredModuleImports =
-                (*adjacentBinaryModuleRequiredImports).moduleImports;
-#ifndef NDEBUG
-            //  Verify that the set of required modules read out from the binary
-            //  module is a super-set of module imports identified in the
-            //  textual interface.
-            for (const auto &requiredImport : Result->getModuleImports()) {
-              assert(
-                  adjacentBinaryModuleRequiredModuleImports.contains(
-                      requiredImport) &&
-                  "Expected adjacent binary module's import set to contain all "
-                  "textual interface imports.");
-            }
-#endif
-
-            for (const auto &requiredImport :
-                 adjacentBinaryModuleRequiredModuleImports)
-              Result->addModuleImport(requiredImport.getKey(),
-                                      &alreadyAddedModules);
-
-            // Optional modules. Will be looked-up on a best-effort basis
-            auto adjacentBinaryModuleOptionalImports = getImportsOfModule(
-                *adjacentBinaryModule, ModuleLoadingBehavior::Optional,
-                isFramework, isRequiredOSSAModules(),
-                isRequiredNoncopyableGenerics(), Ctx.LangOpts.SDKName,
-                Ctx.LangOpts.PackageName, Ctx.SourceMgr.getFileSystem().get(),
-                Ctx.SearchPathOpts.DeserializedPathRecoverer);
-            if (!adjacentBinaryModuleOptionalImports)
-              return adjacentBinaryModuleOptionalImports.getError();
-            auto adjacentBinaryModuleOptionalModuleImports =
-                (*adjacentBinaryModuleOptionalImports).moduleImports;
-            for (const auto &optionalImport :
-                 adjacentBinaryModuleOptionalModuleImports)
-              Result->addOptionalModuleImport(optionalImport.getKey(),
-                                              &alreadyAddedModules);
-          }
-        }
-
         return std::error_code();
       });
 
@@ -324,6 +265,10 @@ ModuleDependencyVector SerializedModuleLoaderBase::getModuleDependencies(
   if (CacheFS)
     tracker = SwiftDependencyTracker(*CacheFS, mapper);
 
+  // Do not load interface module if it is testable import.
+  ModuleLoadingMode MLM =
+      isTestableDependencyLookup ? ModuleLoadingMode::OnlySerialized : LoadMode;
+
   // Instantiate dependency scanning "loaders".
   SmallVector<std::unique_ptr<SwiftModuleScanner>, 2> scanners;
   // Placeholder dependencies must be resolved first, to prevent the
@@ -332,11 +277,10 @@ ModuleDependencyVector SerializedModuleLoaderBase::getModuleDependencies(
   // dependency graph of the placeholder dependency module itself.
   // FIXME: submodules?
   scanners.push_back(std::make_unique<PlaceholderSwiftModuleScanner>(
-      Ctx, LoadMode, moduleId,
-      Ctx.SearchPathOpts.PlaceholderDependencyModuleMap, delegate,
-      moduleOutputPath, tracker));
+      Ctx, MLM, moduleId, Ctx.SearchPathOpts.PlaceholderDependencyModuleMap,
+      delegate, moduleOutputPath, tracker));
   scanners.push_back(std::make_unique<SwiftModuleScanner>(
-      Ctx, LoadMode, moduleId, delegate, moduleOutputPath,
+      Ctx, MLM, moduleId, delegate, moduleOutputPath,
       SwiftModuleScanner::MDS_plain, tracker));
 
   // Check whether there is a module with this name that we can import.
diff --git a/test/ScanDependencies/testable-import.swift b/test/ScanDependencies/testable-import.swift
@@ -0,0 +1,66 @@
+// RUN: %empty-directory(%t)
+// RUN: split-file %s %t
+
+// RUN: %target-swift-frontend -emit-module -module-name C -o %t/C.swiftmodule -swift-version 5 \
+// RUN:   -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib \
+// RUN:   -emit-module-interface-path %t/C.swiftinterface -enable-library-evolution -I %t -enable-testing \
+// RUN:   %t/C.swift
+
+// RUN: %target-swift-frontend -emit-module -module-name B -o %t/B.swiftmodule -swift-version 5 \
+// RUN:   -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib \
+// RUN:   -emit-module-interface-path %t/B.swiftinterface -enable-library-evolution -I %t -enable-testing \
+// RUN:   %t/B.swift
+
+// RUN: %target-swift-frontend -emit-module -module-name A -o %t/A.swiftmodule -swift-version 5 \
+// RUN:   -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib \
+// RUN:   -emit-module-interface-path %t/A.swiftinterface -enable-library-evolution -I %t -enable-testing \
+// RUN:   %t/A.swift
+
+// RUN: %target-swift-frontend -scan-dependencies -module-name Test %t/test1.swift \
+// RUN:   -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib -enable-testing \
+// RUN:   -o %t/deps1.json -I %t -swift-version 5
+// RUN: %{python} %S/../CAS/Inputs/SwiftDepsExtractor.py %t/deps1.json Test directDependencies | %FileCheck %s --check-prefix TEST1
+// TEST1-DAG: "swiftPrebuiltExternal": "B"
+// TEST1-DAG: "swift": "C"
+
+// RUN: %target-swift-frontend -scan-dependencies -module-name Test %t/test2.swift \
+// RUN:   -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib -enable-testing \
+// RUN:   -o %t/deps2.json -I %t -swift-version 5
+// RUN: %{python} %S/../CAS/Inputs/SwiftDepsExtractor.py %t/deps2.json Test directDependencies | %FileCheck %s --check-prefix TEST2
+// TEST2-DAG: "swift": "A"
+// TEST2-DAG: "swiftPrebuiltExternal": "B"
+
+/// Verify that the interface module inside A is overwritten to be binary module due to the deps in main module.
+// RUN: %{python} %S/../CAS/Inputs/SwiftDepsExtractor.py %t/deps2.json A directDependencies | %FileCheck %s --check-prefix TEST2-A
+// TEST2-A-DAG: "swiftPrebuiltExternal": "B"
+// TEST2-A-DAG: "swift": "C"
+
+/// An indirect @testable import is still interface deps.
+// RUN: %target-swift-frontend -scan-dependencies -module-name Test %t/test3.swift \
+// RUN:   -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib -enable-testing \
+// RUN:   -o %t/deps3.json -I %t -swift-version 5
+// RUN: %{python} %S/../CAS/Inputs/SwiftDepsExtractor.py %t/deps3.json Test directDependencies | %FileCheck %s --check-prefix TEST3
+// TEST3-DAG: "swiftPrebuiltExternal": "A"
+// TEST3-DAG: "swift": "C"
+
+//--- test1.swift
+@testable import B
+import C
+
+//--- test2.swift
+import A
+@testable import B
+
+//--- test3.swift
+@testable import A
+import C
+
+//--- A.swift
+import B
+@testable import C
+
+//--- B.swift
+@_spi(Testing) public func b() {}
+
+//--- C.swift
+@_spi(Testing) public func c() {}
