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

fix: executor arg of RawSql methods can have looser bound #3656

Closed
wants to merge 2 commits into from

Conversation

tisonkun
Copy link
Contributor

@tisonkun tisonkun commented Dec 26, 2024

The original code has an unnecessary lifetime bound 'q: 'e.

We have a code snippet like:

    let res = sqlx::raw_sql(&insert_sql)
        .execute(&mut **txn)
        .await
        .change_context_lazy(make_error)?;

failed with: implementation of `std::marker::Send` is not general enough in several caller layers outside.

while change to:

    let res = txn
        .execute(&insert_sql)
        .await
        .change_context_lazy(make_error)?;

works.

A brief is that the txn's lifetime bound to the outer struct, so as long as the returned future is bound to that lifetime, rustc knows it's send for all outer struct's lifetime.

However, with .execute(&mut **txn), the new lifetime c is resolved to e where 'q: 'e and then the return future lifetime is 'e. Now rustc doesn't know the relation of the lifetimes, and failed to compile.

@tisonkun
Copy link
Contributor Author

tisonkun commented Dec 26, 2024

I just find that the implementation of sqlx::query(...).execute(&mut **txn) is correct and we can simply align the code as in the second commit.

@joeydewaal
Copy link
Contributor

Looks like there is already a fix for this (#3613)

@tisonkun
Copy link
Contributor Author

tisonkun commented Dec 31, 2024

@joeydewaal Looks correct. I'd leave to @abonander to manage both PRs. Closing this is fine.

@abonander
Copy link
Collaborator

I believe I tried this approach and it didn't fix the minimal reproduction I came up with. Since this doesn't have a reproduction to show that it fixes the problem, and my PR makes the API match the Query{As, Scalar} types, I'm going to close in favor of #3613.

@abonander abonander closed this Jan 4, 2025
@tisonkun tisonkun deleted the fix-lifetime-for-raw-sql branch January 4, 2025 05:08
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.

3 participants