[DependencyScanning] Handle testable dependencies correctly

Teach scanner to pick and choose binary modules correctly based on if it
is testable import or not. Some situations that scanner need to be
careful when testable is involved:

* When it is a regular import, it should not import binary modules that
  are built with -enable-testing, it should prefer interfaces if that is
  available.
* When testable import, it should only load binary module and it should
  make sure the internal imports from binary modules are actually
  required for testable import to work.

If a testable import only find a regular binary module, dependency
scanner currently will just preceed with such module and leave the
diagnostics to swift-frontend, because the alternative (failed to find
module) can be confusing to users.

rdar://125914165

Author: cachemeifyoucan

include/swift/AST/DiagnosticsSema.def

@@ -1193,6 +1193,9 @@ REMARK(module_api_import_aliases,none,
        "%select{, which reexports definition from %2|}3",
        (const Decl *, ModuleDecl *, ModuleDecl *, bool))
 
+REMARK(skip_module_invalid,none,"skip invalid swiftmodule: %0", (StringRef))
+REMARK(skip_module_testable,none,"skip swiftmodule built with '-enable-testing': %0", (StringRef))
+
 REMARK(macro_loaded,none,
        "loaded macro implementation module %0 from "
        "%select{shared library '%2'|executable '%2'|"

include/swift/Serialization/ScanningLoaders.h

@@ -34,7 +34,8 @@ class SwiftModuleScanner : public SerializedModuleLoaderBase {
 
   /// Scan the given interface file to determine dependencies.
   llvm::ErrorOr<ModuleDependencyInfo>
-  scanInterfaceFile(Twine moduleInterfacePath, bool isFramework);
+  scanInterfaceFile(Twine moduleInterfacePath, bool isFramework,
+                    bool isTestableImport);
 
   InterfaceSubContextDelegate &astDelegate;
 

include/swift/Serialization/SerializedModuleLoader.h

@@ -24,6 +24,7 @@
 namespace swift {
 class ModuleFile;
 class PathObfuscator;
+class ModuleFileSharedCore;
 enum class ModuleLoadingBehavior;
 namespace file_types {
   enum ID : uint8_t;
@@ -162,22 +163,18 @@ class SerializedModuleLoaderBase : public ModuleLoader {
   }
 
   /// Scan the given serialized module file to determine dependencies.
-  llvm::ErrorOr<ModuleDependencyInfo> scanModuleFile(Twine modulePath, bool isFramework);
+  llvm::ErrorOr<ModuleDependencyInfo>
+  scanModuleFile(Twine modulePath, bool isFramework, bool isTestableImport);
 
   struct BinaryModuleImports {
     llvm::StringSet<> moduleImports;
     std::string headerImport;
   };
 
-  static llvm::ErrorOr<BinaryModuleImports>
-  getImportsOfModule(Twine modulePath,
+  static BinaryModuleImports
+  getImportsOfModule(const ModuleFileSharedCore &loadedModule,
                      ModuleLoadingBehavior transitiveBehavior,
-                     bool isFramework,
-                     bool isRequiredOSSAModules,
-                     StringRef SDKName,
-                     StringRef packageName,
-                     llvm::vfs::FileSystem *fileSystem,
-                     PathObfuscator &recoverer);
+                     StringRef packageName);
 
   /// Load the module file into a buffer and also collect its module name.
   static std::unique_ptr<llvm::MemoryBuffer>

lib/Serialization/ScanningLoaders.cpp

@@ -33,6 +33,7 @@
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include <algorithm>
+#include <system_error>
 
 using namespace swift;
 
@@ -55,7 +56,8 @@ std::error_code SwiftModuleScanner::findModuleFilesInDirectory(
   if (LoadMode == ModuleLoadingMode::OnlySerialized || !InPath) {
     if (fs.exists(ModPath)) {
       // The module file will be loaded directly.
-      auto dependencies = scanModuleFile(ModPath, IsFramework);
+      auto dependencies =
+          scanModuleFile(ModPath, IsFramework, isTestableDependencyLookup);
       if (dependencies) {
         this->dependencies = std::move(dependencies.get());
         return std::error_code();
@@ -66,7 +68,8 @@ std::error_code SwiftModuleScanner::findModuleFilesInDirectory(
   }
   assert(InPath);
 
-  auto dependencies = scanInterfaceFile(*InPath, IsFramework);
+  auto dependencies =
+      scanInterfaceFile(*InPath, IsFramework, isTestableDependencyLookup);
   if (dependencies) {
     this->dependencies = std::move(dependencies.get());
     return std::error_code();
@@ -133,7 +136,7 @@ static std::vector<std::string> getCompiledCandidates(ASTContext &ctx,
 
 llvm::ErrorOr<ModuleDependencyInfo>
 SwiftModuleScanner::scanInterfaceFile(Twine moduleInterfacePath,
-                                      bool isFramework) {
+                                      bool isFramework, bool isTestableImport) {
   // Create a module filename.
   // FIXME: Query the module interface loader to determine an appropriate
   // name for the module, which includes an appropriate hash.
@@ -156,12 +159,16 @@ SwiftModuleScanner::scanInterfaceFile(Twine moduleInterfacePath,
             !Ctx.SearchPathOpts.NoScannerModuleValidation) {
           assert(compiledCandidates.size() == 1 &&
                  "Should only have 1 candidate module");
-          auto BinaryDep = scanModuleFile(compiledCandidates[0], isFramework);
-          if (!BinaryDep)
-            return BinaryDep.getError();
+          auto BinaryDep = scanModuleFile(compiledCandidates[0], isFramework,
+                                          isTestableImport);
+          if (BinaryDep) {
+            Result = *BinaryDep;
+            return std::error_code();
+          }
 
-          Result = *BinaryDep;
-          return std::error_code();
+          // If return no such file, just fallback to use interface.
+          if (BinaryDep.getError() != std::errc::no_such_file_or_directory)
+            return BinaryDep.getError();
         }
 
         std::vector<std::string> Args(BaseArgs.begin(), BaseArgs.end());

lib/Serialization/SerializedModuleLoader.cpp

@@ -392,29 +392,13 @@ std::error_code SerializedModuleLoaderBase::openModuleFile(
   return std::error_code();
 }
 
-llvm::ErrorOr<SerializedModuleLoaderBase::BinaryModuleImports>
+SerializedModuleLoaderBase::BinaryModuleImports
 SerializedModuleLoaderBase::getImportsOfModule(
-    Twine modulePath, ModuleLoadingBehavior transitiveBehavior,
-    bool isFramework, bool isRequiredOSSAModules, StringRef SDKName,
-    StringRef packageName, llvm::vfs::FileSystem *fileSystem,
-    PathObfuscator &recoverer) {
-  auto moduleBuf = fileSystem->getBufferForFile(modulePath);
-  if (!moduleBuf)
-    return moduleBuf.getError();
-
+    const ModuleFileSharedCore &loadedModuleFile,
+    ModuleLoadingBehavior transitiveBehavior, StringRef packageName) {
   llvm::StringSet<> importedModuleNames;
   std::string importedHeader = "";
-  std::shared_ptr<const ModuleFileSharedCore> loadedModuleFile;
-  serialization::ValidationInfo loadInfo = ModuleFileSharedCore::load(
-      "", "", std::move(moduleBuf.get()), nullptr, nullptr, isFramework,
-      isRequiredOSSAModules,
-      SDKName, recoverer, loadedModuleFile);
-
-  // If failed to load, just ignore and return do not found.
-  if (loadInfo.status != serialization::Status::Valid)
-    return std::make_error_code(std::errc::no_such_file_or_directory);
-
-  for (const auto &dependency : loadedModuleFile->getDependencies()) {
+  for (const auto &dependency : loadedModuleFile.getDependencies()) {
     if (dependency.isHeader()) {
       assert(importedHeader.empty() &&
              "Unexpected more than one header dependency");
@@ -423,11 +407,11 @@ SerializedModuleLoaderBase::getImportsOfModule(
     }
 
     ModuleLoadingBehavior dependencyTransitiveBehavior =
-        loadedModuleFile->getTransitiveLoadingBehavior(
+        loadedModuleFile.getTransitiveLoadingBehavior(
             dependency,
             /*debuggerMode*/ false,
             /*isPartialModule*/ false, packageName,
-            loadedModuleFile->isTestable());
+            loadedModuleFile.isTestable());
     if (dependencyTransitiveBehavior > transitiveBehavior)
       continue;
 
@@ -441,41 +425,60 @@ SerializedModuleLoaderBase::getImportsOfModule(
     importedModuleNames.insert(moduleName);
   }
 
-  return SerializedModuleLoaderBase::BinaryModuleImports{importedModuleNames, importedHeader};
+  return SerializedModuleLoaderBase::BinaryModuleImports{importedModuleNames,
+                                                         importedHeader};
 }
 
 llvm::ErrorOr<ModuleDependencyInfo>
-SerializedModuleLoaderBase::scanModuleFile(Twine modulePath, bool isFramework) {
+SerializedModuleLoaderBase::scanModuleFile(Twine modulePath, bool isFramework,
+                                           bool isTestableImport) {
   const std::string moduleDocPath;
   const std::string sourceInfoPath;
+
+  // Read and valid module.
+  auto moduleBuf = Ctx.SourceMgr.getFileSystem()->getBufferForFile(modulePath);
+  if (!moduleBuf)
+    return moduleBuf.getError();
+
+  std::shared_ptr<const ModuleFileSharedCore> loadedModuleFile;
+  serialization::ValidationInfo loadInfo = ModuleFileSharedCore::load(
+      "", "", std::move(moduleBuf.get()), nullptr, nullptr, isFramework,
+      isRequiredOSSAModules(), Ctx.LangOpts.SDKName,
+      Ctx.SearchPathOpts.DeserializedPathRecoverer, loadedModuleFile);
+
+  if (!Ctx.SearchPathOpts.NoScannerModuleValidation) {
+    // If failed to load, just ignore and return do not found.
+    if (loadInfo.status != serialization::Status::Valid) {
+      if (Ctx.LangOpts.EnableModuleLoadingRemarks)
+        Ctx.Diags.diagnose(SourceLoc(), diag::skip_module_invalid,
+                           modulePath.str());
+      return std::make_error_code(std::errc::no_such_file_or_directory);
+    }
+
+    if (loadedModuleFile->isTestable() && !isTestableImport) {
+      if (Ctx.LangOpts.EnableModuleLoadingRemarks)
+        Ctx.Diags.diagnose(SourceLoc(), diag::skip_module_testable,
+                           modulePath.str());
+      return std::make_error_code(std::errc::no_such_file_or_directory);
+    }
+  }
+
   // Some transitive dependencies of binary modules are not required to be
   // imported during normal builds.
   // TODO: This is worth revisiting for debugger purposes where
   //       loading the module is optional, and implementation-only imports
   //       from modules with testing enabled where the dependency is
   //       optional.
-  ModuleLoadingBehavior transitiveLoadingBehavior =
-      ModuleLoadingBehavior::Required;
-  auto binaryModuleImports = getImportsOfModule(
-      modulePath, transitiveLoadingBehavior, isFramework,
-      isRequiredOSSAModules(),
-      Ctx.LangOpts.SDKName, Ctx.LangOpts.PackageName,
-      Ctx.SourceMgr.getFileSystem().get(),
-      Ctx.SearchPathOpts.DeserializedPathRecoverer);
-  if (!binaryModuleImports)
-    return binaryModuleImports.getError();
+  auto binaryModuleImports =
+      getImportsOfModule(*loadedModuleFile, ModuleLoadingBehavior::Required,
+                         Ctx.LangOpts.PackageName);
 
   // Lookup optional imports of this module also
-  auto binaryModuleOptionalImports = getImportsOfModule(
-      modulePath, ModuleLoadingBehavior::Optional, isFramework,
-      isRequiredOSSAModules(),
-      Ctx.LangOpts.SDKName, Ctx.LangOpts.PackageName,
-      Ctx.SourceMgr.getFileSystem().get(),
-      Ctx.SearchPathOpts.DeserializedPathRecoverer);
-  if (!binaryModuleOptionalImports)
-    return binaryModuleImports.getError();
+  auto binaryModuleOptionalImports =
+      getImportsOfModule(*loadedModuleFile, ModuleLoadingBehavior::Optional,
+                         Ctx.LangOpts.PackageName);
 
-  auto importedModuleSet = binaryModuleImports.get().moduleImports;
+  auto importedModuleSet = binaryModuleImports.moduleImports;
   std::vector<std::string> importedModuleNames;
   importedModuleNames.reserve(importedModuleSet.size());
   llvm::transform(importedModuleSet.keys(),
@@ -484,8 +487,8 @@ SerializedModuleLoaderBase::scanModuleFile(Twine modulePath, bool isFramework) {
                      return N.str();
                   });
 
-  auto importedHeader = binaryModuleImports.get().headerImport;
-  auto &importedOptionalModuleSet = binaryModuleOptionalImports.get().moduleImports;
+  auto importedHeader = binaryModuleImports.headerImport;
+  auto &importedOptionalModuleSet = binaryModuleOptionalImports.moduleImports;
   std::vector<std::string> importedOptionalModuleNames;
   for (const auto optionalImportedModule : importedOptionalModuleSet.keys())
     if (!importedModuleSet.contains(optionalImportedModule))
@@ -794,7 +797,8 @@ bool SerializedModuleLoaderBase::findModule(
       auto result = findModuleFilesInDirectory(
           moduleID, absoluteBaseName, moduleInterfacePath,
           moduleInterfaceSourcePath, moduleBuffer, moduleDocBuffer,
-          moduleSourceInfoBuffer, skipBuildingInterface, isFramework);
+          moduleSourceInfoBuffer, skipBuildingInterface, isFramework,
+          isTestableDependencyLookup);
       if (!result)
         return true;
       if (result == std::errc::not_supported)

test/ScanDependencies/testable-dependencies.swift

@@ -0,0 +1,78 @@
+// RUN: %empty-directory(%t)
+// RUN: split-file %s %t
+
+// RUN: %target-swift-frontend -emit-module -module-name B -o %t/internal/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/internal/B.swiftinterface -enable-library-evolution -I %t \
+// RUN:   %t/B.swift
+
+// RUN: %target-swift-frontend -emit-module -module-name A -o %t/testable/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/testable/A.swiftinterface -enable-library-evolution -I %t/internal -enable-testing \
+// RUN:   %t/A.swift
+
+// RUN: %target-swift-frontend -emit-module -module-name A -o %t/regular/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/regular/A.swiftinterface -enable-library-evolution -I %t/internal \
+// RUN:   %t/A.swift
+
+/// Import testable build, should use interface.
+// RUN: %target-swift-frontend -scan-dependencies -module-load-mode prefer-serialized -module-name Test %t/main.swift \
+// RUN:   -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib \
+// RUN:   -o %t/deps1.json -I %t/testable -swift-version 5 -Rmodule-loading 2>&1 | %FileCheck %s --check-prefix DIAG
+// DIAG: remark: skip swiftmodule built with '-enable-testing'
+// RUN: %{python} %S/../CAS/Inputs/SwiftDepsExtractor.py %t/deps1.json Test directDependencies | %FileCheck %s --check-prefix TEST1
+// TEST1: "swift": "A"
+// RUN: %{python} %S/../CAS/Inputs/SwiftDepsExtractor.py %t/deps1.json A directDependencies | %FileCheck %s --check-prefix EMPTY --allow-empty
+// EMPTY-NOT: B
+
+/// Import regular build, should use binary.
+// RUN: %target-swift-frontend -scan-dependencies -module-load-mode prefer-serialized -module-name Test %t/main.swift \
+// RUN:   -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib \
+// RUN:   -o %t/deps2.json -I %t/regular -swift-version 5 -Rmodule-loading
+// RUN: %{python} %S/../CAS/Inputs/SwiftDepsExtractor.py %t/deps2.json Test directDependencies | %FileCheck %s --check-prefix TEST2
+// TEST2:  "swiftPrebuiltExternal": "A"
+// RUN: %{python} %S/../CAS/Inputs/SwiftDepsExtractor.py %t/deps2.json swiftPrebuiltExternal:A directDependencies | %FileCheck %s --check-prefix EMPTY --allow-empty
+
+/// Testable import testable build, should use binary, even interface is preferred.
+// RUN: %target-swift-frontend -scan-dependencies -module-load-mode prefer-interface -module-name Test %t/testable.swift \
+// RUN:   -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib -enable-testing \
+// RUN:   -o %t/deps3.json -I %t/testable -I %t/internal -swift-version 5 -Rmodule-loading
+// RUN: %{python} %S/../CAS/Inputs/SwiftDepsExtractor.py %t/deps3.json Test directDependencies | %FileCheck %s --check-prefix TEST3
+// TEST3:  "swiftPrebuiltExternal": "A"
+// RUN: %{python} %S/../CAS/Inputs/SwiftDepsExtractor.py %t/deps3.json swiftPrebuiltExternal:A directDependencies | %FileCheck %s --check-prefix TEST3-A
+// TEST3-A:  "swift": "B"
+
+/// Testable import non-testable build without enable testing.
+// RUN: %target-swift-frontend -scan-dependencies -module-load-mode prefer-interface -module-name Test %t/testable.swift \
+// RUN:   -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib \
+// RUN:   -o %t/deps4.json -I %t/regular -swift-version 5
+// RUN: %{python} %S/../CAS/Inputs/SwiftDepsExtractor.py %t/deps4.json Test directDependencies | %FileCheck %s --check-prefix TEST4
+// TEST4:  "swiftPrebuiltExternal": "A"
+// RUN: %{python} %S/../CAS/Inputs/SwiftDepsExtractor.py %t/deps4.json swiftPrebuiltExternal:A directDependencies | %FileCheck %s --check-prefix EMPTY --allow-empty
+
+/// Testable import non-testable build enable testing, still succeed since swift-frontend can provide a better diagnostics when the module is actually imported.
+// RUN: %target-swift-frontend -scan-dependencies -module-load-mode prefer-interface -module-name Test %t/testable.swift \
+// RUN:   -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib -enable-testing \
+// RUN:   -o %t/deps5.json -I %t/regular -swift-version 5 -Rmodule-loading
+
+/// Regular import a testable module with no interface, this is a dependency scanning error.
+// RUN: rm %t/testable/A.swiftinterface
+// RUN: %target-swift-frontend -scan-dependencies -module-load-mode prefer-interface -module-name Test %t/main.swift \
+// RUN:   -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib -enable-testing \
+// RUN:   -o %t/deps6.json -I %t/testable -swift-version 5 -Rmodule-loading 2>&1 | %FileCheck %s --check-prefix ERROR
+// ERROR: error: Unable to find module dependency: 'A'
+
+//--- main.swift
+import A
+
+//--- testable.swift
+@testable import A
+
+//--- A.swift
+internal import B
+@_spi(Testing) public func a() {}
+
+//--- B.swift
+public func b() {}
+