Use the `package` access level instead of `@testable` imports #1215

ahoppen · 2025-03-17T23:51:33Z

@testable imports have a number of build-time drawbacks, including:

Building the package in release configuration and then running tests requires the entire package to be rebuilt because SwiftPM needs to pass --enable-testing to the compiler invocations.
The usage of @testable means that swift-foundation cannot be included in the unified (aka. multiroot) build of all the other packages in Swift CI, which means that it needs to re-build swift-syntax, unnecessarily increasing CI time.

To fix this, make all @testable imports normal imports and mark the declarations that were previously accessed through @testable as package.

itingliu · 2025-03-18T01:25:08Z

@swift-ci please test

jmschonfeld

One overall comment: I just want to make sure that in all scenarios (building as a package, being built as part of the toolchain etc.) that making these package will not allow any client to start using them (unless they're doing some @_silgen_name shenanigans) since we don't actually install the package swift interface file anywhere, right?

jmschonfeld · 2025-03-20T17:08:25Z

Sources/FoundationEssentials/AttributedString/AttributedString+Guts.swift

@@ -30,7 +30,7 @@ extension AttributedString {
        typealias _AttributeStorage = AttributedString._AttributeStorage

        var version: Version
-        var string: BigString
+        package var string: BigString


It looks like this is only used from one place: NumberFormatStyleTests which could easily do String(self.characters) instead of String(self._guts.string) which would save us from needing to mark this and the imports above as package.

For cases like this, would you rather we update them in a separate change and keep this as just a @testable --> package change, or would you want to incorporate that into this to reduce the number of things that we import as package like the collections modules above?

I’d prefer if this is just a mechanical @testable -> package change. It’s fairly big already and I don’t want to hide any other functionality change in it.

So, if you could make those changes up-front, that would be great.

jmschonfeld · 2025-03-20T17:09:26Z

Sources/FoundationEssentials/Calendar/Calendar_Gregorian.swift

@@ -38,7 +38,7 @@ extension Date {
        timeIntervalSinceReferenceDate / 86400 + Self.julianDayAtDateReference
    }

-    func julianDay() throws (GregorianCalendarError) -> Int {
+    package func julianDay() throws (GregorianCalendarError) -> Int {


I don't see uses of this, the initializer below, or the error type from our tests, was this an accidental addition?

It’s used here:

swift-foundation/Tests/FoundationEssentialsTests/GregorianCalendarTests.swift

Line 69 in 3179457

_ = d.julianDay

That being said, the test should probably call julianDay instead of just getting a function reference to it 😉

Ah yeah that's probably why I missed it in my search since it didn't actually call the function - ok thanks for the explanation

jmschonfeld · 2025-03-20T17:13:20Z

Sources/FoundationEssentials/FileManager/SwiftFileManagerDelegate.swift

@@ -35,7 +35,7 @@ public protocol FileManagerDelegate : AnyObject, Sendable {
 }

 // Default implementations for String-based requirements
-extension FileManagerDelegate {
+package extension FileManagerDelegate {


I think this should actually be public instead of just package visibility, since we want clients to get these default implementations too (I guess this goes to show how uncommon it is for people to use this delegate on Linux/Windows)

Similar here, do you want to make a separate PR for that so we don’t hide functionality change in here?

Yeah we can put this in another PR

jmschonfeld · 2025-03-20T17:17:21Z

Tests/FoundationEssentialsTests/AttributedString/AttributedStringCOWTests.swift

@@ -15,9 +15,9 @@ import TestSupport
 #endif

 #if FOUNDATION_FRAMEWORK
-@testable import Foundation


Since TestSupport has an @_exported import Foundation, we can just remove these few lines entirely. The only reason they're spelled out here is because @_exported doesn't "re-export" the @testable-ness of the import. Same goes for any files that have changes like this

Removed all imports to Foundation(Essentials|Internationalization|) when TestSupport is imported here: a23e682

jmschonfeld · 2025-03-20T17:18:17Z

Tests/FoundationInternationalizationTests/Formatting/ICUPatternGeneratorTests.swift

 #endif

 final class ICUPatternGeneratorTests: XCTestCase {

-    typealias DateFieldCollection = Date.FormatStyle.DateFieldCollection
+    package typealias DateFieldCollection = Date.FormatStyle.DateFieldCollection


Is this needed? The test module isn't a dependency of any other module, so it shouldn't need anything more permissive than internal

Yes, I changed this by mistake

`@testable` imports have a number of build-time drawbacks, including: - Building the package in `release` configuration and then running tests requires the entire package to be rebuilt because SwiftPM needs to pass `--enable-testing` to the compiler invocations. - The usage of `@testable` means that swift-foundation cannot be included in the unified (aka. multiroot) build of all the other packages in Swift CI, which means that it needs to re-build swift-syntax, unnecessarily increasing CI time. To fix this, make all `@testable` imports normal imports and mark the declarations that were previously accessed through `@testable` as `package`. a

…TestSupport` is imported TestSupport has `@_exported` imports for those modules.

ahoppen · 2025-03-21T23:19:53Z

One overall comment: I just want to make sure that in all scenarios (building as a package, being built as part of the toolchain etc.) that making these package will not allow any client to start using them (unless they're doing some @_silgen_name shenanigans) since we don't actually install the package swift interface file anywhere, right?

I am pretty sure that we don’t install .package.swiftinterface but to double-check I just kicked off toolchain builds in swiftlang/swift#37710 and will check them.

ahoppen · 2025-03-24T21:12:21Z

Looks like the package symbols are being successfully stripped.

I checked the Linux build:

ls -lah usr/lib/swift/linux/Foundation.swiftmodule 
total 9912
drwxr-xr-x@  4 alex  staff   128B Mar 21 17:23 ./
drwxr-xr-x@ 67 alex  staff   2.1K Mar 24 13:43 ../
-rw-r--r--@  1 alex  staff   278K Mar 21 17:18 x86_64-unknown-linux-gnu.swiftdoc
-rw-r--r--@  1 alex  staff   4.6M Mar 21 17:18 x86_64-unknown-linux-gnu.swiftmodule

Grepping both .swiftmodule and .swiftdoc for numericStringRepresentationForBinaryInteger did not return any results

Windows also only contains .swiftmodule files (no .swiftinterface). macOS toolchains don’t contain Foundation, so there’s nothing to check here.

jmschonfeld · 2025-03-24T21:28:24Z

@swift-ci please test

parkera

I think it is extremely unfortunate to lose the meaning of what internal vs. package means, even for the benefit of improved CI times. This will also make all of these exported symbols on Darwin. Let's look for another solution.

ahoppen requested a review from jmschonfeld March 17, 2025 23:51

ahoppen force-pushed the no-testable-imports branch from 559c06b to b9a6d4d Compare March 18, 2025 16:08

ahoppen mentioned this pull request Mar 18, 2025

[tests] Run swift-(corelibs-)foundation tests in debug configuration on Windows swiftlang/swift#80076

Merged

jmschonfeld reviewed Mar 20, 2025

View reviewed changes

ahoppen added 2 commits March 21, 2025 16:18

Remove imports of Foundation(Essentials|Internationalization|) when `…

0ffac98

…TestSupport` is imported TestSupport has `@_exported` imports for those modules.

ahoppen force-pushed the no-testable-imports branch from a23e682 to 0ffac98 Compare March 21, 2025 23:18

ahoppen mentioned this pull request Mar 21, 2025

[DO NOT MERGE] Dummy PR for CI testing swiftlang/swift#37710

Draft

parkera requested changes Mar 24, 2025

View reviewed changes

ahoppen closed this Mar 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the `package` access level instead of `@testable` imports #1215

Use the `package` access level instead of `@testable` imports #1215

ahoppen commented Mar 17, 2025

itingliu commented Mar 18, 2025

jmschonfeld left a comment

jmschonfeld Mar 20, 2025

ahoppen Mar 21, 2025 •

edited

Loading

jmschonfeld Mar 20, 2025

ahoppen Mar 21, 2025 •

edited

Loading

jmschonfeld Mar 22, 2025

jmschonfeld Mar 20, 2025

ahoppen Mar 21, 2025

jmschonfeld Mar 22, 2025

jmschonfeld Mar 20, 2025

ahoppen Mar 21, 2025

jmschonfeld Mar 20, 2025

ahoppen Mar 21, 2025

ahoppen commented Mar 21, 2025

ahoppen commented Mar 24, 2025

jmschonfeld commented Mar 24, 2025

parkera left a comment

Use the package access level instead of @testable imports #1215

Use the package access level instead of @testable imports #1215

Conversation

ahoppen commented Mar 17, 2025

itingliu commented Mar 18, 2025

jmschonfeld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoppen Mar 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoppen Mar 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoppen commented Mar 21, 2025

ahoppen commented Mar 24, 2025

jmschonfeld commented Mar 24, 2025

parkera left a comment

Choose a reason for hiding this comment

Use the `package` access level instead of `@testable` imports #1215

Use the `package` access level instead of `@testable` imports #1215

ahoppen Mar 21, 2025 •

edited

Loading

ahoppen Mar 21, 2025 •

edited

Loading