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

[NLL] Tidy up error messages when borrowing locals #48643

Closed
spastorino opened this issue Mar 1, 2018 · 2 comments
Closed

[NLL] Tidy up error messages when borrowing locals #48643

spastorino opened this issue Mar 1, 2018 · 2 comments
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-NLL Area: Non-lexical lifetimes (NLL) C-enhancement Category: An issue proposing an enhancement or a PR with one. NLL-diagnostics Working towards the "diagnostic parity" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@spastorino
Copy link
Member

spastorino commented Mar 1, 2018

Given this test which is included in #48592 and when merged will be located here https://github.com/rust-lang/rust/tree/master/src/test/ui/nll/borrowed-local-error.rs

// compile-flags: -Znll-dump-cause

#![feature(nll)]

fn gimme(x: &(u32,)) -> &u32 {
    &x.0
}

fn main() {
    let x = gimme({
        let v = (22,);
        &v
        //~^ ERROR `v` does not live long enough [E0597]
    });
    println!("{:?}", x);
}

With #48592 merged, the output looks like ...

[santiago@archlinux rust1 (borrowed_value_error)]$ RUST_BACKTRACE=1 rustc +stage1 src/test/ui/nll/borrowed-local-error.rs -Znll-dump-cause
error[E0597]: `v` does not live long enough
  --> src/test/ui/nll/borrowed-local-error.rs:22:9
   |
20 |       let x = gimme({
   |  _____________-
21 | |         let v = (22,);
22 | |         &v
   | |         ^^ borrowed value does not live long enough
23 | |         //~^ ERROR `v` does not live long enough [E0597]
24 | |     });
   | |_____-- borrow later used here
   |       |
   |       borrowed value only lives until here

error: aborting due to previous error

If you want more information on this error, try using "rustc --explain E0597"

@nikomatsakis said that the output should look like ...

This is what I think it should look like:

error[E0597]: `v` does not live long enough
  --> borrowed-local-error.rs:20:9
     |
  18 |       let x = gimme({
     |               ----- borrow later used here, during the call
  19 |           let v = (22,);
  20 |           &v
     |           ^^ borrowed value does not live long enough
  21 |       });
     |       - borrowed value only lives until here

Previous discussion #48592 (comment)

@nikomatsakis nikomatsakis added this to the NLL: diagnostic parity milestone Mar 1, 2018
@nikomatsakis nikomatsakis added A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll A-diagnostics Area: Messages for errors, warnings, and lints labels Mar 1, 2018
@jkordish jkordish added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Mar 7, 2018
@nikomatsakis nikomatsakis added the NLL-diagnostics Working towards the "diagnostic parity" goal label Mar 14, 2018
@matthewjasper matthewjasper self-assigned this Jul 23, 2018
@nikomatsakis nikomatsakis removed this from the NLL: diagnostic parity milestone Jul 24, 2018
@matthewjasper matthewjasper removed their assignment Jul 31, 2018
@pnkfelix
Copy link
Member

While I agree that @nikomatsakis 's suggested revision would be an improvement, the output currently produced by NLL seems competitive with the AST-borrowck diagnostic.

That is, AST-borrowck also fails to identify the role of the function call, as shown here:

error[E0597]: `v` does not live long enough
  --> src/main.rs:9:10
   |
9  |         &v
   |          ^ borrowed value does not live long enough
10 |         //~^ ERROR `v` does not live long enough [E0597]
11 |     });
   |     - `v` dropped here while still borrowed
12 |     println!("{:?}", x);
13 | }
   | - borrowed value needs to live until here

error: aborting due to previous error

(I assume that this is why @nikomatsakis removed this from the "NLL: diagnostic parity" milestone a month ago.)

@pnkfelix
Copy link
Member

Tagging as NLL-deferred to reflect the fact that this is much lower priority than other NLL work items.

@matthewjasper matthewjasper self-assigned this Sep 26, 2018
bors added a commit that referenced this issue Oct 4, 2018
[NLL] Improve "borrow later used here" messages

* In the case of two conflicting borrows, the later used message says which borrow it's referring to
* If the later use is a function call (from the users point of view) say that the later use is for the call. Point just to the function.

r? @pnkfelix
Closes #48643
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-NLL Area: Non-lexical lifetimes (NLL) C-enhancement Category: An issue proposing an enhancement or a PR with one. NLL-diagnostics Working towards the "diagnostic parity" goal 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

5 participants