-
Notifications
You must be signed in to change notification settings - Fork 326
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
base: main
Are you sure you want to change the base?
Conversation
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.
cc @dabrahams |
@swift-ci test |
internal import ArgumentParserToolInfo | ||
internal import class Foundation.JSONEncoder | ||
#elseif swift(>=5.10) | ||
#else | ||
import ArgumentParserToolInfo |
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
@compnerd how frustrating would it be to attempt merging this PR and reverting if it breaks windows? |
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 useinternal import
instead of this attribute. Therefore there is no need for this package to use the attribute anymore.