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] borrowed-universal-test.rs output is not great #48645

Open
spastorino opened this issue Mar 1, 2018 · 5 comments
Open

[NLL] borrowed-universal-test.rs output is not great #48645

spastorino opened this issue Mar 1, 2018 · 5 comments
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. P-low Low priority 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

The output of this test ...

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

#![feature(nll)]
#![allow(warnings)]

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

fn foo<'a>(x: &'a (u32,)) -> &'a u32 {
    let v = 22;
    gimme(&(v,))
    //~^ ERROR borrowed value does not live long enough [E0597]
}

fn main() {}

is not great

[santiago@archlinux rust1 (borrowed_value_error)]$ rustc +stage1 src/test/ui/nll/borrowed-universal-error.rs -Znll-dump-cause
error[E0597]: borrowed value does not live long enough
  --> src/test/ui/nll/borrowed-universal-error.rs:22:12
   |
22 |     gimme(&(v,))
   |            ^^^^ temporary value does not live long enough
23 |     //~^ ERROR borrowed value does not live long enough [E0597]
24 | }
   | - temporary value only lives until here
   |
note: borrowed value must be valid for the lifetime 'a as defined on the function body at 20:1...
  --> src/test/ui/nll/borrowed-universal-error.rs:20:1
   |
20 | fn foo<'a>(x: &'a (u32,)) -> &'a u32 {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

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

cc @nikomatsakis

@pnkfelix pnkfelix added A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll labels Mar 1, 2018
@nikomatsakis
Copy link
Contributor

So, I wanted this opened because:

borrowed value must be valid for the lifetime 'a as defined on the function body at 20:1...

doesn't seem great to me. Not terrible, but not great. I'd prefer to see us tracking whether the value is returned or escapes in some other way, and pointing at the spot where the escape occurs with a nice message. Something like this maybe?

error[E0597]: borrowed value does not live long enough
  --> src/test/ui/nll/borrowed-universal-error.rs:22:12
   |
22 |     gimme(&(v,))
   |      ----- ^^^^ temporary is created here, and then borrowed
   |      |
   |      reference is returned here
24 | }
   | - temporary will be freed here, when this block exits

@nikomatsakis nikomatsakis added the A-diagnostics Area: Messages for errors, warnings, and lints label Mar 1, 2018
@nikomatsakis
Copy link
Contributor

This was spawned from this conversation. I'm going to copy over @estebank's last comment in particular:

For named we currently only point at the signature of the scope that defines it, while for anonymous we point at the entire block on its own span note. We could change this to point at the end where it must live, but that doesn't seem to me entirely different to what we do now, and would lose some context... Of course, this only applies to LL.

I think the best bet is still to continue handling specific cases with more targeted suggestions/explanations, as gaurikholkar was doing. These can get increasingly niche, but those are the cases where rustc holding your hand is most impactful and can be a great teachable opportunity.

@XAMPPRocky XAMPPRocky added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label May 21, 2018
@matthewjasper matthewjasper added NLL-diagnostics Working towards the "diagnostic parity" goal and removed NLL-deferred labels Dec 1, 2018
@matthewjasper
Copy link
Contributor

matthewjasper commented Dec 29, 2018

Current output:

error[E0515]: cannot return value referencing temporary value
  --> <source>:12:5
   |
12 |     gimme(&(v,))
   |     ^^^^^^^----^
   |     |      |
   |     |      temporary value created here
   |     returns a value referencing data owned by the current function
error: aborting due to previous error
For more information about this error, try `rustc --explain E0515`.
Compiler returned: 1

nominating for discussion on whether this should be kept open

@nikomatsakis nikomatsakis added P-low Low priority and removed I-nominated labels Jan 9, 2019
@nikomatsakis
Copy link
Contributor

Discussed in the NLL meeting:

  • New output has very nice content
    • It could make be made more clear by emphasizing that the value will be freed
  • However, the formatting is kind of confusing, particularly due to the overlapping spans

We decided to keep it open for now but mark it P-low.

@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 20, 2020
@Aaron1011
Copy link
Member

This now produces the same error message both with and without #![feature(nll)]

@Aaron1011 Aaron1011 removed the NLL-diagnostics Working towards the "diagnostic parity" goal label Oct 2, 2021
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. P-low Low priority 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

7 participants