Merge pull request #64487 from xymus/transitive-loading-refactor

[Serialization] Refactor how we decide to load transitive dependencies

Author: xymus

lib/Serialization/ModuleFile.cpp

@@ -194,23 +194,11 @@ Status ModuleFile::associateWithFileContext(FileUnit *file, SourceLoc diagLoc,
       continue;
     }
 
-    // If this module file is being installed into the main module, it's treated
-    // as a partial module.
-    auto isPartialModule = M->isMainModule();
-
-    if (dependency.isImplementationOnly() &&
-        !(isPartialModule || ctx.LangOpts.DebuggerSupport)) {
-      // When building normally (and not merging partial modules), we don't
-      // want to bring in the implementation-only module, because that might
-      // change the set of visible declarations. However, when debugging we
-      // want to allow getting at the internals of this module when possible,
-      // and so we'll try to reference the implementation-only module if it's
-      // available.
-      continue;
-    }
+    ModuleLoadingBehavior transitiveBehavior =
+      getTransitiveLoadingBehavior(dependency);
 
-    if (dependency.isPackageOnly() &&
-        ctx.LangOpts.PackageName != this->getModulePackageName())
+    // Skip this dependency?
+    if (transitiveBehavior == ModuleLoadingBehavior::Ignored)
       continue;
 
     ImportPath::Builder builder(ctx, dependency.Core.RawPath,
@@ -230,11 +218,13 @@ Status ModuleFile::associateWithFileContext(FileUnit *file, SourceLoc diagLoc,
           modulePath.front().Item == file->getParentModule()->getName()) {
         return error(Status::MissingUnderlyingModule);
       }
-
       // Otherwise, continue trying to load dependencies, so that we can list
       // everything that's missing.
-      if (!(dependency.isImplementationOnly() && ctx.LangOpts.DebuggerSupport))
+
+      // Report a missing dependency only when really needed.
+      if (transitiveBehavior == ModuleLoadingBehavior::Required)
         missingDependency = true;
+
       continue;
     }
 
@@ -269,6 +259,21 @@ Status ModuleFile::associateWithFileContext(FileUnit *file, SourceLoc diagLoc,
   return status;
 }
 
+ModuleLoadingBehavior
+ModuleFile::getTransitiveLoadingBehavior(const Dependency &dependency) const {
+  ASTContext &ctx = getContext();
+  ModuleDecl *mod = FileContext->getParentModule();
+
+  // If this module file is being installed into the main module, it's treated
+  // as a partial module.
+  auto isPartialModule = mod->isMainModule();
+
+  return Core->getTransitiveLoadingBehavior(dependency.Core,
+                                            ctx.LangOpts.DebuggerSupport,
+                                            isPartialModule,
+                                            ctx.LangOpts.PackageName);
+}
+
 bool ModuleFile::mayHaveDiagnosticsPointingAtBuffer() const {
   if (!hasError())
     return false;
@@ -463,6 +468,10 @@ void ModuleFile::getImportedModules(SmallVectorImpl<ImportedModule> &results,
         continue;
       }
 
+    } else if (dep.isPackageOnly()) {
+      if (!filter.contains(ModuleDecl::ImportFilterKind::PackageOnly))
+        continue;
+
     } else {
       if (!filter.contains(ModuleDecl::ImportFilterKind::Default))
         continue;

lib/Serialization/ModuleFile.h

@@ -646,6 +646,10 @@ class ModuleFile
   Status associateWithFileContext(FileUnit *file, SourceLoc diagLoc,
                                   bool recoverFromIncompatibility);
 
+  /// How should \p dependency be loaded for a transitive import via \c this?
+  ModuleLoadingBehavior
+  getTransitiveLoadingBehavior(const Dependency &dependency) 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
   /// that the object must be kept alive as long as the diagnostics exist.

lib/Serialization/ModuleFileSharedCore.cpp

@@ -1680,3 +1680,48 @@ ModuleFileSharedCore::ModuleFileSharedCore(
 bool ModuleFileSharedCore::hasSourceInfo() const {
   return !!DeclUSRsTable;
 }
+
+ModuleLoadingBehavior
+ModuleFileSharedCore::getTransitiveLoadingBehavior(
+                                          const Dependency &dependency,
+                                          bool debuggerMode,
+                                          bool isPartialModule,
+                                          StringRef packageName) const {
+  if (isPartialModule) {
+    // Keep the merge-module behavior for legacy support. In that case
+    // we load all transitive dependencies from partial modules and
+    // error if it fails.
+    return ModuleLoadingBehavior::Required;
+  }
+
+  if (dependency.isImplementationOnly()) {
+    // Implementation-only dependencies are not usually loaded from
+    // transitive imports.
+    if (debuggerMode) {
+      // In the debugger, try to load the module if possible.
+      // Same in the case of a testable import, try to load the dependency
+      // but don't fail if it's missing as this could be source breaking.
+      return ModuleLoadingBehavior::Optional;
+    } else {
+      // When building normally, ignore transitive implementation-only
+      // imports.
+      return ModuleLoadingBehavior::Ignored;
+    }
+  }
+
+  if (dependency.isPackageOnly()) {
+    // Package dependencies are usually loaded only for import from the same
+    // package.
+    if (!packageName.empty() && packageName == getModulePackageName()) {
+      return ModuleLoadingBehavior::Required;
+    } else if (debuggerMode) {
+      return ModuleLoadingBehavior::Optional;
+    } else {
+      return ModuleLoadingBehavior::Ignored;
+    }
+  }
+
+  // By default, imports are required dependencies.
+  return ModuleLoadingBehavior::Required;
+}
+

lib/Serialization/ModuleFileSharedCore.h

@@ -25,6 +25,15 @@ namespace llvm {
 
 namespace swift {
 
+/// How a dependency should be loaded.
+///
+/// \sa getTransitiveLoadingBehavior
+enum class ModuleLoadingBehavior {
+  Required,
+  Optional,
+  Ignored
+};
+
 /// Serialized core data of a module. The difference with `ModuleFile` is that
 /// `ModuleFileSharedCore` provides immutable data and is independent of a
 /// particular ASTContext. It is designed to be able to be shared across
@@ -581,6 +590,22 @@ class ModuleFileSharedCore {
   bool hasSourceInfo() const;
 
   bool isConcurrencyChecked() const { return Bits.IsConcurrencyChecked; }
+
+  /// How should \p dependency be loaded for a transitive import via \c this?
+  ///
+  /// If \p debuggerMode, more transitive dependencies should try to be loaded
+  /// as they can be useful in debugging.
+  ///
+  /// If \p isPartialModule, transitive dependencies should be loaded as we're
+  /// in merge-module mode.
+  ///
+  /// If \p packageName is set, transitive package dependencies are loaded if
+  /// loaded from the same package.
+  ModuleLoadingBehavior
+  getTransitiveLoadingBehavior(const Dependency &dependency,
+                               bool debuggerMode,
+                               bool isPartialModule,
+                               StringRef packageName) const;
 };
 
 template <typename T, typename RawData>

lib/Serialization/SerializedModuleLoader.cpp

@@ -420,14 +420,18 @@ llvm::ErrorOr<ModuleDependencyInfo> SerializedModuleLoaderBase::scanModuleFile(
     if (dependency.isHeader())
       continue;
 
-    // Transitive @_implementationOnly dependencies of
-    // binary modules are not required to be imported during normal builds
-    // TODO: This is worth revisiting for debugger purposes
-    if (dependency.isImplementationOnly())
-      continue;
-
-    if (dependency.isPackageOnly() &&
-        Ctx.LangOpts.PackageName != loadedModuleFile->getModulePackageName())
+    // 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 transitiveBehavior =
+      loadedModuleFile->getTransitiveLoadingBehavior(dependency,
+                                         /*debuggerMode*/false,
+                                         /*isPartialModule*/false,
+                                         /*package*/Ctx.LangOpts.PackageName);
+    if (transitiveBehavior != ModuleLoadingBehavior::Required)
       continue;
 
     // Find the top-level module name.
@@ -940,10 +944,11 @@ void swift::serialization::diagnoseSerializedASTLoadFailure(
     std::copy_if(
         loadedModuleFile->getDependencies().begin(),
         loadedModuleFile->getDependencies().end(), std::back_inserter(missing),
-        [&duplicates, &Ctx](const ModuleFile::Dependency &dependency) -> bool {
+        [&duplicates, &loadedModuleFile](
+            const ModuleFile::Dependency &dependency) -> bool {
           if (dependency.isLoaded() || dependency.isHeader() ||
-              (dependency.isImplementationOnly() &&
-               Ctx.LangOpts.DebuggerSupport)) {
+              loadedModuleFile->getTransitiveLoadingBehavior(dependency) !=
+                ModuleLoadingBehavior::Required) {
             return false;
           }
           return duplicates.insert(dependency.Core.RawPath).second;