Merge pull request #64522 from xymus/access-level-import-testable

xymus · web-flow · commit cc433261ba97 · 2023-03-22T12:54:39.000-07:00
[Serialization] Load non-public dependencies of modules when testing is enabled
diff --git a/include/swift/AST/Module.h b/include/swift/AST/Module.h
@@ -857,16 +857,20 @@ class ModuleDecl
     Exported = 1 << 0,
     /// Include "regular" imports with an access-level of `public`.
     Default = 1 << 1,
-    /// Include imports declared with `@_implementationOnly` or with an
-    /// access-level of `package` or less.
+    /// Include imports declared with `@_implementationOnly`.
     ImplementationOnly = 1 << 2,
     /// Include imports declared with `package import`.
     PackageOnly = 1 << 3,
+    /// Include imports marked `internal` or lower. These differs form
+    /// implementation-only imports by stricter type-checking and loading
+    /// policies. At this moment, we can group them under the same category
+    /// as they have the same loading behavior.
+    InternalOrBelow = 1 << 4,
     /// Include imports declared with `@_spiOnly`.
-    SPIOnly = 1 << 4,
+    SPIOnly = 1 << 5,
     /// Include imports shadowed by a cross-import overlay. Unshadowed imports
     /// are included whether or not this flag is specified.
-    ShadowedByCrossImportOverlay = 1 << 5
+    ShadowedByCrossImportOverlay = 1 << 6
   };
   /// \sa getImportedModules
   using ImportFilter = OptionSet<ImportFilterKind>;
@@ -877,6 +881,7 @@ class ModuleDecl
             ImportFilterKind::Default,
             ImportFilterKind::ImplementationOnly,
             ImportFilterKind::PackageOnly,
+            ImportFilterKind::InternalOrBelow,
             ImportFilterKind::SPIOnly,
             ImportFilterKind::ShadowedByCrossImportOverlay};
   }
@@ -890,6 +895,7 @@ class ModuleDecl
             ImportFilterKind::Default,
             ImportFilterKind::ImplementationOnly,
             ImportFilterKind::PackageOnly,
+            ImportFilterKind::InternalOrBelow,
             ImportFilterKind::SPIOnly};
   }
 
diff --git a/lib/AST/ImportCache.cpp b/lib/AST/ImportCache.cpp
@@ -195,6 +195,7 @@ ImportSet &ImportCache::getImportSet(const DeclContext *dc) {
     file->getImportedModules(imports,
                              {ModuleDecl::ImportFilterKind::Default,
                               ModuleDecl::ImportFilterKind::ImplementationOnly,
+                              ModuleDecl::ImportFilterKind::InternalOrBelow,
                               ModuleDecl::ImportFilterKind::PackageOnly,
                               ModuleDecl::ImportFilterKind::SPIOnly});
   }
@@ -280,6 +281,7 @@ ImportCache::getAllAccessPathsNotShadowedBy(const ModuleDecl *mod,
     file->getImportedModules(stack,
                              {ModuleDecl::ImportFilterKind::Default,
                               ModuleDecl::ImportFilterKind::ImplementationOnly,
+                              ModuleDecl::ImportFilterKind::InternalOrBelow,
                               ModuleDecl::ImportFilterKind::PackageOnly,
                               ModuleDecl::ImportFilterKind::SPIOnly});
   }
diff --git a/lib/AST/Module.cpp b/lib/AST/Module.cpp
@@ -2118,16 +2118,15 @@ SourceFile::getImportedModules(SmallVectorImpl<ImportedModule> &modules,
   if (!Imports)
     return;
 
-  bool moduleIsResilient = getParentModule()->getResilienceStrategy() ==
-                             ResilienceStrategy::Resilient;
   for (auto desc : *Imports) {
     ModuleDecl::ImportFilter requiredFilter;
     if (desc.options.contains(ImportFlags::Exported))
       requiredFilter |= ModuleDecl::ImportFilterKind::Exported;
-    else if (desc.options.contains(ImportFlags::ImplementationOnly) ||
-             (desc.accessLevel <= AccessLevel::Internal && moduleIsResilient))
+    else if (desc.options.contains(ImportFlags::ImplementationOnly))
       requiredFilter |= ModuleDecl::ImportFilterKind::ImplementationOnly;
-    else if (desc.accessLevel <= AccessLevel::Package && moduleIsResilient)
+    else if (desc.accessLevel <= AccessLevel::Internal)
+      requiredFilter |= ModuleDecl::ImportFilterKind::InternalOrBelow;
+    else if (desc.accessLevel <= AccessLevel::Package)
       requiredFilter |= ModuleDecl::ImportFilterKind::PackageOnly;
     else if (desc.options.contains(ImportFlags::SPIOnly))
       requiredFilter |= ModuleDecl::ImportFilterKind::SPIOnly;
@@ -2483,6 +2482,7 @@ SourceFile::collectLinkLibraries(ModuleDecl::LinkLibraryCallback callback) const
 
   ModuleDecl::ImportFilter topLevelFilter = filter;
   topLevelFilter |= ModuleDecl::ImportFilterKind::ImplementationOnly;
+  topLevelFilter |= ModuleDecl::ImportFilterKind::InternalOrBelow;
   topLevelFilter |= ModuleDecl::ImportFilterKind::PackageOnly,
   topLevelFilter |= ModuleDecl::ImportFilterKind::SPIOnly;
   topLevel->getImportedModules(stack, topLevelFilter);
diff --git a/lib/Serialization/ModuleFile.cpp b/lib/Serialization/ModuleFile.cpp
@@ -468,6 +468,10 @@ void ModuleFile::getImportedModules(SmallVectorImpl<ImportedModule> &results,
         continue;
       }
 
+    } else if (dep.isInternalOrBelow()) {
+      if (!filter.contains(ModuleDecl::ImportFilterKind::InternalOrBelow))
+        continue;
+
     } else if (dep.isPackageOnly()) {
       if (!filter.contains(ModuleDecl::ImportFilterKind::PackageOnly))
         continue;
diff --git a/lib/Serialization/ModuleFile.h b/lib/Serialization/ModuleFile.h
@@ -107,6 +107,9 @@ class ModuleFile
     bool isImplementationOnly() const {
       return Core.isImplementationOnly();
     }
+    bool isInternalOrBelow() const {
+      return Core.isInternalOrBelow();
+    }
     bool isPackageOnly() const {
       return Core.isPackageOnly();
     }
diff --git a/lib/Serialization/ModuleFileSharedCore.cpp b/lib/Serialization/ModuleFileSharedCore.cpp
@@ -1021,6 +1021,8 @@ getActualImportControl(unsigned rawValue) {
     return ModuleDecl::ImportFilterKind::Exported;
   case static_cast<unsigned>(serialization::ImportControl::ImplementationOnly):
     return ModuleDecl::ImportFilterKind::ImplementationOnly;
+  case static_cast<unsigned>(serialization::ImportControl::InternalOrBelow):
+    return ModuleDecl::ImportFilterKind::InternalOrBelow;
   case static_cast<unsigned>(serialization::ImportControl::PackageOnly):
     return ModuleDecl::ImportFilterKind::PackageOnly;
   default:
@@ -1694,6 +1696,8 @@ ModuleFileSharedCore::getTransitiveLoadingBehavior(
     return ModuleLoadingBehavior::Required;
   }
 
+  bool moduleIsResilient = getResilienceStrategy() ==
+                             ResilienceStrategy::Resilient;
   if (dependency.isImplementationOnly()) {
     // Implementation-only dependencies are not usually loaded from
     // transitive imports.
@@ -1709,10 +1713,24 @@ ModuleFileSharedCore::getTransitiveLoadingBehavior(
     }
   }
 
+  if (dependency.isInternalOrBelow()) {
+    // Non-public imports are similar to implementation-only, the module
+    // loading behavior differs on loading those dependencies
+    // on testable imports.
+    if (isTestable() || !moduleIsResilient) {
+      return ModuleLoadingBehavior::Required;
+    } else if (debuggerMode) {
+      return ModuleLoadingBehavior::Optional;
+    } else {
+      return ModuleLoadingBehavior::Ignored;
+    }
+  }
+
   if (dependency.isPackageOnly()) {
     // Package dependencies are usually loaded only for import from the same
     // package.
-    if (!packageName.empty() && packageName == getModulePackageName()) {
+    if ((!packageName.empty() && packageName == getModulePackageName()) ||
+        !moduleIsResilient) {
       return ModuleLoadingBehavior::Required;
     } else if (debuggerMode) {
       return ModuleLoadingBehavior::Optional;
diff --git a/lib/Serialization/ModuleFileSharedCore.h b/lib/Serialization/ModuleFileSharedCore.h
@@ -119,7 +119,7 @@ class ModuleFileSharedCore {
 
   private:
     using ImportFilterKind = ModuleDecl::ImportFilterKind;
-    const unsigned RawImportControl : 2;
+    const unsigned RawImportControl : 3;
     const unsigned IsHeader : 1;
     const unsigned IsScoped : 1;
 
@@ -159,6 +159,9 @@ class ModuleFileSharedCore {
     bool isImplementationOnly() const {
       return getImportControl() == ImportFilterKind::ImplementationOnly;
     }
+    bool isInternalOrBelow() const {
+      return getImportControl() == ImportFilterKind::InternalOrBelow;
+    }
     bool isPackageOnly() const {
       return getImportControl() == ImportFilterKind::PackageOnly;
     }
@@ -573,6 +576,16 @@ class ModuleFileSharedCore {
     return ModulePackageName;
   }
 
+  /// Is the module built with testing enabled?
+  bool isTestable() const {
+     return Bits.IsTestable;
+   }
+
+  /// Whether the module is resilient. ('-enable-library-evolution')
+  ResilienceStrategy getResilienceStrategy() const {
+    return ResilienceStrategy(Bits.ResilienceStrategy);
+  }
+
   /// Returns the list of modules this module depends on.
   ArrayRef<Dependency> getDependencies() const {
     return Dependencies;
diff --git a/lib/Serialization/ModuleFormat.h b/lib/Serialization/ModuleFormat.h
@@ -58,7 +58,7 @@ const uint16_t SWIFTMODULE_VERSION_MAJOR = 0;
 /// describe what change you made. The content of this comment isn't important;
 /// it just ensures a conflict if two people change the module format.
 /// Don't worry about adhering to the 80-column limit for this line.
-const uint16_t SWIFTMODULE_VERSION_MINOR = 754; // allocbox move debug info
+const uint16_t SWIFTMODULE_VERSION_MINOR = 755; // InternalOrBelow dependency
 
 /// A standard hash seed used for all string hashes in a serialized module.
 ///
@@ -583,12 +583,14 @@ enum class ImportControl : uint8_t {
   Normal = 0,
   /// `@_exported import FooKit`
   Exported,
-  /// `@_uncheckedImplementationOnly import FooKit`
+  /// `@_implementationOnly import FooKit`
   ImplementationOnly,
+  /// `internal import FooKit` or more restrictive.
+  InternalOrBelow,
   /// `package import FooKit`
-  PackageOnly
+  PackageOnly,
 };
-using ImportControlField = BCFixed<2>;
+using ImportControlField = BCFixed<3>;
 
 // These IDs must \em not be renumbered or reordered without incrementing
 // the module version.
diff --git a/lib/Serialization/Serialization.cpp b/lib/Serialization/Serialization.cpp
@@ -1218,6 +1218,8 @@ void Serializer::writeInputBlock(const SerializationOptions &options) {
       getImportsAsSet(M, ModuleDecl::ImportFilterKind::Default);
   ImportSet packageOnlyImportSet =
       getImportsAsSet(M, ModuleDecl::ImportFilterKind::PackageOnly);
+  ImportSet internalOrBelowImportSet =
+      getImportsAsSet(M, ModuleDecl::ImportFilterKind::InternalOrBelow);
 
   auto clangImporter =
     static_cast<ClangImporter *>(M->getASTContext().getClangModuleLoader());
@@ -1267,6 +1269,8 @@ void Serializer::writeInputBlock(const SerializationOptions &options) {
       stableImportControl = ImportControl::Normal;
     else if (packageOnlyImportSet.count(import))
       stableImportControl = ImportControl::PackageOnly;
+    else if (internalOrBelowImportSet.count(import))
+      stableImportControl = ImportControl::InternalOrBelow;
     else
       stableImportControl = ImportControl::ImplementationOnly;
 
diff --git a/test/Serialization/access-level-import-dependencies.swift b/test/Serialization/access-level-import-dependencies.swift
@@ -42,15 +42,15 @@ private import HiddenDep
 // RUN:   -enable-experimental-feature AccessLevelOnImport
 
 // RUN: %target-swift-frontend -typecheck %t/ClientOfPublic.swift -I %t \
-// RUN:   -enable-library-evolution -package-name MyOtherPackage \
-// RUN:   -Rmodule-loading 2>&1 | %FileCheck -check-prefix=VISIBLE-HIDDEN-DEP %s
-// VISIBLE-HIDDEN-DEP: source: '{{.*}}HiddenDep.swiftmodule'
+// RUN:   -package-name MyOtherPackage \
+// RUN:   -Rmodule-loading 2>&1 | %FileCheck -check-prefix=VISIBLE-DEP %s
+// VISIBLE-DEP: loaded module 'HiddenDep'
 //--- ClientOfPublic.swift
 import PublicDep
 
 // RUN: %target-swift-frontend -typecheck %t/ClientOfNonPublic.swift -I %t \
-// RUN:   -Rmodule-loading 2>&1 | %FileCheck -check-prefix=HIDDEN-HIDDEN-DEP %s
-// HIDDEN-HIDDEN-DEP-NOT: HiddenDep
+// RUN:   -Rmodule-loading 2>&1 | %FileCheck -check-prefix=HIDDEN-DEP %s
+// HIDDEN-DEP-NOT: loaded module 'HiddenDep'
 //--- ClientOfNonPublic.swift
 import PackageDep
 import InternalDep
@@ -70,7 +70,29 @@ import PrivateDep
 // RUN:   -enable-experimental-feature AccessLevelOnImport
 
 // RUN: %target-swift-frontend -typecheck %t/ClientOfPublic.swift -I %t \
-// RUN:   -Rmodule-loading 2>&1 | %FileCheck -check-prefix=VISIBLE-HIDDEN-DEP %s
+// 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=VISIBLE-HIDDEN-DEP %s
+// 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.
+// RUN: %target-swift-frontend -emit-module %t/PublicDep.swift -o %t -I %t \
+// RUN:   -enable-library-evolution -enable-testing \
+// RUN:   -enable-experimental-feature AccessLevelOnImport
+// RUN: %target-swift-frontend -emit-module %t/PackageDep.swift -o %t -I %t \
+// RUN:   -enable-library-evolution -enable-testing -package-name MyPackage \
+// RUN:   -enable-experimental-feature AccessLevelOnImport
+// RUN: %target-swift-frontend -emit-module %t/InternalDep.swift -o %t -I %t \
+// RUN:   -enable-library-evolution -enable-testing \
+// RUN:   -enable-experimental-feature AccessLevelOnImport
+// RUN: %target-swift-frontend -emit-module %t/FileprivateDep.swift -o %t -I %t \
+// RUN:   -enable-library-evolution -enable-testing \
+// RUN:   -enable-experimental-feature AccessLevelOnImport
+// RUN: %target-swift-frontend -emit-module %t/PrivateDep.swift -o %t -I %t \
+// RUN:   -enable-library-evolution -enable-testing \
+// RUN:   -enable-experimental-feature AccessLevelOnImport
+
+// 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=VISIBLE-DEP %s

Original file line number	Diff line number	Diff line change
`@@ -107,6 +107,9 @@ class ModuleFile`
`107`	`107`	`bool isImplementationOnly() const {`
`108`	`108`	`return Core.isImplementationOnly();`
`109`	`109`	`}`
	`110`	`+ bool isInternalOrBelow() const {`
	`111`	`+ return Core.isInternalOrBelow();`
	`112`	`+ }`
`110`	`113`	`bool isPackageOnly() const {`
`111`	`114`	`return Core.isPackageOnly();`
`112`	`115`	`}`