Explain why a closure is `FnOnce` in closure errors. #42196

tommyip · 2017-05-24T16:42:12Z

Fixes: #42065

rust-highfive · 2017-05-24T16:42:17Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

tommyip · 2017-05-24T16:43:29Z

src/librustc_borrowck/borrowck/mod.rs

@@ -597,6 +597,9 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
                        if let Ok(ty::ClosureKind::FnOnce) =
                           ty::queries::closure_kind::try_get(self.tcx, DUMMY_SP, id) {
                            err.help("closure was moved because it only implements `FnOnce`");
+                            if let Some(&(_kind, Some(span))) = self.tables.closure_kinds.get( ) {


How do you find the node_id here?

This would be the result of self.tcx.hir.as_local_node_id(id), I think -- we're basically looking things up based on the id of the closure. However, what we might want to do is just to include this information in the result of closure_kind::try_get() above, or else make a separate query.

(Also, @eddyb, why is this try_get() anyhow? That seems...dubious to me?)

aidanhs · 2017-05-25T00:04:09Z

Thanks for the PR @tommyip! We'll check in occasionally to make sure @eddyb or someone else reviews/answers your questions soon.

eddyb · 2017-05-25T07:21:19Z

to get the upvar instead of the original variable's span?

There is no "upvar" in the source - it's solely the fact that that a closure referred to a variable at least once.
None of those uses (many of which could be moves) is more representative than any other.
@nikomatsakis might like the idea of using EUV to find the moves and point to one of them.
But it seems unnecessary to me to do anything of the sort.

nikomatsakis · 2017-05-25T15:28:01Z

src/librustc/ty/context.rs

-    pub closure_kinds: NodeMap<ty::ClosureKind>,
+    /// Records the kind of each closure and the span of the variable that
+    /// cause the closure to be this kind.
+    pub closure_kinds: NodeMap<(ty::ClosureKind, Option<Span>)>,


Nit: I think what we want is to include the span of a variable, but perhaps more than that we would prefer to include the name as well, for later use in the error message (including just the node-id of the reference could suffice, but I'm a bit wary of including a node-id in this value, since it will make life harder later as we handle incremental etc).

nikomatsakis · 2017-05-25T15:31:23Z

src/librustc_typeck/check/upvar.rs

@@ -289,7 +290,8 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {

                        // to move out of an upvar, this must be a FnOnce closure
                        self.adjust_closure_kind(upvar_id.closure_expr_id,
-                                                 ty::ClosureKind::FnOnce);
+                                                 ty::ClosureKind::FnOnce,
+                                                 tcx.hir.span(upvar_id.var_id));


I think I would just use guarantor.span here.

nikomatsakis · 2017-05-25T15:31:41Z

src/librustc_typeck/check/upvar.rs

@@ -303,7 +305,8 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {
                        // to be a FnOnce closure to permit moves out
                        // of the environment.
                        self.adjust_closure_kind(upvar_id.closure_expr_id,
-                                                 ty::ClosureKind::FnOnce);
+                                                 ty::ClosureKind::FnOnce,
+                                                 tcx.hir.span(upvar_id.var_id));


Also here, guarantor.span seems good.

nikomatsakis · 2017-05-25T15:33:46Z

src/librustc_typeck/check/upvar.rs

-                self.adjust_closure_kind(upvar_id.closure_expr_id, ty::ClosureKind::FnMut);
+                self.adjust_closure_kind(upvar_id.closure_expr_id,
+                                         ty::ClosureKind::FnMut,
+                                         tcx.hir.span(upvar_id.var_id));


here, in addition to passing in the &Note, we can pass in the cmt that the note originated from, and use that span. That should be the span of the variable reference.

nikomatsakis · 2017-05-25T15:35:51Z

@eddyb

But it seems unnecessary to me to do anything of the sort.

In what sense unnecessary? The idea is to be able to give better error messages -- essentially, when we tell a user that a closure can only be FnOnce or whatever, we should also be able to identify for them the variable that made us infer that the closure must be FnOnce. That seems very useful to me; I know I at least have been annoyed in the past at having to dig through and find the upvar that is being moved. Sometimes it's not entirely obvious.

Do you feel like the error message improvement is unnecessary, or do you have another idea of how to accomplish it? If the latter, I'm all ears. If the former, I'm curious why you think so.

eddyb · 2017-05-25T19:02:28Z

I mean that giving name or pointing to the definition is enough (of an improvement) compared to finding the actual uses. OTOH, EUV is already used to collect those uses during typeck now that I think of it so it should be doable.

tommyip · 2017-05-30T10:09:21Z

@nikomatsakis This should now work

nikomatsakis · 2017-05-30T16:41:18Z

src/test/ui/fn_once-moved.stderr

@@ -1,12 +1,15 @@
 error[E0382]: use of moved value: `debug_dump_dict`
  --> $DIR/fn_once-moved.rs:21:5
   |
+16 |         for (key, value) in dict {
+   |                             ---- dict moved here


This is great! My only concern is that, when you read this message, the dict moved here seems a bit out of the blue. I am torn between trying to give more context in this label vs trying to move the label down below.

Also, as a minor style nit, we should put dict in backticks:

`dict` moved here

Basically, it seems highly likely to me that people will get confused between the closure being moved twice and variable dict being moved twice.

What do you mean by 'move the label down below'?

I don't know =) I was imagining output like this. We do still support this style, though we've been moving away from it in many cases. This may be a case where it's worthwhile:

error[E0382]: use of moved value: `debug_dump_dict` --> $DIR/fn_once-moved.rs:21:5 | 20 | debug_dump_dict(); | --------------- value moved here 21 | debug_dump_dict(); | ^^^^^^^^^^^^^^^ value used here after move | note: closure cannot be invoked more than once because it moves the variable `dict` out of its environment | 16 | for (key, value) in dict { | ---- dict moved here

I think that if you use span_note it will produce something sort of like this? You may have to experiment a bit. (I'm also unclear on whether "help" or "note" is more appropriate here.)

Ok I will try that. Also yeah, a note makes much more sense than a help here.

I changed it to span_note and it looks like:

error[E0382]: use of moved value: `debug_dump_dict` --> src/test/ui/fn_once-moved.rs:21:5 | 20 | debug_dump_dict(); | --------------- value moved here 21 | debug_dump_dict(); | ^^^^^^^^^^^^^^^ value used here after move | = note: closure cannot be invoked more than once because it moves the variable `dict` out of its environment note: `dict` moved here --> src/test/ui/fn_once-moved.rs:16:29 | 16 | for (key, value) in dict { | ^^^^ error: aborting due to previous error

The flow of the error message is indeed better, should I commit this?

@tommyip yeah, seems like progress; one thing I would suggest though -- can we merge the two "notes"? e.g., by using "closure cannot be invoked more than once because it moves the variable dict out of its environment" as the text for span_note?

tommyip · 2017-05-31T11:29:25Z

Updated

nikomatsakis · 2017-05-31T15:50:30Z

@bors r+

Great!

bors · 2017-05-31T15:50:31Z

📌 Commit c2f7e94 has been approved by nikomatsakis

Mark-Simulacrum · 2017-05-31T16:50:07Z

@bors rollup

@nikomatsakis

…matsakis Explain why a closure is `FnOnce` in closure errors. Issue: rust-lang#42065 @nikomatsakis Am I going the right direction with this? ~~I am stuck in a few bits:~~ ~~1. How to trace the code to get the upvar instead of the original variable's span?~~ ~~2. How to find the node id of the upvar where the move occured?~~

Rollup of 7 pull requests - Successful merges: #42126, #42196, #42252, #42277, #42315, #42329, #42330 - Failed merges:

Use tracked data introduced in rust-lang#42196 to provide a better closure error message by showing why a closure implements `FnOnce`. ``` error[E0525]: expected a closure that implements the `Fn` trait, but this closure only implements `FnOnce` --> $DIR/issue_26046.rs:4:19 | 4 | let closure = move || { | ___________________^ 5 | | vec 6 | | }; | |_____^ | note: closure is `FnOnce` because it moves the variable `vec` out of its environment --> $DIR/issue_26046.rs:5:9 | 5 | vec | ^^^ error: aborting due to previous error(s) ``` Fixes rust-lang#26046

@nikomatsakis

Better closure error message Use tracked data introduced in #42196 to provide a better closure error message by showing why a closure implements `FnOnce`. ``` error[E0525]: expected a closure that implements the `Fn` trait, but this closure only implements `FnOnce` --> $DIR/issue_26046.rs:4:19 | 4 | let closure = move || { | ___________________^ 5 | | vec 6 | | }; | |_____^ | note: closure is `FnOnce` because it moves the variable `vec` out of its environment --> $DIR/issue_26046.rs:5:9 | 5 | vec | ^^^ error: aborting due to previous error(s) ``` Fixes #26046 r? @nikomatsakis cc @doomrobo

tommyip added 2 commits May 24, 2017 16:23

Include context info into closure_kinds

7748bc6

Explain why a closure is FnOnce in closure errors

bf25b5e

rust-highfive assigned eddyb May 24, 2017

tommyip commented May 24, 2017

View reviewed changes

aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 25, 2017

nikomatsakis reviewed May 25, 2017

View reviewed changes

Mark-Simulacrum 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 May 28, 2017

Mark-Simulacrum assigned nikomatsakis and unassigned eddyb May 28, 2017

Explain why closure is moved in error message

ee88a87

tommyip changed the title ~~[WIP] Explain why a closure is FnOnce in closure errors.~~ Explain why a closure is FnOnce in closure errors. May 30, 2017

Update fn_once-moved test

31bfdd7

tommyip force-pushed the explain_closure_err branch from 810de2a to 31bfdd7 Compare May 30, 2017 10:21

nikomatsakis reviewed May 30, 2017

View reviewed changes

Update closure errors to use span_note

c2f7e94

tommyip force-pushed the explain_closure_err branch from b24d465 to c2f7e94 Compare May 31, 2017 09:15

Mark-Simulacrum mentioned this pull request May 31, 2017

Rollup of 7 pull requests #42336

Merged

bors added a commit that referenced this pull request May 31, 2017

Auto merge of #42336 - Mark-Simulacrum:rollup, r=Mark-Simulacrum

e0cc22b

Rollup of 7 pull requests - Successful merges: #42126, #42196, #42252, #42277, #42315, #42329, #42330 - Failed merges:

Mark-Simulacrum mentioned this pull request May 31, 2017

Opaque error message when typechecking closure #26046

Closed

bors merged commit c2f7e94 into rust-lang:master May 31, 2017

tommyip deleted the explain_closure_err branch June 2, 2017 16:43

tommyip mentioned this pull request Jun 5, 2017

Better closure error message #42443

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explain why a closure is `FnOnce` in closure errors. #42196

Explain why a closure is `FnOnce` in closure errors. #42196

tommyip commented May 24, 2017 •

edited

Loading

rust-highfive commented May 24, 2017

tommyip May 24, 2017 •

edited

Loading

nikomatsakis May 25, 2017

nikomatsakis May 25, 2017

aidanhs commented May 25, 2017

eddyb commented May 25, 2017

nikomatsakis May 25, 2017

nikomatsakis May 25, 2017

nikomatsakis May 25, 2017

nikomatsakis May 25, 2017

nikomatsakis commented May 25, 2017

eddyb commented May 25, 2017

tommyip commented May 30, 2017

nikomatsakis May 30, 2017

tommyip May 30, 2017

nikomatsakis May 30, 2017 •

edited

Loading

tommyip May 30, 2017 •

edited

Loading

tommyip May 30, 2017 •

edited

Loading

nikomatsakis May 30, 2017

tommyip commented May 31, 2017

nikomatsakis commented May 31, 2017

bors commented May 31, 2017

Mark-Simulacrum commented May 31, 2017

Explain why a closure is FnOnce in closure errors. #42196

Explain why a closure is FnOnce in closure errors. #42196

Conversation

tommyip commented May 24, 2017 • edited Loading

rust-highfive commented May 24, 2017

tommyip May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aidanhs commented May 25, 2017

eddyb commented May 25, 2017

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 May 25, 2017

eddyb commented May 25, 2017

tommyip commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis May 30, 2017 • edited Loading

Choose a reason for hiding this comment

tommyip May 30, 2017 • edited Loading

Choose a reason for hiding this comment

tommyip May 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommyip commented May 31, 2017

nikomatsakis commented May 31, 2017

bors commented May 31, 2017

Mark-Simulacrum commented May 31, 2017

Explain why a closure is `FnOnce` in closure errors. #42196

Explain why a closure is `FnOnce` in closure errors. #42196

tommyip commented May 24, 2017 •

edited

Loading

tommyip May 24, 2017 •

edited

Loading

nikomatsakis May 30, 2017 •

edited

Loading

tommyip May 30, 2017 •

edited

Loading

tommyip May 30, 2017 •

edited

Loading