Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove @_implementationOnly imports #666

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Remove @_implementationOnly imports #666

wants to merge 3 commits into from

Conversation

rauhul
Copy link
Contributor

@rauhul rauhul commented Sep 17, 2024

The @_implementationOnly attribute does not work correctly without library evolution enabled. The only possible client with this enabled is Apple, however they have updated to the Swift 6 compiler and can use internal import instead of this attribute. Therefore there is no need for this package to use the attribute anymore.

The `@_implementationOnly` attribute does not work correctly without
library evolution enabled. The only possible client with this enabled is
Apple, however they have updated to the Swift 6 compiler and can use
`internal import` instead of this attribute. Therefore there is no need
for this package to use the attribute anymore.
@rauhul rauhul requested a review from natecook1000 September 17, 2024 05:34
@rauhul
Copy link
Contributor Author

rauhul commented Sep 17, 2024

cc @dabrahams

@rauhul
Copy link
Contributor Author

rauhul commented Sep 17, 2024

@swift-ci test

internal import ArgumentParserToolInfo
internal import class Foundation.JSONEncoder
#elseif swift(>=5.10)
#else
import ArgumentParserToolInfo
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly concerned about this. The ArgumentParserToolInfo library is statically built, and therefore not distributed. However, this is going to serialise the dependency information into the swiftmodule. This might therefore break builds with swift <6.0 on Windows. I do have a pending change (swiftlang/swift#63814) that would help with this situation, but I was running into some trouble with Linux builds ...

Copy link
Contributor Author

@rauhul rauhul Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm implementation only import causes real miscompiles without library evolution enabled, so I think it really really needs to go.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rauhul Where are we seeing this issue show up right now? Can we gate this change so that the attribute is still used on Windows/Linux?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theres a ton of context in this radar: rdar://78129903 (@_implementationOnly should either work in non-library-evolution-enabled modules, or be disallowed because we know it doesn't work) (sorry non-apple folks).

The jist of it is that this attribute has never properly worked without library evolution and introduces very hard to locate miscompiles due to different compilation units disagreeing about the layout of types.

@mikeash thoughts?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this didn't cause problems all the time. It looks like the issue is when a public struct contains a field that came from a @_implementationOnly module, so maybe this code just doesn't do that, and so it manages to get away with it. Or maybe the fact that the imported types are only protocols and classes avoids the issue.

If this does in fact break builds, and works as-is, then maybe the best move is to keep this for 5.9 and earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I don't understand the benefits of using this attribute at all for non library evo code

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure myself! Looking at @compnerd's PR, it might break builds by emitting different autolinking info?

@rauhul
Copy link
Contributor Author

rauhul commented Nov 6, 2024

@compnerd how frustrating would it be to attempt merging this PR and reverting if it breaks windows?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants