Add Sendable conformance#582
Conversation
vlm
force-pushed
the
add-sendable-conformance
branch
from
8e9e8af to
8b68689
Compare
|
Thanks for this, @vlm! Do we not need to declare the |
|
@natecook1000 looking... |
vlm
force-pushed
the
add-sendable-conformance
branch
from
8b68689 to
aec234d
Compare
|
@natecook1000 these conformances doesn't appear to be required. Added a test to showcase that. No warnings. |
This derives the `hasUpdated` check from the parsed values data type, rather than storing it in the closure (which breaks sendability) or passing it through the closure invocation (which wasn't finished enough to actually work).
This allows for a stronger, compiler-supported guarantee of sendability when a compound `ParsableArguments` or `ParsableCommand` type is marked `Sendable`. Most transformations shouldn't be a problem, since the general case is that these are pure string -> value transformations. In cases where making such a transformation sendable is impossible, an author can always change the property to be just a string and perform the transformation within the context of the command's execution, in either the `run()` or `validate()` methods.
|
@swift-ci Please test |
|
Is it possible to put the additional sendable restrictions behind conditional compilation so this change isn't source breaking? |
| dependencies: ["ArgumentParserToolInfo"], | ||
| exclude: ["CMakeLists.txt"]), | ||
| exclude: ["CMakeLists.txt"], | ||
| swiftSettings: [.unsafeFlags(["-Xfrontend", "-strict-concurrency=complete"])] |
There was a problem hiding this comment.
This is a "very bad idea" ™️ - anyone who pulls in the 5.6 manifest will see this propagated to their library or application and potentially introduce compiler errors and warnings.
We solved this in Vapor with a 5.9 specific manifest using the experimental feature flag which restricts it to that package - e.g. https://github.com/vapor/vapor/blob/main/Package%40swift-5.9.swift#L96
There was a problem hiding this comment.
Definitely! Thanks for that note — I was planning to just remove the flag, and didn't realize that enableExperimentalFeature allows us to keep the setting package-local.
|
@rauhul None of these changes should be source breaking, since checking is only a warning until you upgrade to Swift 6 mode. Additionally, to turn off the warnings making other source changes, a package that depends on ArgumentParser can add |
This adds the `@preconcurrency` attribute to all public APIs that have changed to take a `@Sendable` closure. This will ease the migration path for sendable adoption for ArgumentParser users, since a warning will only appear for using these APIs (like the `transform` parameter in an @option or @argument) once they've turned on strict concurrency checking. I'm also backing out changes that avoided those warnings in the tests and examples, since in most cases those warnings are spurious; unapplied functions don't capture state. See https://forums.swift.org/t/pitch-inferring-sendable-for-methods-and-key-path-literals/68011 for more on this and hopefully an upcoming fix for these issues.
In order to provide `@preconcurrency` support, the package needs to have a minimum Swift requirement of 5.7. This makes that change and updates the README to indicate this for the next version.
natecook1000
force-pushed
the
add-sendable-conformance
branch
from
de76384 to
2812f57
Compare
|
@swift-ci Please test |
4 participants
It can be useful to send
AsyncParsableCommandas an argument to some function. This is trivial to do in a non-async context. However, inasynccontext the type that is sent as an argument has to be Sendable. This patch addsSendableconformance so this becomes expressible and warning-free:The warnings will become errors in Swift 6, so this is a forward-looking change.
Checklist