Use '@_implementationOnly' when importing 'Foundation.Bundle' in generated resource bundle accessor #5728

xwu · 2022-08-16T18:55:29Z

Motivation:

As per discussion in a Swift Forums thread, use of Foundation.Bundle causes Foundation APIs to be implicitly imported into the code of downstream users of the package.

This is counterintuitive and can cause unexpected code size increases when those APIs extend standard library types and are inadvertently used, without any indication in searchable source where the Foundation dependency is coming from.

Modifications:

We mark the import of Foundation.Bundle in the generated resource bundle accessor as @_implementationOnly; the extension is implicitly internal, so hopefully this will do the trick. In this draft, it's gated on the tools version, but arguably shouldn't be.

Result:

We'll need to test that this actually solves the problem, and to add this test somewhere. Guidance on this point would be welcome.

…rated resource bundle accessor

xwu · 2022-08-16T18:55:59Z

@swift-ci test

MaxDesiatov · 2022-08-17T11:14:10Z

@swift-ci please smoke test

neonichu · 2022-08-17T16:48:27Z

Thanks for your PR!

I think a good way for testing would be to look at the existing tests in Tests/FunctionalTests which are essentially integration tests using fixture packages.

We could have an example which looks like this:

a library package which has at least one resource
a client package that depends on the library and uses some Foundation API without importing Foundation

The expectation would be that the build fails because Foundation is no longer re-exported from the library with resources.

abertelrud · 2022-09-21T21:05:28Z

@xwu Can you add a test here so that we can merge this PR? Thanks!

xwu · 2022-09-21T21:32:18Z

Thanks for the reminder. I'll try to take a look this weekend.

xwu · 2022-09-27T08:12:46Z

Ugh, I always seem to have trouble with the CI bot in this repository.

Tests/FunctionalTests/ResourcesTests.swift

tomerd · 2022-09-27T16:46:49Z

@swift-ci smoke test

xwu · 2022-09-27T17:41:28Z

@swift-ci smoke test

xwu · 2022-09-27T17:47:12Z

Groovy, the Linux self-hosted testing seems to suggest that it all works... I was pessimistic.

…dle'.

xwu · 2022-09-28T03:52:35Z

@swift-ci smoke test

xwu · 2022-09-28T04:22:29Z

@neonichu @tomerd

All righty, I think this PR is ready modulo a policy decision: What is the proper tools version gating for this change, if any?

It is technically possible to emit @_implementationOnly import regardless of tools version because @_implementationOnly long pre-dates SwiftPM resource support. On the one hand, that would be a flavor of source breaking. However, it is an odd sort of source breaking because the affected end user's source code (using a Foundation API without importing Foundation) was always ill-formed and is rejected by every version of the Swift compiler but for invisible code emitted into dependencies by SPM.

While I am firmly in agreement with the overarching project's policy not to break source absent a new language version, this issue is unique because it comes down to tooling, and applying the same policy to tools version comes with a significant drawback here: The problem at issue is inadvertently bringing in Foundation for the end user, but whether this fix is applied would be down to third-party dependencies upgrading to the latest tools version.

That is to say, shipping this gated on the tools version would effectively put the fix out of the reach of impacted users for an indefinite amount of time. If it were my code involved, I'd prefer the first scenario (a technically source-breaking error that tells me to write import Foundation or choose to use non-Foundation APIs, which would be entirely within my power to address immediately) than the second scenario (an unwanted Foundation dependency which it is not within my power to address short of forking an unspecified number of packages I depend on in order to upgrade their tools version).

I'll await your guidance as to how to proceed; if you think the language workgroup should weigh in too, let me know and I will bring it to them.

neonichu · 2022-09-28T06:14:06Z

Thanks again for your PR!

To me it's pretty clear that this has to be guarded by the tools-version, because

any breakage like this will be impossible to address for any package version that has previously shipped since they're by definition immutable (this is the main reason why we are trying our best to not change the behavior present in older tools-versions)
a large portion of our user base is on Darwin platforms where this won't really matter in the majority of cases
the only supported way to access resources on any platform is via Foundation.Bundle which means using a resource currently always implies a Foundation dependency (this is might be something we want to further refine)

On top of that, I don't think this will on its own eliminate the Foundation dependency in practice? The generated code will still be present and therefore Foundation will be linked to any clients, wouldn't it?

BTW, I think we need to switch to vNext as the guard since 5.7 has shipped and we don't have a constant for the next version, yet. You should be able to switch the test cases to 999.0 as their tools-version to opt-in to the vNext behavior.

xwu · 2022-09-28T07:29:51Z

@neonichu This situation is unique (I think) because the source-breaking effect of this PR (and the bug that prompted it) is not on the package in which the tools version is defined, but of the end user who uses the package as a dependency and writes invalid code. It is very difficult to reason about even when one explains it!

If this PR is gated on the tools version, then the tools version of the dependency will determine if the user who uses that dependency experiences this bug. It is actually this scenario that is impossible to address: it is a coming together of a suboptimal thing that an old tools version does to a dependency and an invalid thing inadvertently done by the end user of the dependency.

By contrast, if this PR is not gated on any tools version, there is actually no breakage that can't be addressed, since there is no effect on the package itself, and the end user's code that relies on the package as a dependency is by construction in the hands of the end user.

However, since it is all a fairly niche proposition, we can certainly guard on vNext if that's deemed the right way to go; I'll make that change now and run the CI bots.

What would be the right #if for gating the test itself, though? Currently, I have it as #if os(Linux) && swift(>=5.7); I can up it to swift(>=5.8) but I'd imagine the test would then be skipped in some or all of the CI testing?

… of 'Foundation.Bundle'.

xwu · 2022-09-28T07:47:54Z

@swift-ci smoke test

neonichu · 2022-09-28T17:52:18Z

I don't think we know where the breakage will occur since clients can be library packages as well. I would agree with you if this was limited to "top-level" clients that no one else could depend on.

xwu · 2022-09-28T18:24:10Z

@neonichu Good point, I guess Foundation may be currently pulled in transitively through multiple layers of dependencies, oof…

ncooke3 · 2022-12-22T02:02:29Z

I think this surfaced a warning for the case where a Swift package with resources does import Foundation without using @_implementationOnly. It's just a warning– not an error– but ideally there is a way to work around it. I have more details and a reproducible example in #5991!

This was a change that landed in #5728 but it has the unintended consequence of generating unfixable warnings for packages which do import Foundation. We can probably solve that with import scanning to decide between the two import types, but given where we are in the 5.8 schedule, this seems to be something better done for a future manifest version.

…5997) This was a change that landed in #5728 but it has the unintended consequence of generating unfixable warnings for packages which do import Foundation. We can probably solve that with import scanning to decide between the two import types, but given where we are in the 5.8 schedule, this seems to be something better done for a future manifest version.

…5997) This was a change that landed in #5728 but it has the unintended consequence of generating unfixable warnings for packages which do import Foundation. We can probably solve that with import scanning to decide between the two import types, but given where we are in the 5.8 schedule, this seems to be something better done for a future manifest version. (cherry picked from commit cb9d03b)

…5997) (#6055) This was a change that landed in #5728 but it has the unintended consequence of generating unfixable warnings for packages which do import Foundation. We can probably solve that with import scanning to decide between the two import types, but given where we are in the 5.8 schedule, this seems to be something better done for a future manifest version. (cherry picked from commit cb9d03b)

Use '@_implementationOnly' when importing 'Foundation.Bundle' in gene…

6f05199

…rated resource bundle accessor

xwu requested review from abertelrud, neonichu, tomerd and elsh as code owners August 16, 2022 18:55

MaxDesiatov mentioned this pull request Aug 17, 2022

Automatically create destination JSON to allow linking Foundation swiftwasm/carton#4

Closed

tomerd assigned neonichu Aug 23, 2022

This comment was marked as outdated.

Sign in to view

tomerd reviewed Sep 27, 2022

View reviewed changes

Tests/FunctionalTests/ResourcesTests.swift Outdated Show resolved Hide resolved

xwu force-pushed the implementation-only-foundation-bundle branch from 0fe9746 to 4934aad Compare September 27, 2022 17:41

xwu force-pushed the implementation-only-foundation-bundle branch from 4934aad to 407c762 Compare September 28, 2022 03:48

[Build] Add test for '@_implementationOnly' import of 'Foundation.Bun…

027033e

…dle'.

xwu force-pushed the implementation-only-foundation-bundle branch from 407c762 to 027033e Compare September 28, 2022 03:52

[Build] Update tools version gating for '@_implementationOnly' import…

c8998db

… of 'Foundation.Bundle'.

neonichu approved these changes Sep 28, 2022

View reviewed changes

xwu merged commit b217c04 into main Sep 28, 2022

xwu deleted the implementation-only-foundation-bundle branch September 28, 2022 18:24

xwu added a commit that referenced this pull request Sep 29, 2022

Update changelog for #5728

8a3ab6c

xwu added a commit that referenced this pull request Sep 30, 2022

Update changelog for #5728 (#5788)

318811f

ncooke3 added a commit to ncooke3/swift-package-manager that referenced this pull request Dec 22, 2022

Add regression test for swiftlang#5728

357782c

ncooke3 mentioned this pull request Dec 22, 2022

Add unit test for surfacing potential regression from #5728 #5991

Merged

neonichu mentioned this pull request Dec 22, 2022

Disable implementationOnly Foundation import for resource accessor #5997

Merged

neonichu mentioned this pull request Jan 17, 2023

[5.8] Disable implementationOnly Foundation import for resource accessor #6055

Merged

neonichu pushed a commit that referenced this pull request Apr 21, 2023

Add regression test for #5728 (#5991)

aa0670b

Use '@_implementationOnly' when importing 'Foundation.Bundle' in generated resource bundle accessor #5728

Use '@_implementationOnly' when importing 'Foundation.Bundle' in generated resource bundle accessor #5728

Uh oh!

Conversation

xwu commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation:

Modifications:

Result:

Uh oh!

xwu commented Aug 16, 2022

Uh oh!

MaxDesiatov commented Aug 17, 2022

Uh oh!

neonichu commented Aug 17, 2022

Uh oh!

abertelrud commented Sep 21, 2022

Uh oh!

xwu commented Sep 21, 2022

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

xwu commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tomerd commented Sep 27, 2022

Uh oh!

xwu commented Sep 27, 2022

Uh oh!

xwu commented Sep 27, 2022

Uh oh!

xwu commented Sep 28, 2022

Uh oh!

xwu commented Sep 28, 2022

Uh oh!

neonichu commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xwu commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xwu commented Sep 28, 2022

Uh oh!

neonichu commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xwu commented Sep 28, 2022

Uh oh!

ncooke3 commented Dec 22, 2022

Uh oh!

Uh oh!

xwu commented Aug 16, 2022 •

edited

Loading

xwu commented Sep 27, 2022 •

edited

Loading

neonichu commented Sep 28, 2022 •

edited

Loading

xwu commented Sep 28, 2022 •

edited

Loading

neonichu commented Sep 28, 2022 •

edited

Loading