[Serialization] Load non-public transitive dependencies on @testable imports

xymus · xymus · commit f7f69c6ae12b · 2023-03-29T13:59:28.000-07:00
A @testable import allows a client to call internal decls which may refer to non-public dependencies. To support such a use case, load non-public transitive dependencies of a module when it's imported @testable from the main module. This replaces the previous behavior where we loaded those dependencies for any modules built for testing. This was risky as we would load more module for any debug build, opening the door to a different behavior between debug and release builds. In contrast, applying this logic to @testable clients will only change the behavior of test targets. rdar://107329303
diff --git a/include/swift/AST/FileUnit.h b/include/swift/AST/FileUnit.h
@@ -294,6 +294,9 @@ class FileUnit : public DeclContext, public ASTAllocated<FileUnit> {
   virtual void
   collectLinkLibraries(ModuleDecl::LinkLibraryCallback callback) const {}
 
+  /// Load extra dependencies of this module to satisfy a testable import.
+  virtual void loadDependenciesForTestable(SourceLoc diagLoc) const {}
+
   /// Returns the path of the file or directory that defines the module
   /// represented by this \c FileUnit, or empty string if there is none.
   /// Cross-import overlay specifiers are found relative to this path.
diff --git a/include/swift/Serialization/SerializedModuleLoader.h b/include/swift/Serialization/SerializedModuleLoader.h
@@ -454,6 +454,8 @@ class SerializedASTFile final : public LoadedFile {
   virtual void
   collectLinkLibraries(ModuleDecl::LinkLibraryCallback callback) const override;
 
+  virtual void loadDependenciesForTestable(SourceLoc diagLoc) const override;
+
   Identifier getDiscriminatorForPrivateValue(const ValueDecl *D) const override;
 
   virtual StringRef getFilename() const override;
diff --git a/lib/Sema/ImportResolution.cpp b/lib/Sema/ImportResolution.cpp
@@ -324,6 +324,12 @@ void ImportResolver::bindImport(UnboundImport &&I) {
     return;
   }
 
+  // Load more dependencies for testable imports.
+  if (I.import.options.contains(ImportFlags::Testable)) {
+    for (auto file: M->getFiles())
+      file->loadDependenciesForTestable(ID.get()->getStartLoc());
+  }
+
   auto topLevelModule = I.getTopLevelModule(M, SF);
 
   I.validateOptions(topLevelModule, SF);
diff --git a/lib/Serialization/ModuleFile.cpp b/lib/Serialization/ModuleFile.cpp
@@ -133,6 +133,9 @@ ModuleFile::loadDependenciesForFileContext(const FileUnit *file,
 
   bool missingDependency = false;
   for (auto &dependency : Dependencies) {
+    if (forTestable && dependency.isLoaded())
+      continue;
+
     assert(!dependency.isLoaded() && "already loaded?");
 
     if (dependency.isHeader()) {
@@ -156,7 +159,7 @@ ModuleFile::loadDependenciesForFileContext(const FileUnit *file,
     }
 
     ModuleLoadingBehavior transitiveBehavior =
-      getTransitiveLoadingBehavior(dependency);
+      getTransitiveLoadingBehavior(dependency, forTestable);
 
     if (ctx.LangOpts.EnableModuleLoadingRemarks) {
       ctx.Diags.diagnose(diagLoc,
@@ -282,7 +285,8 @@ Status ModuleFile::associateWithFileContext(FileUnit *file, SourceLoc diagLoc,
 }
 
 ModuleLoadingBehavior
-ModuleFile::getTransitiveLoadingBehavior(const Dependency &dependency) const {
+ModuleFile::getTransitiveLoadingBehavior(const Dependency &dependency,
+    bool forTestable) const {
   ASTContext &ctx = getContext();
   ModuleDecl *mod = FileContext->getParentModule();
 
@@ -293,7 +297,8 @@ ModuleFile::getTransitiveLoadingBehavior(const Dependency &dependency) const {
   return Core->getTransitiveLoadingBehavior(dependency.Core,
                                             ctx.LangOpts.DebuggerSupport,
                                             isPartialModule,
-                                            ctx.LangOpts.PackageName);
+                                            ctx.LangOpts.PackageName,
+                                            forTestable);
 }
 
 bool ModuleFile::mayHaveDiagnosticsPointingAtBuffer() const {
diff --git a/lib/Serialization/ModuleFile.h b/lib/Serialization/ModuleFile.h
@@ -664,7 +664,8 @@ class ModuleFile
 
   /// How should \p dependency be loaded for a transitive import via \c this?
   ModuleLoadingBehavior
-  getTransitiveLoadingBehavior(const Dependency &dependency) const;
+  getTransitiveLoadingBehavior(const Dependency &dependency,
+                               bool forTestable) const;
 
   /// Returns `true` if there is a buffer that might contain source code where
   /// other parts of the compiler could have emitted diagnostics, to indicate
diff --git a/lib/Serialization/ModuleFileSharedCore.cpp b/lib/Serialization/ModuleFileSharedCore.cpp
@@ -1688,7 +1688,8 @@ ModuleFileSharedCore::getTransitiveLoadingBehavior(
                                           const Dependency &dependency,
                                           bool debuggerMode,
                                           bool isPartialModule,
-                                          StringRef packageName) const {
+                                          StringRef packageName,
+                                          bool forTestable) const {
   if (isPartialModule) {
     // Keep the merge-module behavior for legacy support. In that case
     // we load all transitive dependencies from partial modules and
@@ -1717,7 +1718,7 @@ ModuleFileSharedCore::getTransitiveLoadingBehavior(
     // Non-public imports are similar to implementation-only, the module
     // loading behavior differs on loading those dependencies
     // on testable imports.
-    if (isTestable() || !moduleIsResilient) {
+    if (forTestable || !moduleIsResilient) {
       return ModuleLoadingBehavior::Required;
     } else if (debuggerMode) {
       return ModuleLoadingBehavior::Optional;
@@ -1730,6 +1731,7 @@ ModuleFileSharedCore::getTransitiveLoadingBehavior(
     // Package dependencies are usually loaded only for import from the same
     // package.
     if ((!packageName.empty() && packageName == getModulePackageName()) ||
+        forTestable ||
         !moduleIsResilient) {
       return ModuleLoadingBehavior::Required;
     } else if (debuggerMode) {
diff --git a/lib/Serialization/ModuleFileSharedCore.h b/lib/Serialization/ModuleFileSharedCore.h
@@ -614,11 +614,17 @@ class ModuleFileSharedCore {
   ///
   /// If \p packageName is set, transitive package dependencies are loaded if
   /// loaded from the same package.
+  ///
+  /// If \p forTestable, get the desired loading behavior for a @testable
+  /// import. Reports non-public dependencies as required for a testable
+  /// client so it can access internal details, which in turn can reference
+  /// those non-public dependencies.
   ModuleLoadingBehavior
   getTransitiveLoadingBehavior(const Dependency &dependency,
                                bool debuggerMode,
                                bool isPartialModule,
-                               StringRef packageName) const;
+                               StringRef packageName,
+                               bool forTestable) const;
 };
 
 template <typename T, typename RawData>
diff --git a/lib/Serialization/SerializedModuleLoader.cpp b/lib/Serialization/SerializedModuleLoader.cpp
@@ -430,7 +430,8 @@ llvm::ErrorOr<ModuleDependencyInfo> SerializedModuleLoaderBase::scanModuleFile(
       loadedModuleFile->getTransitiveLoadingBehavior(dependency,
                                          /*debuggerMode*/false,
                                          /*isPartialModule*/false,
-                                         /*package*/Ctx.LangOpts.PackageName);
+                                         /*package*/Ctx.LangOpts.PackageName,
+                                         loadedModuleFile->isTestable());
     if (transitiveBehavior != ModuleLoadingBehavior::Required)
       continue;
 
@@ -1039,11 +1040,12 @@ void swift::serialization::diagnoseSerializedASTLoadFailureTransitive(
     std::copy_if(
         loadedModuleFile->getDependencies().begin(),
         loadedModuleFile->getDependencies().end(), std::back_inserter(missing),
-        [&duplicates, &loadedModuleFile](
+        [&duplicates, &loadedModuleFile, forTestable](
             const ModuleFile::Dependency &dependency) -> bool {
           if (dependency.isLoaded() || dependency.isHeader() ||
-              loadedModuleFile->getTransitiveLoadingBehavior(dependency) !=
-                ModuleLoadingBehavior::Required) {
+              loadedModuleFile->getTransitiveLoadingBehavior(dependency,
+                                                             forTestable)
+                != ModuleLoadingBehavior::Required) {
             return false;
           }
           return duplicates.insert(dependency.Core.RawPath).second;
@@ -1472,6 +1474,18 @@ void SerializedASTFile::collectLinkLibraries(
   }
 }
 
+void SerializedASTFile::loadDependenciesForTestable(SourceLoc diagLoc) const {
+  serialization::Status status =
+    File.loadDependenciesForFileContext(this, diagLoc, /*forTestable=*/true);
+
+  if (status != serialization::Status::Valid) {
+    if (diagLoc)
+      serialization::diagnoseSerializedASTLoadFailureTransitive(
+          getASTContext(), diagLoc, status, &File,
+          getParentModule()->getName(), /*forTestable*/true);
+  }
+}
+
 bool SerializedASTFile::isSIB() const {
   return File.isSIB();
 }
diff --git a/test/Serialization/access-level-import-dependencies.swift b/test/Serialization/access-level-import-dependencies.swift
@@ -74,8 +74,8 @@ import PrivateDep
 // RUN: %target-swift-frontend -typecheck %t/ClientOfNonPublic.swift -I %t \
 // RUN:   -Rmodule-loading 2>&1 | %FileCheck -check-prefix=VISIBLE-DEP %s
 
-/// Even with resilience, all access-level dependencies are visible to clients
-/// for modules with testing enabled.
+/// Even with resilience and testing enabled, all non-public dependencies are
+/// hidden if there are no testable imports.
 // RUN: %target-swift-frontend -emit-module %t/PublicDep.swift -o %t -I %t \
 // RUN:   -enable-library-evolution -enable-testing \
 // RUN:   -enable-experimental-feature AccessLevelOnImport
@@ -95,4 +95,24 @@ import PrivateDep
 // RUN: %target-swift-frontend -typecheck %t/ClientOfPublic.swift -I %t \
 // RUN:   -Rmodule-loading 2>&1 | %FileCheck -check-prefix=VISIBLE-DEP %s
 // RUN: %target-swift-frontend -typecheck %t/ClientOfNonPublic.swift -I %t \
+// RUN:   -Rmodule-loading 2>&1 | %FileCheck -check-prefix=HIDDEN-DEP %s
+
+/// With testable imports, transitive dependencies are required.
+//--- TestableClientOfPublic.swift
+@testable import PublicDep
+
+//--- TestableClientOfNonPublic.swift
+@testable import PackageDep // expected-error {{missing required module 'HiddenDep'}}
+@testable import InternalDep // expected-error {{missing required module 'HiddenDep'}}
+@testable import FileprivateDep // expected-error {{missing required module 'HiddenDep'}}
+@testable import PrivateDep // expected-error {{missing required module 'HiddenDep'}}
+
+// RUN: %target-swift-frontend -typecheck %t/TestableClientOfPublic.swift -I %t \
+// RUN:   -Rmodule-loading 2>&1 | %FileCheck -check-prefix=VISIBLE-DEP %s
+// RUN: %target-swift-frontend -typecheck %t/TestableClientOfNonPublic.swift -I %t \
 // RUN:   -Rmodule-loading 2>&1 | %FileCheck -check-prefix=VISIBLE-DEP %s
+
+/// Fail if the transitive dependency is missing.
+// RUN: rm %t/HiddenDep.swiftmodule
+// RUN: %target-swift-frontend -typecheck %t/TestableClientOfNonPublic.swift -I %t \
+// RUN:   -verify -show-diagnostics-after-fatal
