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 #1161

Merged
merged 4 commits into from Nov 6, 2022
Merged

use GAT to elide StreamTrait lifetime #1161

merged 4 commits into from Nov 6, 2022

Conversation

nappa85
Copy link
Contributor

@nappa85 nappa85 commented Oct 27, 2022

Clone of PR #1149, but on master branch

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 27, 2022

generic associated types are unstable

umm @billy1624 aren't we using the latest stable Rust?

am I missing something here - is some bit of it still in unstable realm?

@nappa85
Copy link
Contributor Author

nappa85 commented Oct 27, 2022

generic associated types are unstable

umm @billy1624 aren't we using the latest stable Rust?

am I missing something here - is some bit of it still in unstable realm?

GATs should stabilize today with rust 1.65.0

@nappa85
Copy link
Contributor Author

nappa85 commented Oct 27, 2022 via email

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 30, 2022

Yeah I have been using it without the feature tag for a while (in nightly). Let's merge this after 1.65

@tyt2y3
Copy link
Member

tyt2y3 commented Nov 5, 2022

Tried again! Cool, I think the clippy error is unrelated to this PR and we should fix it on master @billy1624
Other than that, I think this can be merged.

Actually, I have no clue about Rust's tentative release dates.

@nappa85
Copy link
Contributor Author

nappa85 commented Nov 5, 2022

Tried again! Cool, I think the clippy error is unrelated to this PR and we should fix it on master @billy1624 Other than that, I think this can be merged.

Yes, it's not from my PR

Actually, I have no clue about Rust's release schedule.

A new stable every 6 weeks https://www.whatrustisit.com/

@tyt2y3 tyt2y3 merged commit 9d25ee9 into SeaQL:master Nov 6, 2022
@tyt2y3
Copy link
Member

tyt2y3 commented Nov 6, 2022

One question, I think this does not break the API despite type changes, so justifiable as a minor release?

Edit: actually we use rust-version = "1.60" currently. So strictly speaking this should be a major release where we update it to 1.65 as well.

@nappa85
Copy link
Contributor Author

nappa85 commented Nov 6, 2022 via email

@tyt2y3
Copy link
Member

tyt2y3 commented Nov 6, 2022

I released 0.10.2 anyway

@tyt2y3
Copy link
Member

tyt2y3 commented Nov 7, 2022

Mentioning rust-lang/rust#96709

@tyt2y3
Copy link
Member

tyt2y3 commented Nov 7, 2022

@nappa85 Would you mind sharing a snippet that bugs you before but is solved by this change?

I am drafting the release blog post for this feature and I would like to illustrate it with an example.

I imagine you've had a generic 'stream processor' function that accepts a generic Connection, in which the 'a lifetime propagates to other places?

@nappa85
Copy link
Contributor Author

nappa85 commented Nov 7, 2022

It wasn't a real bug, more a lifetime problem, let me explain...

I always ask for a generic connection C: ConnectionTrait and, when needed, also StreamTrait and/or TransactionTrait. This way my functions can be called on a normal connection or on a transaction, or anything else.

With old code, when you need StreamTrait, you end up needing a lifetime, e.g.

async fn do_something<'a, C>(conn: &'a C) -> Result<...>
where C: ConnectionTrait + StreamTrait<'a> {...}

Most of the times it worked like a charm, but sometimes it won't compile telling you that the connection is still borrowed when it really isn't, but the problem is generated by connection reference and stream sharing the same lifetime.

It was on situations like that:

async fn complex_situation<C>(conn: &C) -> Result<...>
where C: ConnectionTrait + TransactionTrait
{
  let txn = conn.begin().await?;
  do_something(&txn).await?;
  txn.commit().await?;// here we have the error, because it tells txn is still borrowed
}

Unfortunately I don't have a MVP demonstrating the problem.

With GAT, the same function loses the lifetime, e.g.

async fn do_something<C>(conn: &C) -> Result<...>
where C: ConnectionTrait + StreamTrait {...}

This is cleaner, and the problem is solved because now the stream has his own lifetime disconnected from connection reference.

@ikrivosheev
Copy link
Member

@tyt2y3 I think we need to upgrade minimum rust version after merge GAT...

@tyt2y3
Copy link
Member

tyt2y3 commented Nov 8, 2022

@tyt2y3 I think we need to upgrade minimum rust version after merge GAT...

I think I did bumped all rust_version , did I miss something?

@nappa85 Thank you for the elaboration!

@ikrivosheev
Copy link
Member

@tyt2y3 I think we need to upgrade minimum rust version after merge GAT...

I think I did bumped all rust_version , did I miss something?

@nappa85 Thank you for the elaboration!

Yes, you did! Sorry, I don't find it in PR.

zhaofengli added a commit to zhaofengli/sea-orm that referenced this pull request Jan 16, 2023
…QL#1161)"

To reduce MSRV so Attic builds with nixpkgs 22.11 that has
Rust 1.64. This workaround will be removed when 23.05 becomes
stable.

This reverts commit 9d25ee9.
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.

None yet

3 participants