ref(develop): Update Rust async trait section #11950
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
Co-authored-by: Riccardo Busetti <riccardob36@gmail.com>
| trait Database { | ||
| fn get_user(&self) -> Pin<Box<dyn Future<Output = User> + Send + '_>>; | ||
| pub trait Database { | ||
| fn get_user(&self) -> impl Future<Output = User> + Send; |
There was a problem hiding this comment.
Wanna add a callout here why we should be explicit with Send and Sync and prefer impl Future over async for that reason in trait definitions?
Personally, I'd add a recommendation to use associated types where possible (especially for Iterators!) as this is extremely useful for composition in the absence of generators, but for async there's simply no need to create named futures in most cases.
There was a problem hiding this comment.
Wanna add a callout here why we should be explicit with Send and Sync and prefer impl Future over async for that reason?
I kept this message, hoping it would address this clearly enough:
Note that the returned future type is
Send, to ensure that it can run on a multi-threaded runtime.
Personally, I'd add a recommendation to name types where possible (especially for Iterators!) as this is extremely useful for composition in the absence of generators [...]
This requires explicit associated types until this restriction is lifted. I think for most of Sentry's (application) code this would be overkill. I can add another note that explains that as well.
|
Merging this, can work on the GAT variant in a followup. |
DESCRIBE YOUR PR
Async traits exist for a year now. We should use them.
IS YOUR CHANGE URGENT?
Help us prioritize incoming PRs by letting us know when the change needs to go live.
SLA
Thanks in advance for your help!
PRE-MERGE CHECKLIST
Make sure you've checked the following before merging your changes: