Package CMO: Prevent serializing types from SDK/system modules imported as @_implementationOnly.

Currently, types from @_implementationOnly modules can be serialized into client modules if the
modules are SDK or system modules.

However, @_implementationOnly is intended to hide types from external clients, and when combined
with serialization, it may cause the type’s metadata (e.g., field offsets) to be stripped. This
can lead to linker errors, especially in dynamic contexts like Codable, where runtime metadata
is required for type resolution.

This PR prevents all types imported with @_implementationOnly from being serialized.

Resolves rdar://144181455

Author: elsh

lib/SILOptimizer/IPO/CrossModuleOptimization.cpp

@@ -793,11 +793,29 @@ bool CrossModuleOptimization::checkImports(DeclContext *ctxt) const {
   ModuleDecl::ImportFilter filter;
 
   if (isPackageCMOEnabled(M.getSwiftModule())) {
-    // If Package CMO is enabled, decls imported with `package import`
-    // or `@_spiOnly import` into this module should be allowed to be
-    // serialized. They are used in decls with `package` or higher
-    // access level, with or without @_spi; a client of this module
-    // should be able to access them directly if in the same package.
+    // When Package CMO is enabled, decls imported with `package import` or
+    // `@_spiOnly import` into this module should be allowed to be serialized.
+    // They are used in decls with `package` or higher access level, with or
+    // without `@_spi`; a client of this module should be able to access them
+    // directly if it is in the same package.
+    //
+    // However, decls that contain types from modules imported as
+    // `@_implementationOnly` should not be serialized. If such a type is
+    // serialized, its metadata including field offsets may be stripped, as the
+    // type is meant to remain hidden from external clients. In such cases, if
+    // the client attempts to use the serialized accessor in a dynamic context
+    // (e.g. Codable), it may require the missing metadata to resolve field
+    // offsets, type info, etc. Since the metadata has been stripped, this can
+    // lead to a linker error.
+    //
+    // Because modules imported with `@_implementationOnly` affect their
+    // dependencies transitively, the same rule that strips metadata applies to
+    // types from those transitively imported modules as well. For example,
+    // `@_implementationOnly import Foundation` indirectly imports `ObjectiveC`,
+    // meaning that types from `ObjectiveC` such as `NSObject` can also have
+    // their metadata stripped. To avoid such issues, decls that reference types
+    // from `@_implementationOnly` modules should not be serialized into client
+    // modules.
     filter = { ModuleDecl::ImportFilterKind::ImplementationOnly };
   } else {
     // See if context is imported in a "regular" way, i.e. not with
@@ -813,17 +831,8 @@ bool CrossModuleOptimization::checkImports(DeclContext *ctxt) const {
 
   auto &imports = M.getSwiftModule()->getASTContext().getImportCache();
   for (auto &desc : results) {
-    if (imports.isImportedBy(moduleOfCtxt, desc.importedModule)) {
-      // E.g. `@_implementationOnly import QuartzCore_Private.CALayerPrivate`
-      // imports `Foundation` as its transitive dependency module; use of a
-      // a `public` decl in `Foundation` such as `IndexSet` in a function
-      // signature should not block serialization in Package CMO given the
-      // function has `package` or higher access level.
-      if (isPackageCMOEnabled(M.getSwiftModule()) &&
-          moduleOfCtxt->isNonUserModule())
-          continue;
+    if (imports.isImportedBy(moduleOfCtxt, desc.importedModule))
       return false;
-    }
   }
   return true;
 }

test/SILOptimizer/package-cmo-import-filter.swift

@@ -1,6 +1,30 @@
 // RUN: %empty-directory(%t)
 // RUN: split-file %s %t
 
+/// 1. Test `@_implementationOnly import`.
+// RUN: %target-build-swift-dylib(%t/%target-library-name(Utils)) \
+// RUN: %t/UtilsA.swift %t/UtilsB.swift \
+// RUN: -module-name Utils -emit-module -package-name Pkg \
+// RUN: -Xfrontend -experimental-package-cmo -Xfrontend -experimental-allow-non-resilient-access \
+// RUN: -enable-library-evolution -O -wmo
+// RUN: %target-sil-opt %t/Utils.swiftmodule -I %t -sil-verify-all -o %t/Utils.sil
+// RUN: %FileCheck %s --check-prefix=CHECK-UTILS < %t/Utils.sil
+
+/// Ensure PkgKlass.second accessor symbols are present in the final binary.
+// RUN: nm -gU %t/%target-library-name(Utils) | grep "second" | %FileCheck %s --check-prefix=CHECK-NM
+// CHECK-NM: s5Utils8PkgKlassC6secondSo8NSObjectCvM
+// CHECK-NM: s5Utils8PkgKlassC6secondSo8NSObjectCvMTj
+// CHECK-NM: s5Utils8PkgKlassC6secondSo8NSObjectCvMTq
+// CHECK-NM: s5Utils8PkgKlassC6secondSo8NSObjectCvg
+// CHECK-NM: s5Utils8PkgKlassC6secondSo8NSObjectCvgTj
+// CHECK-NM: s5Utils8PkgKlassC6secondSo8NSObjectCvgTq
+
+/// Ensure accessing PkgKlass.second from a client does not cause a linker error.
+// RUN: %target-build-swift -I %t -L %t %t/Client.swift -package-name Pkg \
+// RUN: %target-rpath(%t) -lUtils -o %t/a.out
+
+
+/// 2. Test `package import` and `@_spiOnly import`.
 // RUN: %target-swift-frontend %t/CoreA.swift \
 // RUN: -module-name=CoreA -package-name Pkg \
 // RUN: -parse-as-library -emit-module \
@@ -13,31 +37,65 @@
 // RUN: -emit-module-path %t/CoreB.swiftmodule -I%t \
 // RUN: -O -wmo -enable-library-evolution
 
-// RUN: %target-swift-frontend %t/Lib.swift \
-// RUN: -module-name=Lib -package-name Pkg \
+// RUN: %target-swift-frontend %t/UI.swift \
+// RUN: -module-name=UI -package-name Pkg \
 // RUN: -parse-as-library -emit-module \
 // RUN: -experimental-spi-only-imports \
-// RUN: -emit-module-path %t/Lib.swiftmodule -I %t \
+// RUN: -emit-module-path %t/UI.swiftmodule -I %t \
 // RUN: -experimental-package-cmo -experimental-allow-non-resilient-access \
-// RUN: -O -wmo -enable-library-evolution -Rmodule-loading 2> %t/Lib-result.txt
-// RUN: %target-sil-opt %t/Lib.swiftmodule -I %t -sil-verify-all -o %t/Lib.sil
-// RUN: %FileCheck %s < %t/Lib.sil
+// RUN: -O -wmo -enable-library-evolution -Rmodule-loading 2> %t/UI-result.txt
+// RUN: %target-sil-opt %t/UI.swiftmodule -I %t -sil-verify-all -o %t/UI.sil
+// RUN: %FileCheck %s < %t/UI.sil
 
 // REQUIRES: swift_in_compiler
 
+//--- Client.swift
+public import Utils
+
+package func clientFunc(_ arg: PkgKlass) {
+  print(arg.second as! AnyObject)
+}
+
+//--- UtilsA.swift
+public import Foundation
+
+package class PkgKlass {
+  /// Serialized since it does not reference a type from module imported as @_implementationOnly.
+  // PkgKlass.first.getter
+  // CHECK-UTILS-DAG: sil package [serialized_for_package] [canonical] [ossa]  @$s5Utils8PkgKlassC5firstSSvg : $@convention(method) (@guaranteed PkgKlass) -> @owned String {
+  package var first: String
+
+  /// Not serialized since it references a type from module imported as @_implementationOnly.
+  // PkgKlass.second.getter
+  // CHECK-UTILS-DAG: sil package_external [canonical] @$s5Utils8PkgKlassC6secondSo8NSObjectCvg : $@convention(method) (@guaranteed PkgKlass) -> @owned NSObject
+  package var second: NSObject
+
+  package init(_ arg1: String, _ arg2: NSObject) {
+    self.first = arg1
+    self.second = arg2
+  }
+}
+
+//--- UtilsB.swift
+@_implementationOnly import Foundation
+
+public func utilsFunc() {
+  let x: NSString = "utilsfunc"
+  print(x)
+}
 
-//--- Lib.swift
+//--- UI.swift
 package import CoreA
 @_spiOnly public import CoreB
 
 /// PkgStruct is imported with `package import` and should be serialized.
-// CHECK-DAG: sil package [serialized_for_package] [canonical] [ossa] @$s3Lib7libFuncyy5CoreA9PkgStructVF : $@convention(thin) (@in_guaranteed PkgStruct) -> () {
-package func libFunc(_ arg: PkgStruct) {
+// CHECK-DAG: sil package [serialized_for_package] [canonical] [ossa] @$s2UI6uiFuncyy5CoreA9PkgStructVF : $@convention(thin) (@in_guaranteed PkgStruct) -> () {
+package func uiFunc(_ arg: PkgStruct) {
   print(arg.pkgVar)
 }
 
 /// PubStruct is imported with `@_spiOnly public import` and should be serialized.
-// CHECK-DAG: sil [serialized_for_package] [canonical] [ossa] @$s3Lib7spiFuncyy5CoreB15PubStructForSPIVF : $@convention(thin) (@in_guaranteed PubStructForSPI) -> () {
+// CHECK-DAG: sil [serialized_for_package] [canonical] [ossa] @$s2UI7spiFuncyy5CoreB15PubStructForSPIVF : $@convention(thin) (@in_guaranteed PubStructForSPI) -> () {
 @_spi(InCoreB)
 public func spiFunc(_ arg: PubStructForSPI) {
   print(arg.pubVarForSPI)