Remove @_implementationOnly imports#666
Conversation
|
cc @dabrahams |
|
@swift-ci test |
| internal import class Foundation.JSONEncoder | ||
| #elseif swift(>=5.10) | ||
| #else | ||
| import ArgumentParserToolInfo |
There was a problem hiding this comment.
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.
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.
@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.
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.
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.
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.
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? |
d409af5 to
812d575
Compare
|
@swift-ci test |
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.
|
@swift-ci test |
|
I'm merging this with the understanding that it may need to be reverted |
The
@_implementationOnlyattribute 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 importinstead of this attribute. Therefore there is no need for this package to use the attribute anymore.