Custom error when moving arg outside of its closure #47144

estebank · 2018-01-02T23:29:00Z

When given the following code:

fn give_any<F: for<'r> FnOnce(&'r ())>(f: F) {
    f(&());
}

fn main() {
    let mut x = None;
    give_any(|y| x = Some(y));
}

provide a custom error:

error: borrowed data cannot be moved outside of its closure
 --> file.rs:7:27
  |
6 |     let mut x = None;
  |         ----- borrowed data cannot be moved into here...
7 |     give_any(|y| x = Some(y));
  |              ---          ^ cannot be moved outside of its closure
  |              |
  |              ...because it cannot outlive this closure

instead of the generic lifetime error:

error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
 --> file.rs:7:27
  |
7 |     give_any(|y| x = Some(y));
  |                           ^
  |
note: first, the lifetime cannot outlive the anonymous lifetime #2 defined on the body at 7:14...
 --> file.rs:7:14
  |
7 |     give_any(|y| x = Some(y));
  |              ^^^^^^^^^^^^^^^
note: ...so that expression is assignable (expected &(), found &())
 --> file.rs:7:27
  |
7 |     give_any(|y| x = Some(y));
  |                           ^
note: but, the lifetime must be valid for the block suffix following statement 0 at 6:5...
 --> file.rs:6:5
  |
6 | /     let mut x = None;
7 | |     give_any(|y| x = Some(y));
8 | | }
  | |_^
note: ...so that variable is valid at time of its declaration
 --> file.rs:6:9
  |
6 |     let mut x = None;
  |         ^^^^^

Fix #45983.

rust-highfive · 2018-01-02T23:29:03Z

r? @petrochenkov

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

durka · 2018-01-03T05:12:54Z

I find the phrasing confusing, especially "closure you can't escape" -- I am not in a closure! How about something like this:

error: borrowed data cannot be moved outside of its closure
 --> file.rs:7:27
  |
6 |     let mut x = None;
  |         ----- move destination (outside closure)
7 |     give_any(|y| x = Some(y));
  |              ---          ^ borrowed data which cannot escape this closure
  |              |
  |              closure

I'm not even sure about the word "escape". Maybe too jargon-ish?

estebank · 2018-01-03T05:54:22Z

I am not in a closure!

After the second glass of wine, I certainly am.

I'm not even sure about the word "escape". Maybe too jargon-ish?

It probably is, but wanted to push the PR out for exactly this feedback :)

I like your suggested output, but I feel we could still further improve it. I'm just not sure how.

durka · 2018-01-03T07:49:58Z

We could do an ellipsis monstrosity:

error: borrowed data cannot be moved outside of its closure
 --> file.rs:7:27
  |
6 |     let mut x = None;
  |         ----- borrowed data cannot be moved to here...
7 |     give_any(|y| x = Some(y));
  |              ---          ^ ...from here...
  |              |
  |              ...because it cannot outlive this closure

estebank · 2018-01-03T17:01:16Z

I didn't want to do that because the closure could be

|y| {
    Some(y)
}

durka · 2018-01-03T17:10:13Z

Yeah, it would require some kind of special case depending on ordering which isn't great.

estebank · 2018-01-03T21:16:58Z

What about

error: borrowed data cannot be moved outside of its closure
 --> file.rs:7:27
  |
6 |     let mut x = None;
  |         ----- borrowed data cannot be moved to here...
7 |     give_any(|y| x = Some(y));
  |              ---          ^ cannot be moved outside of its closure
  |              |
  |              ...because it cannot outlive this closure

It doesn't read as nicely, but it has the advantage of always making kind of sense, regardless of the code's formatting, and any tools that might not show the secondary spans still get a meaningful label.

petrochenkov · 2018-01-03T21:25:51Z

r? @nikomatsakis

estebank · 2018-01-04T06:13:53Z

Edit: DONE
Final output for this case:

error: borrowed data cannot be moved outside of its closure
  --> ../../src/test/compile-fail/issue-7573.rs:31:27
   |
27 |     let mut lines_to_use: Vec<&CrateId> = Vec::new();
   |                               - cannot infer an appropriate lifetime
28 |
29 |     let push_id = |installed_id: &CrateId| {
   |                   ------------------------ borrowed data cannot outlive this closure
30 |
31 |         lines_to_use.push(installed_id);
   |                           ^^^^^^^^^^^^ cannot be moved outside of its closure

There's an incorrectly handled case:

% rustc src/test/compile-fail/issue-7573.rs
error: borrowed data cannot be moved outside of its closure
  --> src/test/compile-fail/issue-7573.rs:27:31
   |
27 |     let mut lines_to_use: Vec<&CrateId> = Vec::new();
   |                               ^ cannot be moved outside of its closure
28 |
29 |     let push_id = |installed_id: &CrateId| {
   |         -------   ------------------------ ...because it cannot outlive this closure
   |         |
   |         borrowed data cannot be moved into here...

error: aborting due to previous error

It should probably look this way:

error: borrowed data cannot be moved outside of its closure
  --> src/test/compile-fail/issue-7573.rs:27:31
   |
27 |     let mut lines_to_use: Vec<&CrateId> = Vec::new();
   |         ---------------- borrowed data cannot be moved into here...
28 |
29 |     let push_id = |installed_id: &CrateId| {
   |                   ------------------------ ...because it cannot outlive this closure
30 |        lines_to_use.push(installed_id);
   |                          ^^^^^^^^^^^^ cannot be moved outside of its closure

error: aborting due to previous error

It currently looks like this:

error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
  --> src/main.rs:16:31
   |
16 |     let mut lines_to_use: Vec<&CrateId> = Vec::new(); //~ ERROR E0495
   |                               ^
   |
note: first, the lifetime cannot outlive the anonymous lifetime #1 defined on the body at 17:19...
  --> src/main.rs:17:19
   |
17 |       let push_id = |installed_id: &CrateId| {
   |  ___________________^
18 | |         lines_to_use.push(installed_id);
19 | |     };
   | |_____^
note: ...so that reference does not outlive borrowed content
  --> src/main.rs:18:27
   |
18 |         lines_to_use.push(installed_id);
   |                           ^^^^^^^^^^^^
note: but, the lifetime must be valid for the block suffix following statement 1 at 17:5...
  --> src/main.rs:17:5
   |
17 | /     let push_id = |installed_id: &CrateId| {
18 | |         lines_to_use.push(installed_id);
19 | |     };
20 | |     list_database(push_id);
...  |
25 | |
26 | | }
   | |_^
note: ...so that variable is valid at time of its declaration
  --> src/main.rs:17:9
   |
17 |     let push_id = |installed_id: &CrateId| {
   |         ^^^^^^^

error: aborting due to previous error

nikomatsakis · 2018-01-09T12:45:25Z

@estebank

There's an incorrectly handled case:

Hmm, I sort of suspected this would be the case. It might be worth investing some energy to see if we can do better, though I am also tempted to redirect that effort into the MIR borrowck errors, which go down a slightly different path.

nikomatsakis · 2018-01-09T12:46:22Z

Oh, I guess you're saying you fixed it. Let me read what's there first. =)

nikomatsakis · 2018-01-09T12:49:49Z

src/librustc/infer/error_reporting/mod.rs

+
+        // #45983: when trying to assign the contents of an argument to a binding outside of a
+        // closure, provide a specific message pointing this out.
+        if let (&SubregionOrigin::BindingTypeIsNotValidAtDecl(ref external_span),


This could should really go into the nice_region_error module, I think, following the precedent we've established. There would be a file like nice_region_error/outlives_closure.rs and try_report_nice_region_error would invoke it.

Moved to its own module.

nikomatsakis · 2018-01-09T13:07:11Z

src/librustc/infer/error_reporting/mod.rs

+                        err.span_label(sup_sp, "cannot be moved outside of its closure");
+                        if sup_sp == origin_sp {
+                            err.span_label(*external_span,
+                                           "borrowed data cannot be moved into here...");


I still find the word move potentially misleading. I would prefer "stored".

nikomatsakis · 2018-01-09T13:07:57Z

src/test/ui/borrowck/issue-45983.stderr

+   |              |
+   |              ...because it cannot outlive this closure
+
+error: aborting due to previous error


can you add a ui test for the other case you mentioned? The one where you push into a vector that exists outside the closure?

Moved existing test that shows the behavior to ui: https://github.com/rust-lang/rust/pull/47144/files#diff-e5107c6d2a404ecd34f782e9e9e8b20f

shepmaster · 2018-01-13T00:56:49Z

Ping from triage, @estebank! Do you think you'll be able to address some of the feedback here?

When given the following code: ```rust fn give_any<F: for<'r> FnOnce(&'r ())>(f: F) { f(&()); } fn main() { let mut x = None; give_any(|y| x = Some(y)); } ``` provide a custom error: ``` error: borrowed data cannot be moved outside of its closure --> file.rs:7:27 | 6 | let mut x = None; | ----- binding declared outside of closure 7 | give_any(|y| x = Some(y)); | --- ^ cannot be assigned to binding outside of its closure | | | closure you can't escape ``` instead of the generic lifetime error: ``` error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements --> file.rs:7:27 | 7 | give_any(|y| x = Some(y)); | ^ | note: first, the lifetime cannot outlive the anonymous lifetime rust-lang#2 defined on the body at 7:14... --> file.rs:7:14 | 7 | give_any(|y| x = Some(y)); | ^^^^^^^^^^^^^^^ note: ...so that expression is assignable (expected &(), found &()) --> file.rs:7:27 | 7 | give_any(|y| x = Some(y)); | ^ note: but, the lifetime must be valid for the block suffix following statement 0 at 6:5... --> file.rs:6:5 | 6 | / let mut x = None; 7 | | give_any(|y| x = Some(y)); 8 | | } | |_^ note: ...so that variable is valid at time of its declaration --> file.rs:6:9 | 6 | let mut x = None; | ^^^^^ ```

Trigger new diagnostic in `compile-fail/regions-escape-bound-fn.rs` test, and not only in `compile-fail/regions-escape-bound-fn-2.rs`.

- Move specialized borrow checker diagnostic for bindings escaping its closure to its own module. - Move affected tests to `ui`.

estebank · 2018-01-15T19:20:08Z

Addressed the review comments.

estebank · 2018-01-16T20:07:11Z

src/test/ui/borrowck/regions-escape-bound-fn.stderr

+   |              |       |    |
+   |              |       |    cannot be stored outside of its closure
+   |              |       cannot infer an appropriate lifetime
+   |              borrowed data cannot outlive this closure


@nikomatsakis I'm not entirely happy with the output on this case :-/

This triggers the case I originally neglected and was trying to fix...

Should we keep this PR on hold until I have time to look at how to differentiate between these two cases for a more appropriate output? In my mind, this case should completely ignore the lifetime issue for Some(_) and point at the destination binding, like in the output of the original case for this PR:

error: borrowed data cannot be stored outside of its closure --> $DIR/regions-escape-bound-fn.rs:18:27 | 17 | let mut x: Option<&isize> = None; | - borrowed data cannot be stored into here... 18 | with_int(|y| x = Some(y)); | --- ^ cannot be stored outside of its closure | | | ...because it cannot outlive this closure

It should be possible to do so as the current diagnostic does point at x's declaration:

error[E0495]: cannot infer an appropriate lifetime for automatic coercion due to conflicting requirements --> src/main.rs:8:22 | 8 | with_int(|y| x = Some(y)); | ^^^^^^^ | note: first, the lifetime cannot outlive the anonymous lifetime #2 defined on the body at 8:14... --> src/main.rs:8:14 | 8 | with_int(|y| x = Some(y)); | ^^^^^^^^^^^^^^^ note: ...so that reference does not outlive borrowed content --> src/main.rs:8:27 | 8 | with_int(|y| x = Some(y)); | ^ note: but, the lifetime must be valid for the block suffix following statement 0 at 7:5... --> src/main.rs:7:5 | 7 | / let mut x: Option<&isize> = None; 8 | | with_int(|y| x = Some(y)); 9 | | //~^ ERROR borrowed data cannot be stored outside of its closure 10| | } | |_^ note: ...so that variable is valid at time of its declaration --> src/main.rs:7:9 | 7 | let mut x: Option<&isize> = None; | ^^^^^

Also, should I create a new error code for each of these cases, one shared between all of them or reuse the existing one?

I think this output is way better than the original output. The main change I would do is not to print the "cannot infer an appropriate lifetime" label, which seems to add zero value. Saying that the data cannot escape the closure and indicating the closure is pretty awesome.

nikomatsakis · 2018-01-17T16:18:38Z

src/librustc/infer/error_reporting/nice_region_error/outlives_closure.rs

+                            } else {
+                                err.span_label(*closure_span,
+                                               "borrowed data cannot outlive this closure");
+                                err.span_label(origin_sp,


I think we should just drop this label. I don't really think it conveys useful information to the end-user. The origin_sp is not especially meaningful -- it's basically a point where the region is relevant, but not necessarily a particularly good choice of point.

I feel uneasy about removing the span completely because it's the original error span, and want to point to it in some way. Modified so that it occurs in fewer cases, but will keep it for the #7573 case. I'm planning to revisit that and the type mismatch cases at a later PR.

nikomatsakis · 2018-01-19T21:51:07Z

Ok, this feels like progress in any case. r=me once travis is happy.

estebank · 2018-01-19T23:56:04Z

@bors r=nikomatsakis

bors · 2018-01-19T23:56:05Z

📌 Commit 6f9ecaa has been approved by nikomatsakis

bors · 2018-01-22T01:27:08Z

⌛ Testing commit 6f9ecaa with merge eabb46a2351fab39802b4ad72bce00c5d83063f5...

bors · 2018-01-22T05:27:08Z

💥 Test timed out

estebank · 2018-01-22T05:30:33Z

@bors retry

bors · 2018-01-22T05:30:43Z

⌛ Testing commit 6f9ecaa with merge bc072ed...

Custom error when moving arg outside of its closure When given the following code: ```rust fn give_any<F: for<'r> FnOnce(&'r ())>(f: F) { f(&()); } fn main() { let mut x = None; give_any(|y| x = Some(y)); } ``` provide a custom error: ``` error: borrowed data cannot be moved outside of its closure --> file.rs:7:27 | 6 | let mut x = None; | ----- borrowed data cannot be moved into here... 7 | give_any(|y| x = Some(y)); | --- ^ cannot be moved outside of its closure | | | ...because it cannot outlive this closure ``` instead of the generic lifetime error: ``` error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements --> file.rs:7:27 | 7 | give_any(|y| x = Some(y)); | ^ | note: first, the lifetime cannot outlive the anonymous lifetime #2 defined on the body at 7:14... --> file.rs:7:14 | 7 | give_any(|y| x = Some(y)); | ^^^^^^^^^^^^^^^ note: ...so that expression is assignable (expected &(), found &()) --> file.rs:7:27 | 7 | give_any(|y| x = Some(y)); | ^ note: but, the lifetime must be valid for the block suffix following statement 0 at 6:5... --> file.rs:6:5 | 6 | / let mut x = None; 7 | | give_any(|y| x = Some(y)); 8 | | } | |_^ note: ...so that variable is valid at time of its declaration --> file.rs:6:9 | 6 | let mut x = None; | ^^^^^ ``` Fix #45983.

bors · 2018-01-22T08:10:37Z

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

rust-highfive assigned petrochenkov Jan 2, 2018

estebank added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 2, 2018

estebank force-pushed the moved-closure-arg branch 2 times, most recently from 14e8eed to 1295c68 Compare January 3, 2018 01:45

rust-highfive assigned nikomatsakis and unassigned petrochenkov Jan 3, 2018

estebank force-pushed the moved-closure-arg branch 3 times, most recently from 38d587a to 0c5f4f6 Compare January 4, 2018 05:50

estebank force-pushed the moved-closure-arg branch from 62998a0 to 5831359 Compare January 4, 2018 20:29

nikomatsakis requested changes Jan 9, 2018

View reviewed changes

shepmaster 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 Jan 13, 2018

estebank added 4 commits January 15, 2018 07:04

Reword diagnostic

c31c60c

Generalize cases where specific move error ocurrs

2c5f2df

Trigger new diagnostic in `compile-fail/regions-escape-bound-fn.rs` test, and not only in `compile-fail/regions-escape-bound-fn-2.rs`.

Handle case of moving into vec with uninferred lifetime

1a1afd7

estebank force-pushed the moved-closure-arg branch from 5831359 to 86a4c74 Compare January 15, 2018 18:56

Move diagnostic logic to its own module

1820da5

- Move specialized borrow checker diagnostic for bindings escaping its closure to its own module. - Move affected tests to `ui`.

estebank force-pushed the moved-closure-arg branch from 86a4c74 to 1820da5 Compare January 15, 2018 19:12

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

estebank commented Jan 16, 2018

View reviewed changes

nikomatsakis reviewed Jan 17, 2018

View reviewed changes

Tweak wording and spans of closure lifetime errors

6f9ecaa

estebank force-pushed the moved-closure-arg branch from 71e0e36 to 6f9ecaa Compare January 19, 2018 21:28

nikomatsakis approved these changes Jan 19, 2018

View reviewed changes

nikomatsakis 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 Jan 19, 2018

estebank 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 Jan 20, 2018

bors merged commit 6f9ecaa into rust-lang:master Jan 22, 2018

estebank deleted the moved-closure-arg branch November 9, 2023 05:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom error when moving arg outside of its closure #47144

Custom error when moving arg outside of its closure #47144

estebank commented Jan 2, 2018 •

edited

Loading

rust-highfive commented Jan 2, 2018

durka commented Jan 3, 2018

estebank commented Jan 3, 2018

durka commented Jan 3, 2018

estebank commented Jan 3, 2018

durka commented Jan 3, 2018

estebank commented Jan 3, 2018 •

edited

Loading

petrochenkov commented Jan 3, 2018

estebank commented Jan 4, 2018 •

edited

Loading

nikomatsakis commented Jan 9, 2018 •

edited

Loading

nikomatsakis commented Jan 9, 2018

nikomatsakis Jan 9, 2018

estebank Jan 15, 2018

nikomatsakis Jan 9, 2018

estebank Jan 15, 2018

nikomatsakis Jan 9, 2018

estebank Jan 15, 2018

shepmaster commented Jan 13, 2018

estebank commented Jan 15, 2018

estebank Jan 16, 2018

nikomatsakis Jan 17, 2018

nikomatsakis Jan 17, 2018

estebank Jan 19, 2018

nikomatsakis commented Jan 19, 2018

estebank commented Jan 19, 2018

bors commented Jan 19, 2018

bors commented Jan 22, 2018

bors commented Jan 22, 2018

estebank commented Jan 22, 2018

bors commented Jan 22, 2018

bors commented Jan 22, 2018

Custom error when moving arg outside of its closure #47144

Custom error when moving arg outside of its closure #47144

Conversation

estebank commented Jan 2, 2018 • edited Loading

rust-highfive commented Jan 2, 2018

durka commented Jan 3, 2018

estebank commented Jan 3, 2018

durka commented Jan 3, 2018

estebank commented Jan 3, 2018

durka commented Jan 3, 2018

estebank commented Jan 3, 2018 • edited Loading

petrochenkov commented Jan 3, 2018

estebank commented Jan 4, 2018 • edited Loading

nikomatsakis commented Jan 9, 2018 • edited Loading

nikomatsakis commented Jan 9, 2018

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

shepmaster commented Jan 13, 2018

estebank commented Jan 15, 2018

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 commented Jan 19, 2018

estebank commented Jan 19, 2018

bors commented Jan 19, 2018

bors commented Jan 22, 2018

bors commented Jan 22, 2018

estebank commented Jan 22, 2018

bors commented Jan 22, 2018

bors commented Jan 22, 2018

estebank commented Jan 2, 2018 •

edited

Loading

estebank commented Jan 3, 2018 •

edited

Loading

estebank commented Jan 4, 2018 •

edited

Loading

nikomatsakis commented Jan 9, 2018 •

edited

Loading