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.

Sign up for GitHub

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

Jump to bottom

Remove @_implementationOnly imports #666

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+6 −14
Open

Remove @_implementationOnly imports #666

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b3cdd6
Remove `@_implementationOnly` imports
rauhul Sep 17, 2024
b51c08d
ThaMerge branch 'main' into no-implonly
rauhul Sep 30, 2024
8fe3852
Merge branch 'main' into no-implonly
rauhul Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions Sources/ArgumentParser/Usage/DumpHelpGenerator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,12 @@
//
//===----------------------------------------------------------------------===//

#if swift(>=5.11)
#if swift(>=6.0)
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 •
edited
Loading

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?

import class Foundation.JSONEncoder
#else
@_implementationOnly import ArgumentParserToolInfo
@_implementationOnly import class Foundation.JSONEncoder
#endif

internal struct DumpHelpGenerator {
Expand Down
7 changes: 2 additions & 5 deletions Sources/ArgumentParser/Usage/MessageInfo.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,12 @@
//
//===----------------------------------------------------------------------===//

#if swift(>=5.11)
#if swift(>=6.0)
internal import protocol Foundation.LocalizedError
internal import class Foundation.NSError
#elseif swift(>=5.10)
#else
import protocol Foundation.LocalizedError
import class Foundation.NSError
#else
@_implementationOnly import protocol Foundation.LocalizedError
@_implementationOnly import class Foundation.NSError
#endif

enum MessageInfo {
Expand Down
6 changes: 2 additions & 4 deletions Sources/ArgumentParser/Usage/UsageGenerator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,10 @@
//
//===----------------------------------------------------------------------===//

#if swift(>=5.11)
#if swift(>=6.0)
internal import protocol Foundation.LocalizedError
#elseif swift(>=5.10)
import protocol Foundation.LocalizedError
#else
@_implementationOnly import protocol Foundation.LocalizedError
import protocol Foundation.LocalizedError
#endif

struct UsageGenerator {
Expand Down