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

Automatic &mut reborrowing doesn't happen when arguments are for impl Trait #85161

Open
alexcrichton opened this issue May 10, 2021 · 7 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-inference Area: Type inference D-confusing Diagnostics: Confusing error or lint that should be reworked. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

Given code like this:

fn foo(a: &mut i32) {
    auto_reborrow(a); // works
    auto_reborrow(a); // works

    no_reborrow(a); // works
    no_reborrow(a); // ERROR
}

fn auto_reborrow(_: &mut i32) {}

fn no_reborrow(_: impl std::fmt::Debug) {}

it currently fails to compile with:

error[E0382]: use of moved value: `a`
 --> src/lib.rs:6:17
  |
1 | fn foo(a: &mut i32) {
  |        - move occurs because `a` has type `&mut i32`, which does not implement the `Copy` trait
...
5 |     no_reborrow(a); // works
  |                 - value moved here
6 |     no_reborrow(a); // ERROR
  |                 ^ value used here after move

error: aborting due to previous error

While not a super serious issue ergonomically it'd be great if rustc would apply the same automatic reborrowing logic here to &mut T arguments regardless of whether the target function takes a &mut T or it's satisfying an impl Trait bound of some form.

@crlf0710
Copy link
Member

Seems the cause is that generics suppresses coercions.

@fitzgen
Copy link
Member

fitzgen commented Jul 21, 2021

For what it is worth, this is not specific to impl Trait, and the equivalent thing with explicit generic type parameters also does not get automatic reborrows: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=61a5e437792c83938187171b72375574

@nrc
Copy link
Member

nrc commented Jul 21, 2021

I think the issue here is not specific to reborrowing or to impl Trait but is because when the compiler converts types it always need a 'target' type which the compiler is trying to convert to. The compiler does not take trait impls into account. So if the compiler knows we want type U it will try and convert the type (whether U is a concrete type or generic), but if the code requires an expression to have a trait bound T, it will not search for all types which implement T and try and convert to each, even for the 'obvious' case of converting &mut V to &V

@jyn514
Copy link
Member

jyn514 commented Apr 9, 2023

I think this is not a bug and changes here should involve T-lang. I'm going to close this for now.

@jyn514 jyn514 closed this as not planned Won't fix, can't repro, duplicate, stale Apr 9, 2023
@jyn514 jyn514 added T-lang Relevant to the language team, which will review and decide on the PR/issue. A-inference Area: Type inference labels Apr 9, 2023
@bnjbvr
Copy link
Contributor

bnjbvr commented Apr 9, 2023

Can I try to convince you this is still a serious usability bug? Especially in the sense that, if you look at the initial example, we have two forms of function calls that look exactly the same, but behave differently because of (if I understand correctly) an implementation detail. It doesn't behave in a uniform way, which is a UX pain point (nmatsakis has explained this much better than I can!). The error it leads to is confusing, in my opinion, since based on my own experience and observations other developers around me, everyone expected the re-borrow to happen automatically, so the user kept on looking for another programming mistake.

@jyn514
Copy link
Member

jyn514 commented Apr 9, 2023

Sure thing, I don't mind keeping this open if the goal is to improve the error message.

@jyn514 jyn514 reopened this Apr 9, 2023
@jyn514 jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints D-confusing Diagnostics: Confusing error or lint that should be reworked. labels Apr 9, 2023
@Anti-Alias
Copy link

Just got bit by this while using the sqlx crate using the postgres backend, specifically when using transactions. (probably a backend-independent problem)

let mut transaction = connection_pool.begin().await?;
let conn: &mut PgConnection = transaction.acquire().await?;

sqlx::query("...").execute(conn).await?;    // Totally fine.
sqlx::query("...").execute(conn).await?;    // Totally NOT fine since conn has moved.

I am able to get the code to work by manually reborrowing, but I feel like this shouldn't be necessary.
An inexperienced developer might give up and just start creating multiple connection objects from the transaction to get around what seems like a limitation. I almost did this.

Note that you don't have to reborrow when transactions aren't involved, as you can just pass in a &PgPool, which uses interior mutability. No &mut reborrowing there.

sqlx::query("...").execute(&connection_pool).await?;    // Totally fine.
sqlx::query("...").execute(&connection_pool).await?;    // Totally fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-inference Area: Type inference D-confusing Diagnostics: Confusing error or lint that should be reworked. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants