[pigeon] add modern concurrency support - #12382
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the Pigeon generator to produce native asynchronous signatures (suspend in Kotlin and async throws in Swift) by default for @FlutterApi and @HostApi methods, while introducing a new @asyncCallback annotation to retain callback-style signatures. Feedback on the changes identifies a thread-safety issue in the Swift generator where spawning a Task without isolation can cause the reply callback to execute on a background thread, and suggests isolating the Task to @MainActor when no background queue is specified.
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Looks good at a high level, with the caveat that I have very little experience with the modern concurrency syntax (although certainly the ergonomics in the test plugin look way better). It might be worth making a draft PR that adopts it in one of our plugins and getting a review from Android and iOS folks so they can flag anything that looks unexpected in real use, especially since we are making this the default. (To avoid pulling the entire diff into that PR, you can use a git dependency that points to this branch).
This will also need README updates, both explaining the two modes, and also explaining that only some generators distinguish, and explaining what happens when it's not supported (which IIUC is that the two annotations do the same thing).
…-async-kotlin-suspend
stuartmorgan-g
left a comment
There was a problem hiding this comment.
LGTM
I'll leave it up to you if you want to get a draft adoption PR reviewed by platform team folks before landing, or just want to iterate in future PRs if any issues come up that the tests missed.
Adds support for modern async method styles in swift and kotlin. This is now the default. Also adds a new annotation
AsyncCallbackthat reverts to the old style.precursor to #11352