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.

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

Feature-gate async_trait in lieu of new, standardized async_fn_in_trait feature. #236

Open
Qix- opened this issue Jan 29, 2024 · 5 comments

Comments

@Qix-
Copy link

Qix- commented Jan 29, 2024

Would it be possible to put the async-trait stuff behind a feature of the same name, and then use a #[cfg_attr(feature = "async-trait", ::async_trait::async_trait)] for all occurrences, such that we can use the newly stabilized async-in-trait functionality?

@Sherlock-Holo
Copy link
Contributor

for now trait-variant can't handle default methods, but I think if it support, russh can use AFIT directly instead of the async-trait crate

@Eugeny
Copy link
Owner

Eugeny commented Feb 4, 2024

Sadly this doesn't work yet as native async trait futures aren't Send and can't be used to spawn tokio tasks: https://blog.rust-lang.org/inside-rust/2022/11/17/async-fn-in-trait-nightly.html#limitation-spawning-from-generics

@indietyp
Copy link

indietyp commented Feb 5, 2024

That isn't completely true, if you specify the desurgared form in a trait definition you can specify additional bounds (like Send + 'static) in implementations you can still use async fn.

You can see this in action here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=ec570d1da8499bb14e80bac0827e233b

Comment out the Rc implementation to see a compilation error. (with RPITIT normal future rules apply, here just using Rc to illustrate the point more clearly)

@Eugeny
Copy link
Owner

Eugeny commented Feb 6, 2024

Thanks for the tip! I gave it a try and unfortunately I'm running into rust-lang/rust#110338

If you want to give it a try: #244

@Qix-
Copy link
Author

Qix- commented Feb 6, 2024

As a complete alternative (a large, hairy alternative), it might be a good evolution to implement proper Future types, given that russh is a library. Then all of these issues go away. But that's a massive change, for sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants