[NLL] Avoid borrowed value must be valid for lifetime '_#2r..." in errors #48592

spastorino · 2018-02-27T19:10:21Z

Closes #48428

If NLL is enabled, do not invoke note_and_explain_region
Modify -Zdump-nll-cause to not print the overwhelming debug output here. This way we should I believe at least get nice-ish output for our original example.
Extend explain_why_borrow_contains_point to also work for "universal lifetimes" like the 'a in the example at the end of this comment.
Figure out how to enable causal information all the time (but that is make causal tracking lazy #46590).

rust-highfive · 2018-02-27T19:10:33Z

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

spastorino · 2018-02-27T19:11:02Z

r? @nikomatsakis

nikomatsakis · 2018-02-28T14:20:57Z

src/test/ui/nll/borrowed-local-error.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![feature(nll)]


For now, can you add // compile-flags: -Znll-dump-cause?

nikomatsakis · 2018-02-28T14:25:04Z

src/test/ui/nll/borrowed-local-error.rs

+        &v
+        //~^ ERROR `v` does not live long enough [E0597]
+    });
+    println!("{:?}", x);


Current output for this test:

lunch-box. rustc +nightly borrowed-local-error.rs -Znll-dump-cause error[E0597]: `v` does not live long enough --> borrowed-local-error.rs:20:9 | 18 | let x = gimme({ | _____________- 19 | | let v = (22,); 20 | | &v | | ^^ borrowed value does not live long enough 21 | | //~^ ERROR `v` does not live long enough [E0597] 22 | | }); | |_____-- borrow later used here | | | borrowed value only lives until here | = note: borrowed value must be valid for lifetime '_#2r... error: aborting due to previous error

So this output is pretty terrible, but mostly because of the multi-line highlight and the extra text about must be valid for lifetime '_#2r.

However, the "borrowed value only lives until here" and "borrow later used here" are also not especially helpful.

@estebank -- how do you normally deal with those btw? I remember it's a bit of a case-by-case basis; I think in this case the span comes from the MIR. Are there some helper functions we can invoke to try and reduce the span to a single line?

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

I think that to make this happen, we need to:

(a) identify that the later use is a call and 'shorten' the highlight to just the function name and/or method name -- @estebank may be able to help with that point

(b) modify the text, since it is due to a function call, to say "during the call"

Also:

borrowed value only lives until here

that is ungreat. I think it should be more of action, like "v is dropped here". So let me take another crack at this. I also think the "sequence" here is perhaps a bit confusing, so i'm going to try adding numbers:

error[E0597]: `v` does not live long enough --> borrowed-local-error.rs:20:9 | 18 | let x = gimme({ | ----- (3) But the reference is later used here, during the call 19 | let v = (22,); 20 | &v | ^^ (1) First, `v` is borrowed here, creating a reference 21 | }); | - (2) Then, `v` is dropped here, at the end of the block

I think all of this is feasible: for example, when the message is about something going out of scope, and that thing is a local variable (v) rather than a temporary, we can say "at the end of the block", since that is always when local variables go out of scope.

Given that the order things happen is meaningful to providing a good explanation, we should either use span notes:

error[E0597]: `v` does not live long enough --> borrowed-local-error.rs:20:9 | 20 | &v | ^^ borrowed here, creating a reference 21 | }); | - `v` is dropped here, at the end of the block | note: ...but the reference must live long enough to be used here, during the call | 18 | let x = gimme({ | -----

or change the text to be "orderless":

error[E0597]: `v` does not live long enough --> borrowed-local-error.rs:20:9 | 18 | let x = gimme({ | ----- `v` must live long enough to be used in this call, but it doesn't 19 | let v = (22,); 20 | &v | ^^ borrowed here, creating a reference 21 | }); | - `v` is dropped here, at the end of the block |

As for wether there's a method to extract the function call, there isn't one now and in all errors where we point at the function call itself we've kept the span around since the AST. I'd have to dig in the current code to see if we do have it available already, but I don't think it should be that hard to find/add.

nikomatsakis · 2018-02-28T14:25:33Z

src/test/ui/nll/borrowed-temporary-error.rs

+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or


current output for this test:

lunch-box. rustc +nightly borrowed-temporary-error.rs -Znll-dump-cause error[E0597]: borrowed value does not live long enough --> borrowed-temporary-error.rs:20:10 | 20 | &(v,) | ^^^^ temporary value does not live long enough 21 | //~^ ERROR borrowed value does not live long enough [E0597] 22 | }); | - temporary value only lives until here 23 | println!("{:?}", x); | - borrow later used here | = note: borrowed value must be valid for lifetime '_#2r... error: aborting due to previous error

this seems pretty good to me, modulo the borrowed value must be valid for lifetime '_#2r..., which I know you already removed.

I think we should only be showing the borrowed value must be valid for lifetime '_#2r for anon lifetimes that cover an entire block and named lifetimes. That shouldn't be that hard to do.

I don't understand. I don't think there is any circumstances in which we should show the text '_#2r. I guess you are saying that a sentence of this kind only makes sense for either an entire block or named lifetimes?

In that case, I both agree and disagree: it probably makes sense for named lifetimes, but I don't think it makes sense for blocks.

Mistyped and you interpreted me correctly. Meant to write borrowed value must be valid for {} for the current cases (entire block or named).

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.

nikomatsakis · 2018-02-28T14:27:35Z

src/test/ui/nll/borrowed-universal-error.rs

+fn foo<'a>(x: &'a (u32,)) -> &'a u32 {
+    let v = 22;
+    gimme(&(v,))
+}


Current output for this test:

lunch-box. rustc +nightly borrowed-universal-error.rs -Znll-dump-cause error[E0597]: borrowed value does not live long enough --> borrowed-universal-error.rs:18:12 | 18 | gimme(&(v,)) | ^^^^ temporary value does not live long enough 19 | } | - temporary value only lives until here | = note: borrowed value must be valid for lifetime '_#3r... = note: because of an outlives relation created at `bb0[11]` because of an outlives relation created at `bb0[12]` because of an outlives relation created at `bb0[12]` because of an outlives relation created at `bb0[12]` because of an outlives relation created at `bb2[0]` because of an outlives relation created at `bb2[1]` because of an outlives relation created at `bb2[1]` because of an outlives relation created at `bb0[0]` because `'_#1r` is universally quantified

this is clearly missing some mention of 'a. Compare to the AST checker:

lunch-box. rustc +nightly borrowed-universal-error.rs -Znll-dump-cause error[E0597]: borrowed value does not live long enough --> borrowed-universal-error.rs:17:12 | 17 | gimme(&(v,)) | ^^^^ temporary value does not live long enough 18 | } | - temporary value only lives until here | note: borrowed value must be valid for the lifetime 'a as defined on the function body at 15:1... --> borrowed-universal-error.rs:15:1 | 15 | fn foo<'a>(x: &'a (u32,)) -> &'a u32 { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

If we invoke note_and_explain_region from explain_why_borrow_contains_point in the case of a LiveUniversal root cause, then we would get output similar to the AST-checker.

nikomatsakis · 2018-02-28T14:43:42Z

src/test/ui/nll/borrowed-universal-error-2.rs

+    let v = 22;
+    let r = gimme(&(v,));
+    r
+}


Pushed one more example. Current output:

> rustc +nightly borrowed-universal-error-2.rs -Znll-dump-cause error[E0597]: borrowed value does not live long enough --> borrowed-universal-error-2.rs:18:20 | 18 | let r = gimme(&(v,)); | ^^^^ - temporary value only lives until here | | | temporary value does not live long enough 19 | r | - borrow later used here | = note: borrowed value must be valid for lifetime '_#3r... error: aborting due to previous error

Whoops, this example is not that interesting I guess. Well, it is interesting, but not the reason I initially thought.

I thought it would be an example where we are returning something that has to have the lifetime 'a, like the borrow-universal-error.rs, but in fact we get the error earlier on, because of the temporary we've created. Let me revise it a bit.

nikomatsakis · 2018-02-28T14:47:29Z

src/test/ui/nll/borrowed-universal-error-2.rs

 fn foo<'a>(x: &'a (u32,)) -> &'a u32 {
    let v = 22;
-    let r = gimme(&(v,));
-    r
+    &v


Output now:

lunch-box. rustc +nightly borrowed-universal-error-2.rs -Znll-dump-cause error[E0597]: `v` does not live long enough --> borrowed-universal-error-2.rs:16:5 | 16 | &v | ^^ borrowed value does not live long enough 17 | } | - borrowed value only lives until here | = note: borrowed value must be valid for lifetime '_#3r... = note: because of an outlives relation created at `bb0[4]` because of an outlives relation created at `bb0[5]` because of an outlives relation created at `bb0[5]` because of an outlives relation created at `bb0[0]` because `'_#1r` is universally quantified error: aborting due to previous error

Still ungreat.

nikomatsakis

This looks good @spastorino

nikomatsakis · 2018-03-01T19:32:46Z

src/librustc/infer/error_reporting/mod.rs

-                    Some(hir_map::NodeItem(it)) => item_scope_tag(&it),
-                    Some(hir_map::NodeTraitItem(it)) => trait_item_scope_tag(&it),
-                    Some(hir_map::NodeImplItem(it)) => impl_item_scope_tag(&it),
+                    Some(hir_map::NodeItem(it)) => TyCtxt::item_scope_tag(&it),


Nit: in cases like these, you can write Self::, which I personally prefer.

nikomatsakis · 2018-03-02T00:10:25Z

@bors r+

bors · 2018-03-02T00:10:26Z

📌 Commit 99c42dc has been approved by nikomatsakis

bors · 2018-03-04T18:16:50Z

⌛ Testing commit 99c42dc with merge 259e4a6...

[NLL] Avoid borrowed value must be valid for lifetime '_#2r..." in errors Closes #48428 - [x] If NLL is enabled, [do not invoke `note_and_explain_region`](#48428 (comment)) - [x] Modify `-Zdump-nll-cause` to not print [the overwhelming debug output here](https://github.com/rust-lang/rust/blob/master/src/librustc_mir/borrow_check/nll/region_infer/mod.rs#L1288-L1299). This way we should I believe at least get nice-ish output for [our original example](#48428 (comment)). - [x] Extend `explain_why_borrow_contains_point` to also work for "universal lifetimes" like the `'a` in [the example at the end of this comment](#48428 (comment)). - [ ] Figure out how to enable causal information all the time (but that is #46590).

bors · 2018-03-04T21:13:38Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 259e4a6 to master...

rust-highfive assigned pnkfelix Feb 27, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 27, 2018

rust-highfive assigned nikomatsakis and unassigned pnkfelix Feb 27, 2018

spastorino force-pushed the borrowed_value_error branch 2 times, most recently from ac09a40 to dd187ca Compare February 27, 2018 20:22

nikomatsakis reviewed Feb 28, 2018

View reviewed changes

pietroalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 28, 2018

nikomatsakis reviewed Feb 28, 2018

View reviewed changes

spastorino added 2 commits February 28, 2018 12:31

Do not report _#nr lifetimes names in errors

a17a2e3

Avoid printing overwhelming info on -Znll-dump-cause

7580826

spastorino force-pushed the borrowed_value_error branch from fde93fe to 2b95e34 Compare February 28, 2018 18:52

spastorino changed the title ~~[WIP] [NLL] Avoid borrowed value must be valid for lifetime '_#2r..." in errors~~ [NLL] Avoid borrowed value must be valid for lifetime '_#2r..." in errors Mar 1, 2018

spastorino force-pushed the borrowed_value_error branch 4 times, most recently from 3100389 to 71dc3ff Compare March 1, 2018 18:55

spastorino mentioned this pull request Mar 1, 2018

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

Closed

nikomatsakis approved these changes Mar 1, 2018

View reviewed changes

spastorino added 3 commits March 1, 2018 16:40

Refactor note_and_explain_region to call note_and_explain_free_region

834e392

Make explain borrow work for Universal lifetimes

ff7dca5

rustfmt over error_reporting, explain_borrow and region_infer mods

99c42dc

spastorino force-pushed the borrowed_value_error branch from 71dc3ff to 99c42dc Compare March 1, 2018 19:43

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 2, 2018

Manishearth mentioned this pull request Mar 2, 2018

Rollup of 9 pull requests #48652

Merged

nikomatsakis mentioned this pull request Mar 3, 2018

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

Open

bors merged commit 99c42dc into rust-lang:master Mar 4, 2018

bors mentioned this pull request Mar 4, 2018

[NLL] Make causal tracking lazy #48682

Merged

pnkfelix mentioned this pull request Mar 21, 2018

Compiler panic: unexpected unsized tail: TyProjection(ProjectionTy { substs: Slice([chrono::Utc]), ... #48336

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NLL] Avoid borrowed value must be valid for lifetime '_#2r..." in errors #48592

[NLL] Avoid borrowed value must be valid for lifetime '_#2r..." in errors #48592

spastorino commented Feb 27, 2018 •

edited

Loading

rust-highfive commented Feb 27, 2018

spastorino commented Feb 27, 2018

nikomatsakis Feb 28, 2018

nikomatsakis Feb 28, 2018 •

edited

Loading

nikomatsakis Feb 28, 2018

nikomatsakis Feb 28, 2018

nikomatsakis Feb 28, 2018

nikomatsakis Feb 28, 2018

nikomatsakis Feb 28, 2018

estebank Feb 28, 2018 •

edited

Loading

nikomatsakis Feb 28, 2018

nikomatsakis Feb 28, 2018

estebank Feb 28, 2018

nikomatsakis Mar 1, 2018

estebank Mar 1, 2018

nikomatsakis Feb 28, 2018

nikomatsakis Feb 28, 2018

nikomatsakis Feb 28, 2018 •

edited

Loading

nikomatsakis Feb 28, 2018

nikomatsakis Feb 28, 2018

nikomatsakis Feb 28, 2018

nikomatsakis left a comment

nikomatsakis Mar 1, 2018

spastorino Mar 1, 2018

nikomatsakis commented Mar 2, 2018

bors commented Mar 2, 2018

bors commented Mar 4, 2018

bors commented Mar 4, 2018

[NLL] Avoid borrowed value must be valid for lifetime '_#2r..." in errors #48592

[NLL] Avoid borrowed value must be valid for lifetime '_#2r..." in errors #48592

Conversation

spastorino commented Feb 27, 2018 • edited Loading

rust-highfive commented Feb 27, 2018

spastorino commented Feb 27, 2018

Choose a reason for hiding this comment

nikomatsakis Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Mar 2, 2018

bors commented Mar 2, 2018

bors commented Mar 4, 2018

bors commented Mar 4, 2018

spastorino commented Feb 27, 2018 •

edited

Loading

nikomatsakis Feb 28, 2018 •

edited

Loading

estebank Feb 28, 2018 •

edited

Loading

nikomatsakis Feb 28, 2018 •

edited

Loading