Add unit test for surfacing potential regression from #5728

[ncooke3]

Adds a currently failing unit test to highlight one potential regression from #5728.

I stumbled upon this when building up the test suite in #5919.

Motivation:

#5728 introduced using @_implementationOnly when importing Foundation.Bundle in the generated resource bundle accessor. For Swift packages that import Foundation without @_implementationOnly, this seems to cause the following warning:

UtilsWithFoundationPkg/Sources/UtilsWithFoundationPkg/FooUtils.swift:1:8: error: 'Foundation' inconsistently imported as implementation-only
    import Foundation
           ^
    @_implementationOnly 

UtilsWithFoundationPkg.build/DerivedSources/resource_bundle_accessor.swift:1:35: note: imported as implementation-only here
    @_implementationOnly import class Foundation.Bundle

This is reproducible when using the latest 5.8 toolchain and running the unit test added in this PR.

It seems that, by using @_implementationOnly in the resource bundle accessor to import Foundation, it causes a warning if another swift file in the module imports Foundation without it. Ideally, the @_implementationOnly in the resource bundle accessor should be ignored when doing this check. More info w.r.t. the warning in this bug: swiftlang/swift#56093.

Modifications:

Add a unit test that is currently failing but should pass. The proper implementation fix isn't obvious to me but I wanted to open this both for reproduction and tracking purposes.

Result:

Assuming CI is using the 5.8 toolchain, it should highlight this regression.

cc: @xwu @neonichu

[xwu]

😔

I'm not sure this warning should be triggered at all when the imports do not match exactly... Or, we should have some way to silence it in code, defaulting to parens.

[ncooke3]

Starting a thread to discuss...

@xwu: I'm not sure this warning should be triggered at all when the imports do not match exactly... Or, we should have some way to silence it in code, defaulting to parens.

I did some researching and there is some good discussion in https://forums.swift.org/t/update-on-implementation-only-imports/26996/12 as to why the warning is triggered in the first place. What makes this tricky is that I do not think there is a way to pass a flag to silence only this warning.

[xwu]

Understand the rationale, but this issue points to a use case where the current design doesn’t serve our needs (and, incidentally, if it were ever stabilized as an “access level” on import like once pitched, wouldn’t behave like access modifiers do elsewhere).

I posted some thoughts over in the linked PR about why even import scanning would be brittle, and I think possibly the most resilient solution here is to come up with a new “flavor” of @_implementationOnly (rather than a flag) that explicitly acknowledges that it can be overridden or “upgraded.”

[tomerd]

cc @neonichu

[neonichu]

Thanks @ncooke3, this is a good point.

We could do an import scan of modules which are using resources and adjust the generated code based on whether they import Foundation themselves or not, though the existing facility we have for this in SwiftPM actually does not tell us whether the imports are implementation-only themselves (which the previous version of the generated didn't address, either).

Given where we are in the 5.8 schedule, we may want to disable this change for the 5.8 tools-version and move it to vNext for now.

[tomerd]

@swift-ci smoke test

[neonichu]

@swift-ci smoke test macOS

[MaxDesiatov]

Are there any other updates needed for this PR?

[ncooke3]

@MaxDesiatov, nothing from my end.

[MaxDesiatov]

@swift-ci smoke test

[ncooke3]

@MaxDesiatov, good to merge?

[MaxDesiatov]

This can't be merged until it has approvals from reviewers.

[ncooke3]

Oh right, my bad–– cc: @neonichu

[neonichu]

LGTM, but I wanted to run the test locally before approving since the FunctionalTests are still disabled.

[neonichu]

Sorry for dropping the ball on this, functional tests are back enabled now, so I am just going to run the CI again here.

[neonichu]

@swift-ci smoke test

[ncooke3]

Sorry for dropping the ball on this, functional tests are back enabled now, so I am just going to run the CI again here.

No worries– the main point of this PR was to highlight/demonstrate the issue! 🙂 And that's great about the functional tests, I'll get my mixed language branch synced up so the functional tests I added can run on CI.

[neonichu]

@swift-ci smoke test linux