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

use GAT to elide StreamTrait lifetime #1149

Closed
wants to merge 1 commit into from
Closed

use GAT to elide StreamTrait lifetime #1149

wants to merge 1 commit into from

Conversation

nappa85
Copy link
Contributor

@nappa85 nappa85 commented Oct 24, 2022

Hello guys, I'm back!
Thursday Rust 1.65.0 will be stabilized, and with it GATs will land on stable.
I've seen only this morning you've release a release candidate for 0.10.0, but this work is portable to the new version for sure.
Long story short, using GATs we can remove the lifetime from StreamTrait and move it internally.
To be honest, that lifetime has given me more than one headache, I think this is a big usability improvement.
It's still a work in progress, I'm still testing, I've already seen some corner cases and I'll work on it tomorrow, but I wanted to share with you this improvement so you can start thinking about landing an improvement that needs next stable compiler to work
Best regards

edit: I wrote TAIT instead of GAT, I need some rest...

@nappa85 nappa85 changed the title WIP: use TAIT to elide StreamTrait lifetime WIP: use GAT to elide StreamTrait lifetime Oct 24, 2022
@tyt2y3
Copy link
Member

tyt2y3 commented Oct 25, 2022

Welcome back!

Yes I think GAT can be used to replace the use of async_trait and other things.
As I have said, there are three hard things in Rust: lifetime, orphan rule and generic associated types.
And this library is (going to) tackle all three.

Awesome! Let me know the progress or problems.

@nappa85
Copy link
Contributor Author

nappa85 commented Oct 25, 2022

Welcome back!

Yes I think GAT can be used to replace the use of async_trait and other things. As I have said, there are three hard things in Rust: lifetime, orphan rule and generic associated types. And this library is (going to) tackle all three.

Awesome! Let me know the progress or problems.

How do you think to use GAT to replace async_trait? Because, as far as I know, while we don't have TAIT we can't have type Foo = impl Future; in traits, but maybe I'm missing something, I'm all ears.
Are you planning to write concrete futures for all trait return types? Sounds like a big work to me...

Anyway, until now, all the problems I've found wasn't sea-orm's related, I'll keep you updated

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 26, 2022

How do you think to use GAT to replace async_trait

I think I learnt this technique here: https://www.sobyte.net/post/2022-04/rust-gat-async-trait/

Are you planning to write concrete futures for all trait return types?

No concrete plan for now

@nappa85
Copy link
Contributor Author

nappa85 commented Oct 26, 2022

How do you think to use GAT to replace async_trait

I think I learnt this technique here: https://www.sobyte.net/post/2022-04/rust-gat-async-trait/

Yes, this uses both GATs and TAITs
GAT allow you to do

trait Foo {
  type Bar<'a>: AnotherTrait<'a>;
}

but when you impl the trait you still need to use a concrete type, the only difference is that it can have a lifetime that isn't global to the trait himself
with TAIT you can use impl AnotherTrait<'a> for your type Bar<'a>

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 26, 2022

Umm... I think the original concern is that async_trait basically "Box the Future", and GAT removes that.
It's still pretty unergonomic, but we can probably hide the details with yet another macro.
It might make sense to use this technique on TransactionTrait, but this is probably another task.

@nappa85
Copy link
Contributor Author

nappa85 commented Oct 26, 2022

Umm... I think the original concern is that async_trait basically "Box the Future", and GAT removes that. It's still pretty unergonomic, but we can probably hide the details with yet another macro. It might make sense to use this technique on TransactionTrait, but this is probably another task.

Yes, what I'm trying to say is that to replace async_trait you need both GAT and TAIT, like in your article

#![feature(generic_associated_types)] //GAT
#![feature(type_alias_impl_trait)] //TAIT

or else you need to create a concrete future type for every return, like tokio does internally

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 26, 2022

Ah sad I forgot TAIT is still unstable. Some day, then.

@nappa85 nappa85 changed the title WIP: use GAT to elide StreamTrait lifetime use GAT to elide StreamTrait lifetime Oct 27, 2022
@nappa85
Copy link
Contributor Author

nappa85 commented Oct 27, 2022

My tests are all green, I'll open another PR for master branch

@nappa85
Copy link
Contributor Author

nappa85 commented Oct 27, 2022

Thinking about the breaking change, maybe you'll prefer merging this only on 0.10.0 to not introduce breaking changes on 0.9.x

@tyt2y3
Copy link
Member

tyt2y3 commented Nov 6, 2022

I think we can close this?

@tyt2y3
Copy link
Member

tyt2y3 commented Nov 10, 2022

Since this requires newer Rust version, I don't think we can backport this

@tyt2y3 tyt2y3 closed this Nov 10, 2022
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

Successfully merging this pull request may close these issues.

2 participants