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

Improve borrow error when assigning borrowed local value to "argument by mutable reference" #99430

Open
estebank opened this issue Jul 18, 2022 · 12 comments
Labels
A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@estebank
Copy link
Contributor

estebank commented Jul 18, 2022

Given

fn f(mut p: &mut i32) {

    let mut number = 111;
    p = &mut number;

    *p = 2;
    println!("{}", *p);
}

we currently emit

error[E0597]: `number` does not live long enough
 --> f44.rs:4:9
  |
1 | fn f(mut p: &mut i32) {
  |             - let's call the lifetime of this reference `'1`
...
4 |     p = &mut number;
  |     ----^^^^^^^^^^^
  |     |   |
  |     |   borrowed value does not live long enough
  |     assignment requires that `number` is borrowed for `'1`
...
8 | }
  | - `number` dropped here while still borrowed

but it could be more informative:

error[E0597]: `number` does not live long enough
 --> f44.rs:4:9
  |
1 | fn f(mut p: &mut i32) {
  |             - let's call the lifetime of this reference `'1`
...
3 |     let mut number = 111;
  |             -----   --- this value is assigned to `number` and lives as long as `f`
  |             |
  |             this binding only lives for the duration of function `f` and gets dropped at the function
4 |     p = &mut number;
  |     ----^^^^^^^^^^^
  |     |   |
  |     |   borrowed value does not live long enough
  |     this assignment to `p` will outlive `f` because it is a `&mut i32` owned by the caller's scope
...
8 | }
  | - `number` dropped here while still borrowed
@estebank estebank added A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions A-borrow-checker Area: The borrow checker T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. D-papercut Diagnostics: An error or lint that needs small tweaks. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. labels Jul 18, 2022
@BoxyUwU
Copy link
Member

BoxyUwU commented Jul 18, 2022

I dislike the suggested error message- p is not owned by the callers scope. The lifetime on the p reference will outlive the borrow of number because it is chosen by the caller who can only name lifetimes that outlive borrows of locals inside of f. If this error is to be improved (imo) it should suggest adding a let mut p = p at the start of the function to allow shortening the caller chosen lifetime to that of the borrow of number. (I suppose to be clear- i think the missing information here is that the lifetime on p is chosen by the caller who can only name lifetimes that outlive borrows of the locals, this explains both why the let mut p = p would work and why the original code doesnt work)

@ryanavella
Copy link

ryanavella commented Jul 18, 2022

This is an odd one. I used to think that fn f(mut t: T) { ... } was just sugar for fn f(t: T) {let mut t = t; ... }, but clearly that isn't the case. The error can be suppressed by rewriting it in my hypothetical desugared form, like this:

fn f(p: &mut i32) {
    let mut p = p; // <-- suppresses error!

    let mut number = 111;
    p = &mut number;

    *p = 2;
    println!("{}", *p);
}

I'm not familiar with rustc internals so I don't know what information is known by this pass, but is it possible to distinguish this specific case from the more general case and suggest introducing a let-binding?

i.e. something like this:

error[E0597]: `number` does not live long enough

....

help: use `let mut` if you meant to introduce a local binding
  |
4 |     let mut p = &mut number;
  |     ~~~~~~~ 

... or this

error[E0597]: `number` does not live long enough

....

help: use `let mut` if you meant to introduce a local binding
  |
2 |     let mut p = p;
  |     ~~~~~~~~~~~~~~
3 |     let mut number = 111;

@jam1garner
Copy link
Contributor

this assignment to `p` will outlive `f` because it is a `&mut i32` owned by the caller's scope

I heavily dislike this phrasing, and would go as far as to say it's incorrect.

Now to help demonstrate, here's an example that I think this *would* be applicable for:

fn f(p: &mut &mut i32) {

    let mut number = 111;
    *p = &mut number;

    **p = 2;
    println!("{}", **p);
}

(Note the change from mut p: &mut i32 to p: &mut &mut i32, as it's the only change aside from updating deref counts to match)

Going back to the phrasing:

a &mut i32 owned by the caller's scope

I would argue this implies the reference (not the underlying data) is owned by the caller. Which, isn't really the case, as it's actually pass-by-move-ing the reference, so the caller doesn't actually own the reference itself anymore.

Brief intermission: the distinction I'm making is both pedantic and dependent on (a) my mental model and (b) my interpretation of words like "own". So really, whether or not I'm correct comes down to semantics and wholly respect disagreement, but everything will be phrased as if it is fact for the sake of not bogging this down with uncertain phrasing.

So to indicate what I feel is actually happening, I think it's important to desugar what's going on, especially as I think Rust could very well allow the code you posted (more on this in a minute). This means that the explanation being in terms of "this is wrong, it doesn't live long enough" is potentially suboptimal, as the reason the code is rejected is more arbitrary than that.

So desugared:

fn f<'a>(mut p: &'a mut i32) {

    let mut number = 111;
    p = &mut number;

    *p = 2;
    println!("{}", *p);
}

Now the issue here is not "the reference outlives number", the issue is that the caller can choose arbitrary lifetimes that &mut number cannot necessarily conform to. From a functionality standpoint, the implementation just sees 'a as "must live as long as this function, maybe longer".

So maybe the explanation should include the desugaring? I am not sure how to explain it otherwise.

@jam1garner
Copy link
Contributor

jam1garner commented Jul 18, 2022

I think @ryanavella's suggestion is not a bad one (honestly not sure I like that's it's necessary though. I'm curious if lifetimes should be coercible to the minimum when not observable by the caller?), although I would add some context explaining why this fixes it:

help: use `let mut` if you meant to introduce a local binding to shorten the lifetime to this function's scope

(phrasing subject to overhaul, my only nit was "this suggestion is too magical and unintuitive without the 'why'")

@cuviper
Copy link
Member

cuviper commented Jul 19, 2022

Note that you can compile code like the OP wrote when there's a deeper pattern involved -- see #86989.

fn f((_i, mut p): (usize, &mut i32)) {

    let mut number = 111;
    p = &mut number;

    *p = 2;
    println!("{}", *p);
}

But still, if the intention was to change the caller's reference to the local, that will never work.

@ryanavella
Copy link

@cuviper That is definitely interesting, even a 1-tuple is enough of a pattern to suppress the error:

fn f((mut p,): (&mut i32,)) {

    let mut number = 111;
    p = &mut number;

    *p = 2;
    println!("{}", *p);
}

It isn't clear to me which should be the "correct behavior" here. I can see an argument that the error is spurious because there is nothing inherently unsound here. Plus the error message is confusing to newcomers.

But is it possible that some users could be relying on this error to enforce a type-level invariant? If so, I wouldn't know how to construct an example. All of the examples I can think of can be easily bypassed by shadow-binding, as @BoxyUwU first suggested.

@jam1garner
Copy link
Contributor

jam1garner commented Jul 19, 2022

wow! great link @cuviper, very interesting behavior. hopefully the non-nested bindings are loosened to act like the nested ones?

@mdaffin
Copy link

mdaffin commented Jul 19, 2022

Plus the error message is confusing to newcomers.

It is very confusing to new comers. But so is the whole situation. It seems quite a lot of people seem to assume the problem is that p itself somehow outlives the function and so must anything that gets assigned to it otherwise it is unsound when this is not the case. Nothing is technically unsound here but IMO it smells badly of a bug in the code. Why would you ever want to reassign p when it points to a reference?

That is the only thing you can do by declaring it mut. It makes me think that the caller either mean to rebind with let or they actually wanted p: &mut &mut u32 to change where the original reference points to.

@QuineDot
Copy link

QuineDot commented Sep 4, 2022

That is definitely interesting, even a 1-tuple is enough of a pattern to suppress the error

@ryanavella Subpatterns are enough.

fn f(mut p @ _: &mut i32) {
    let mut number = 111;
    p = &mut number;
    *p = 2;
    println!("{}", *p);
}

@estebank
Copy link
Contributor Author

I managed to get the exact spans I originally wanted, and I tested a few different cases:

This is the case that (I'm guessing) I was really interested in, but it is different from the one in the ticket. I believe that the proposed wording was accurate for this case.

error[E0597]: `number` does not live long enough
 --> f11.rs:5:10
  |
2 | fn f(p: &mut &S) {
  |              - let's call the lifetime of this reference `'1`
3 |
4 |     let mut number = S;
  |         ----------   - this value is assigned to `number` and lives as long as `f`
  |         |
  |         this binding only lives for the duration of `f` and gets dropped at the function
5 |     *p = &number;
  |     -----^^^^^^^
  |     |    |
  |     |    borrowed value does not live long enough
  |     assignment requires that `number` is borrowed for `'1`
6 | }
  | - `number` dropped here while still borrowed

This is the case in the report that people have pointed out should not have been rejected in the first place. I believe that this is a separate issue, but it also gets the new span labels :-/

error[E0597]: `number` does not live long enough
  --> f11.rs:14:9
   |
12 | fn g(mut p: &mut S) {
   |             - let's call the lifetime of this reference `'1`
13 |     let mut number = S;
   |         ----------   - this value is assigned to `number` and lives as long as `g`
   |         |
   |         this binding only lives for the duration of `g` and gets dropped at the function
14 |     p = &mut number;
   |     ----^^^^^^^^^^^
   |     |   |
   |     |   borrowed value does not live long enough
   |     assignment requires that `number` is borrowed for `'1`
15 | }
   | - `number` dropped here while still borrowed

This is a variation where the argument is not mutable and assigned to. No changes, other than the suggestion pointing you maybe in the wrong direction, but the three errors being emitted might have enough context for people to make the right choice. I'm not sure.

error[E0384]: cannot assign to immutable argument `p`
  --> f11.rs:10:5
   |
7  | fn a(p: &mut &S) {
   |      - help: consider making this binding mutable: `mut p`
...
10 |     p = &mut &number;
   |     ^^^^^^^^^^^^^^^^ cannot assign to immutable argument

error[E0716]: temporary value dropped while borrowed
  --> f11.rs:10:14
   |
7  | fn a(p: &mut &S) {
   |         - let's call the lifetime of this reference `'1`
...
10 |     p = &mut &number;
   |     ---------^^^^^^^- temporary value is freed at the end of this statement
   |     |        |
   |     |        creates a temporary value which is freed while still in use
   |     assignment requires that borrow lasts for `'1`

error[E0597]: `number` does not live long enough
  --> f11.rs:10:14
   |
7  | fn a(p: &mut &S) {
   |              - let's call the lifetime of this reference `'2`
8  |
9  |     let mut number = S;
   |         ----------   - this value is assigned to `number` and lives as long as `a`
   |         |
   |         this binding only lives for the duration of `a` and gets dropped at the function
10 |     p = &mut &number;
   |     ---------^^^^^^^
   |     |        |
   |     |        borrowed value does not live long enough
   |     assignment requires that `number` is borrowed for `'2`
11 | }
   | - `number` dropped here while still borrowed

estebank added a commit to estebank/rust that referenced this issue Jan 15, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 25, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 25, 2023
@estebank
Copy link
Contributor Author

Current output:

error[E0597]: `number` does not live long enough
 --> src/lib.rs:4:9
  |
1 | fn f(mut p: &mut i32) {
  |             - let's call the lifetime of this reference `'1`
2 |
3 |     let mut number = 111;
  |         ---------- binding `number` declared here
4 |     p = &mut number;
  |     ----^^^^^^^^^^^
  |     |   |
  |     |   borrowed value does not live long enough
  |     assignment requires that `number` is borrowed for `'1`
...
8 | }
  | - `number` dropped here while still borrowed

@Rudxain
Copy link
Contributor

Rudxain commented Apr 22, 2024

Related:

fn main() {
    let x = 0;
    let mut y = &x;
    let mut z = &y;
    y = &1;
    // rm `let` to get E0716
    let z = &&x; // why does shadowing magically fix it?
    println!("{z}");
}

Playground: 1.79.0-nightly (2024-04-21 fb89862)

The docs don't explain anything about that:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants