Add help message if a FnOnce is moved #41772

GuillaumeGomez · 2017-05-05T21:29:02Z

estebank · 2017-05-05T22:09:03Z

src/test/ui/fn_once-moved.stderr

+   |     ^^^^^^^^^^^^^^^ value used here after move
+   |
+   = help: closure was moved because it only implements `FnOnce`
+   = note: move occurs because `debug_dump_dict` has type `[closure@$DIR/fn_once-moved.rs:15:27: 19:6 dict:std::collections::HashMap<i32, i32>]`, which does not implement the `Copy` trait


Should this note be removed (only for this case)?

I was wondering about this actually...

It does actually have some helpful information: std::collections::HashMap<i32, i32> ... Copy. I don't know if that's intentionally there, though...

I think we should better keep it.

Personally, I think it should be removed. I highly doubt anyone will extract much value out of it -- particularly in non-trivial cases with more upvars. It'd be better to extend the upvar inference to track why a move is occurring, I suspect.

@GuillaumeGomez, interested in pursuing that?

estebank · 2017-05-05T22:09:13Z

src/test/ui/fn_once-moved.stderr

+21 |     debug_dump_dict();
+   |     ^^^^^^^^^^^^^^^ value used here after move
+   |
+   = help: closure was moved because it only implements `FnOnce`


Should this point at the closure's span? If so, the text should be changed to something along the lines of

error[E0382]: use of moved value: `debug_dump_dict` --> $DIR/fn_once-moved.rs:21:5 15 | let debug_dump_dict = || { | --------------- closure only implements `FnOnce` so calling it moves it ... 20 | debug_dump_dict(); | --------------- value moved here 21 | debug_dump_dict(); | ^^^^^^^^^^^^^^^ value used here after move

Good point!

eddyb · 2017-05-06T03:40:39Z

r? @nikomatsakis

nikomatsakis · 2017-05-08T17:58:47Z

src/librustc_borrowck/borrowck/mod.rs

+                    ty::TypeVariants::TyClosure(id, _) => {
+                        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`");


Hmm. I'm torn. This message is definitely an improvement. I wonder if we can't do better. One thing that would be really nice is if we could show why the closure implements FnOnce (i.e., because it has a move from the environment, typically).

We would then say something like:

closure cannot be invoked twice because it moves the variable dict out of its environment

Computing this would require modifying upvar.rs to make the information available. But it seems worthwhile (it's been on my to-do list for a while). Possibly should be deferred to a separate PR, I'm not sure.

nikomatsakis · 2017-05-08T17:59:44Z

src/test/ui/fn_once-moved.stderr

+   |     ^^^^^^^^^^^^^^^ value used here after move
+   |
+   = help: closure was moved because it only implements `FnOnce`
+   = note: move occurs because `debug_dump_dict` has type `[closure@$DIR/fn_once-moved.rs:15:27: 19:6 dict:std::collections::HashMap<i32, i32>]`, which does not implement the `Copy` trait


Personally, I think it should be removed. I highly doubt anyone will extract much value out of it -- particularly in non-trivial cases with more upvars. It'd be better to extend the upvar inference to track why a move is occurring, I suspect.

@GuillaumeGomez, interested in pursuing that?

nikomatsakis · 2017-05-08T18:00:17Z

(To be clear, I'd be happy to mentor those changes.)

GuillaumeGomez · 2017-05-09T11:03:01Z

Totally interested in pursuing it.

nikomatsakis · 2017-05-16T00:48:03Z

@GuillaumeGomez shall we start by landing this PR, or you want to pursue in this PR?

The part of the code you have to look at a bit is in librustc_typeck/check/upvar.rs. This code basically walks the AST and observes what happens, gradually "bumping up" the closure-kind to be something more specific based on what it observes. Ultimately, these bumps occur by calls to adjust_closure_kind(). Right now, that method takes zero context as to why the bump occurred.

We would want to extend it with a span, I think. Then we could extend the temp_closure_kinds table to carry not only the closure kind but also the span that caused us to change it (and maybe a bit more info). We'd have to trace back to the callers to encode this span.

We'll also have to change the closure_kinds field of TypeckTables so that it maps not only to a ClosureKind but also the Span (and whatever else) that we are tracing through. This is a bit of a shame, since this info is only relevant in the local crate; I'm wondering then if we want to consider storing the span in some other place. But TypeckTables is realy going to be the most convenient place, so I think that's ultimately where I'd put it.

Question for @eddyb -- is it ok to have a Span in TypeckTables? Will the metadata code freak out? Should we avoid it?

eddyb · 2017-05-16T05:20:24Z

Span is fine anywhere nowadays.

GuillaumeGomez · 2017-05-16T08:13:27Z

@nikomatsakis: I think it'll take me quite some time to finish the full changes requested, so better merge this one and I'll open another one once I'll start. It'll allow me to not be bothered by master's changes too much.

nikomatsakis · 2017-05-17T19:56:58Z

@GuillaumeGomez ok, I opened #42065 and copied mentoring instructions in there. Maybe you or someone else will pick it up.

I'd be happy to r+ this, except that I still think we should suppress the note that dumps the full type of the closure. It's just too much information and I think it's unhelpful.

GuillaumeGomez · 2017-05-18T07:54:44Z

Ok, I'll remove it so we can move forward.

GuillaumeGomez · 2017-05-18T19:29:40Z

Updated.

estebank · 2017-05-19T21:22:33Z

@nikomatsakis approving

@bors r+

bors · 2017-05-19T21:22:33Z

📌 Commit 0c0d11b has been approved by estebank

bors · 2017-05-19T23:38:54Z

⌛ Testing commit 0c0d11b with merge 4662b15...

@eddyb

Add help message if a FnOnce is moved Fixes #40855. r? @eddyb

bors · 2017-05-20T02:13:22Z

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 4662b15 to master...

rust-highfive assigned pnkfelix May 5, 2017

GuillaumeGomez assigned eddyb and unassigned pnkfelix May 5, 2017

estebank reviewed May 5, 2017

View reviewed changes

rust-highfive assigned nikomatsakis and unassigned eddyb May 6, 2017

Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 7, 2017

nikomatsakis reviewed May 8, 2017

View reviewed changes

nikomatsakis mentioned this pull request May 17, 2017

closure errors should explain **why** a closure is FnOnce etc #42065

Closed

Add help message if a FnOnce is moved

0c0d11b

GuillaumeGomez force-pushed the fn-once-message branch from a327aff to 0c0d11b Compare May 18, 2017 19:29

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

Auto merge of #41772 - GuillaumeGomez:fn-once-message, r=estebank

4662b15

Add help message if a FnOnce is moved Fixes #40855. r? @eddyb

bors merged commit 0c0d11b into rust-lang:master May 20, 2017

GuillaumeGomez deleted the fn-once-message branch May 20, 2017 09:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add help message if a FnOnce is moved #41772

Add help message if a FnOnce is moved #41772

GuillaumeGomez commented May 5, 2017 •

edited

Loading

estebank May 5, 2017

GuillaumeGomez May 6, 2017

Mark-Simulacrum May 7, 2017

GuillaumeGomez May 7, 2017

nikomatsakis May 8, 2017

estebank May 5, 2017

GuillaumeGomez May 7, 2017

eddyb commented May 6, 2017

nikomatsakis May 8, 2017

nikomatsakis May 8, 2017

nikomatsakis commented May 8, 2017

GuillaumeGomez commented May 9, 2017

nikomatsakis commented May 16, 2017

eddyb commented May 16, 2017

GuillaumeGomez commented May 16, 2017

nikomatsakis commented May 17, 2017

GuillaumeGomez commented May 18, 2017

GuillaumeGomez commented May 18, 2017

estebank commented May 19, 2017

bors commented May 19, 2017

bors commented May 19, 2017

bors commented May 20, 2017

Add help message if a FnOnce is moved #41772

Add help message if a FnOnce is moved #41772

Conversation

GuillaumeGomez commented May 5, 2017 • 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

eddyb commented May 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented May 8, 2017

GuillaumeGomez commented May 9, 2017

nikomatsakis commented May 16, 2017

eddyb commented May 16, 2017

GuillaumeGomez commented May 16, 2017

nikomatsakis commented May 17, 2017

GuillaumeGomez commented May 18, 2017

GuillaumeGomez commented May 18, 2017

estebank commented May 19, 2017

bors commented May 19, 2017

bors commented May 19, 2017

bors commented May 20, 2017

GuillaumeGomez commented May 5, 2017 •

edited

Loading