[Dependency Scanning] Pull required dependencies from the adjacent binary module for direct '@testable' interface dependencies

They may be a super-set of the ones that appear in the textual interface - e.g. 'internal' imports will be contained in the adjacent binary module, but not the textual interface

Author: artemcm

lib/Serialization/ModuleDependencyScanner.cpp

@@ -186,25 +186,51 @@ ErrorOr<ModuleDependencyInfo> ModuleDependencyScanner::scanInterfaceFile(
       Result->addModuleImport(import.module.getModulePath(), &alreadyAddedModules);
     }
 
-    // Check the adjacent binary module, if one is found,
-    // for additional "optional" dependencies.
+    // For a `@testable` direct dependency, read in the dependencies
+    // from an adjacent binary module, for completeness.
     if (isTestableImport) {
-      auto adjacentBinaryCandidate = std::find_if(
+      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 (adjacentBinaryCandidate != compiledCandidates.end()) {
-        auto adjacentBinaryCandidateModules = getModuleImportsOfModule(
-            *adjacentBinaryCandidate, ModuleLoadingBehavior::Optional,
-            isFramework, isRequiredOSSAModules(), Ctx.LangOpts.SDKName,
+      if (adjacentBinaryModule != compiledCandidates.end()) {
+        // Required modules.
+        auto adjacentBinaryModuleRequiredImports = getModuleImportsOfModule(
+            *adjacentBinaryModule, ModuleLoadingBehavior::Required, isFramework,
+            isRequiredOSSAModules(), Ctx.LangOpts.SDKName,
             Ctx.LangOpts.PackageName, Ctx.SourceMgr.getFileSystem().get(),
             Ctx.SearchPathOpts.DeserializedPathRecoverer);
-        if (!adjacentBinaryCandidateModules)
-          return adjacentBinaryCandidateModules.getError();
-        for (const auto &t : *adjacentBinaryCandidateModules)
-          Result->addOptionalModuleImport(t.getKey(), &alreadyAddedModules);
+        if (!adjacentBinaryModuleRequiredImports)
+          return adjacentBinaryModuleRequiredImports.getError();
+
+#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(adjacentBinaryModuleRequiredImports->contains(requiredImport) &&
+                 "Expected adjacent binary module's import set to contain all "
+                 "textual interface imports.");
+        }
+#endif
+
+        for (const auto &requiredImport : *adjacentBinaryModuleRequiredImports)
+          Result->addModuleImport(requiredImport.getKey(),
+                                  &alreadyAddedModules);
+
+        // Optional modules. Will be looked-up on a best-effort basis
+        auto adjacentBinaryModuleOptionalImports = getModuleImportsOfModule(
+            *adjacentBinaryModule, ModuleLoadingBehavior::Optional, isFramework,
+            isRequiredOSSAModules(), Ctx.LangOpts.SDKName,
+            Ctx.LangOpts.PackageName, Ctx.SourceMgr.getFileSystem().get(),
+            Ctx.SearchPathOpts.DeserializedPathRecoverer);
+        if (!adjacentBinaryModuleOptionalImports)
+          return adjacentBinaryModuleOptionalImports.getError();
+        for (const auto &optionalImport : *adjacentBinaryModuleOptionalImports)
+          Result->addOptionalModuleImport(optionalImport.getKey(),
+                                          &alreadyAddedModules);
       }
     }
 

lib/Serialization/SerializedModuleLoader.cpp

@@ -420,7 +420,7 @@ SerializedModuleLoaderBase::getModuleImportsOfModule(
             /*debuggerMode*/ false,
             /*isPartialModule*/ false, packageName,
             loadedModuleFile->isTestable());
-    if (dependencyTransitiveBehavior != transitiveBehavior)
+    if (dependencyTransitiveBehavior > transitiveBehavior)
       continue;
 
     // Find the top-level module name.

test/ScanDependencies/required_deps_of_testable_imports.swift

@@ -0,0 +1,20 @@
+// RUN: %empty-directory(%t)
+// RUN: %empty-directory(%t/clang-module-cache)
+// RUN: %empty-directory(%t/Foo.swiftmodule)
+// RUN: echo "internal import A; public func foo() {}" > %t/Foo.swift
+// REQUIRES: executable_test
+// REQUIRES: objc_interop
+
+@testable import Foo
+
+// Step 1: build swift interface and swift module side by side, make them testable
+// RUN: %target-swift-frontend -emit-module %t/Foo.swift -emit-module-path %t/Foo.swiftmodule/%target-swiftmodule-name -module-name Foo -emit-module-interface-path %t/Foo.swiftmodule/%target-swiftinterface-name -I %S/Inputs/CHeaders -I %S/Inputs/Swift -enable-testing -enable-experimental-feature AccessLevelOnImport
+
+// Step 3: scan dependencies
+// RUN: %target-swift-frontend -scan-dependencies %s -o %t/deps.json -I %t -sdk %t -prebuilt-module-cache-path %t/clang-module-cache -I %S/Inputs/CHeaders -I %S/Inputs/Swift
+
+// The dependency of `Foo` on `A` will not be visible if the scanner simply scans the textual interface
+// of `Foo`. So we verify that for a `@testable` import, the scanner also opens up the adjacent binary module and
+// resolves the required dependencies contained within.
+//
+// CHECK: "swift": "A"