[Pigeon] add Sendable conformance and @Sendable closure annotations - #12902
LongCatIsLooong wants to merge 6 commits into
Conversation
| swiftFunction: method.swiftFunction, | ||
| documentationComments: method.documentationComments, | ||
| serialBackgroundQueue: method.taskQueueType == TaskQueueType.serialBackgroundThread | ||
| serialBackgroundQueue: method.taskQueueType == .serialBackgroundThread |
There was a problem hiding this comment.
This is for generating the host API message handler implementation in pigeon.
| returnType: method.returnType, | ||
| isAsynchronous: method.isAsynchronous, | ||
| isAsynchronousCallback: true, | ||
| isCompletionClosureSendable: true, |
There was a problem hiding this comment.
This is inside
// Writes the delegate method that handles making a call from for a host
// method.
void _writeProxyApiHostMethodDelegateMethods(
| /// | ||
| /// Accessing and inserting to an InstanceManager is thread safe. | ||
| final class ${swiftInstanceManagerClassName(options)} { | ||
| final class ${swiftInstanceManagerClassName(options)}: @unchecked Sendable { |
There was a problem hiding this comment.
@unchecked because it uses locking primitives that swift concurrency doesn't understand (so the compiler can't prove the correctness).
Sendable because the public interface is thread safe and the class is final.
| isAsynchronous: method.isAsynchronous, | ||
| isAsynchronousCallback: method.isAsynchronousCallback, | ||
| swiftFunction: method.swiftFunction, | ||
| isCompletionClosureSendable: !generatorOptions.useFfi, |
There was a problem hiding this comment.
This is for generating method requirement signatures in the host API protocol. The completion closure is always sendable (unless using ffi) because FlutterReply is thread-safe.
| _ everything: AllTypes, completion: @escaping @Sendable (Result<AllTypes, Error>) -> Void) | ||
| func echoNullable( | ||
| _ aString: String?, completion: @escaping @Sendable (Result<String?, Error>) -> Void) | ||
| func throwError(completion: @escaping @Sendable (Result<Any?, Error>) -> Void) |
There was a problem hiding this comment.
It's possible that we can make this
func throwError(completion: sending @escaping (Result<Any?, Error>) -> Void)so the caller can send non-sendable results via completion block,
but sending is only available in Swift 6.0+.
Additionally, these methods are called by pigeon generated code so we can implement workarounds in pigeon generated code if needed (so the ugliness lives in pigeon generated code).
There was a problem hiding this comment.
Also using sending can break implementors (callers of completion), if they call completion.success on a value stored as an instance variable (so it violates the sending contract).
Ok looks like this is incorrect. sending doesn't imply the parameters are also sending. So the only caveat is sending requires Swift 6.0+
LongCatIsLooong
changed the title
Sendable conformance and @Sendable annotationsSendable conformance and @Sendable closure annotations
| @@ -1,3 +1,9 @@ | |||
| ## 29.1.0 | |||
There was a problem hiding this comment.
Technically this still can break existing clients, but it's unlikely so I only bumped minor version.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates Pigeon to version 29.1.0, marking generated Swift completion closures for host APIs and ProxyApi host method delegates as @Sendable, and marking the generated InstanceManager as @unchecked Sendable. Feedback points out that using @Sendable closures with non-Sendable types like FlutterStandardTypedData or Any may trigger compiler warnings under Swift 6 strict concurrency, and suggests adding retroactive conformances or documenting these limitations.
| final sendablePrefix = isCompletionClosureSendable ? '@Sendable ' : ''; | ||
| final completion = | ||
| 'completion: @escaping $sendablePrefix(Result<$returnTypeString, $errorTypeName>) -> Void'; |
There was a problem hiding this comment.
While adding @Sendable to completion closures is a key step toward Swift concurrency support, using @Sendable closures with non-Sendable types like Any (used for Object), [Any?] (used for List), or FlutterStandardTypedData will trigger compiler warnings or errors under Swift 6 / strict concurrency.
To make this fully compatible with strict concurrency:
FlutterStandardTypedData: Consider generating a retroactive conformance in the helper section:extension FlutterStandardTypedData: @unchecked Sendable {}
Any/ Collections: SinceAnyis notSendable, closures usingResult<Any, Error>orResult<[Any?], Error>are technically not fully sendable-safe. We should consider if these types can be migrated toSendable(i.e.,Any & Sendable) in the future, or document this limitation.
There was a problem hiding this comment.
see: #12902 (comment),
With this change, client code doesn't have to deal with non-sendable types, as all relevant call sites are only in generated code. There will be warnings in pigeon generated code but unless the client turns on full strict concurrency for the target, it does not block compilation.
1 participant
Fixes flutter/flutter#140439, but this PR alone doesn't allow people to turn on fully strict concurrency (or Swift 6 language mode).
Extracted from #12885 (adding isolation annotations is trickier, because the Swift compiler always treat isolation violations as errors regardless of the compiler flags specified. See the open questions in that PR), this PR adds
Sendablein generated code based on the underlying platform channel threading contract. This does not affect FFI generated code.This DOES NOT add
Sendableconformances to pigeon-generated data classes:for Swift
structs, the compiler can infer the senability (astructis sendable if all fields are sendable)for non-final
classes (pigeon generated class are notfinal), they shouldn't be marked asSendablesince subtypes can introduce non-thread-safe APIs.Client code can use extensions to add
@unchecked Sendableto pigeon generated data classes if so desired, since client code will be in the same Swift target as the pigeon generated data classes. Also@uncheckis required anyway because the compiler can't prove the type is sendable.Breaking Change?
This change assumes the plugin has not enabled complete strict concurrency checking for the Swift target. When strict concurrency check is not set to complete, sendability diagnostics are warnings (or ignored) in Swift 5 language mode, so the opt-out can be done via compiler flags.
Adding sendability annotations can still be breaking if the client has already enabled fully strict concurrency checking for the swift target. But it is unlikely: with the pigeon generated swift code being in the same target as the client code, it is unlikely for the client target to enable full strict concurrency checking, unless the client code followed the (incorrect) threading contract currently generated by pigeon.
Versioning?
Tentatively bumped the version to
29.1.0but if this is deemed a breaking change I can bump this to30.0.0.Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2