Better closure error message #42443

tommyip · 2017-06-05T10:42:08Z

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

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 · 2017-06-05T16:40:29Z

src/librustc/traits/error_reporting.rs

+                        };
+
+                        if let Some(tables) = infer_tables {
+                            if let Some(&(ty::ClosureKind::FnOnce, Some((span, name)))) =


This message is specific to FnOnce, but similar scenarios can arise in other situations. For example, we could highlight the x += 1 here and say something like "mutating this variable":

fn foo<F: Fn()>(f: F) { } fn main() { let mut x = 0; foo(|| x += 1); // Error }

Should we match more cases here or make the error more generic?

Good question. I think it'd be better to match more cases, and keep the message fairly tailored -- there are really only two cases. The closure might be FnOnce (in which case we can talk about something being moved) or it might be FnMut (in which case we can talk about something being written to).

It now handles both FnMut and FnOnce case.

This tests the FnMut case.

tommyip · 2017-06-06T09:51:48Z

Updated

nikomatsakis · 2017-06-06T18:39:56Z

src/test/ui/closure_context/issue-26046-fn-mut.stderr

+16 | |     };
+   | |_____^
+   |
+note: closure is `FnMut` because it mutates the variable `num` here


nikomatsakis · 2017-06-06T18:43:51Z

src/librustc/traits/error_reporting.rs

+                                        "closure is `FnMut` because it mutates the \
+                                         variable `{}` here", name));
+                                },
+                                _ => {}


One minor point -- in this arm, we emit neither the new note nor the old one. Seems like the old one might still be helpful?

Actually, maybe we should always emit the old note -- but instead of a note, just make it a span_label? Seems like it's useful information either way. In other words, the old note tells you "why it has to be Fn", and the new note tells you why it cannot be...

If you agree, maybe move the body of the else below out and above this if, and convert span_note to span_label (so that it prints within one code snippet)

Good point, I didn't think about that!

tommyip · 2017-06-07T10:45:22Z

For some reason span_label is producing a trailing whitespace:

  --> src/test/ui/closure_context/issue-26046-fn-once.rs:14:19
   |
14 |       let closure = move || {
   |  ___________________^
15 | |         vec
16 | |     };
   | |_____^
17 | 
18 |       Box::new(closure)
   |       ----------------- the requirement to implement `Fn` derives from here
   |
note: closure is `FnOnce` because it moves the variable `vec` out of its environment
  --> src/test/ui/closure_context/issue-26046-fn-once.rs:15:9
   |
15 |         vec
   |         ^^^

error: aborting due to previous error(s)

Next to 17 |.
I checked my changes and the ui tests and couldn't find anything that would cause this:

diff --git a/src/librustc/traits/error_reporting.rs b/src/librustc/traits/error_reporting.rs
index 7375338..c8e99c0 100644
--- a/src/librustc/traits/error_reporting.rs
+++ b/src/librustc/traits/error_reporting.rs
@@ -648,6 +648,10 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                             kind,
                             found_kind);
 
+                        err.span_label(
+                            obligation.cause.span,
+                            format!("the requirement to implement `{}` derives from here", kind));
+
                         let infer_tables = match self.tables {
                             InferTables::Interned(tables) =>
                                 Some(InferTablesRef::Interned(tables)),
@@ -656,6 +660,8 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                             InferTables::Missing => None,
                         };
 
+                        // Additional context information explaining why the closure only implements
+                        // a particular trait.
                         if let Some(tables) = infer_tables {
                             match tables.closure_kinds.get(&node_id) {
                                 Some(&(ty::ClosureKind::FnOnce, Some((span, name)))) => {
@@ -670,11 +676,6 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                                 },
                                 _ => {}
                             }
-                        } else {
-                            err.span_note(
-                                obligation.cause.span,
-                                &format!("the requirement to implement `{}` \
-                                          derives from here", kind));
                         }
 
                         err.emit();

nikomatsakis · 2017-06-07T20:23:19Z

@tommyip perhaps we just always print a trailing space in that scenario? Is that really a problem?

nikomatsakis · 2017-06-07T20:24:19Z

Er, I guess you didn't push the commits that implement that part yet?

tommyip · 2017-06-07T20:25:14Z

I'll push them now

tommyip · 2017-06-07T20:28:23Z

Pushed

nikomatsakis · 2017-06-07T20:52:02Z

src/test/ui/closure_context/issue-26046-fn-mut.stderr

@@ -6,6 +6,9 @@ error[E0525]: expected a closure that implements the `Fn` trait, but this closur
 15 | |         num += 1;
 16 | |     };
   | |_____^
+17 | 


cc @estebank -- @tommyip mentioned that there is a trailing space after this 17 | -- this doesn't bother me per se, but is that just how the diagnostics output works in general?

@nikomatsakis that space is to pad to compensate for the multiline markers. We can add a check for empty lines and avoid it.

nikomatsakis · 2017-06-08T08:31:04Z

@bors r+

bors · 2017-06-08T08:31:06Z

📌 Commit 345b833 has been approved by nikomatsakis

bors · 2017-06-08T08:37:55Z

⌛ Testing commit 345b833 with merge f09576c...

@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

bors · 2017-06-08T11:05:01Z

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

nikomatsakis · 2017-06-08T18:47:57Z

this is really cool :)

rust-highfive assigned nikomatsakis Jun 5, 2017

tommyip added 2 commits June 5, 2017 12:01

Add ui tests for issue 26046

9cbb5f9

tommyip force-pushed the better_closure_msg branch from c04d0a3 to 9cbb5f9 Compare June 5, 2017 11:02

carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 5, 2017

nikomatsakis requested changes Jun 5, 2017

View reviewed changes

tommyip added 3 commits June 6, 2017 09:06

Update closure errors to emit context for FnMut

94c808c

It now handles both FnMut and FnOnce case.

Add additional ui tests for issue 26046

2c282b8

This tests the FnMut case.

Group closure context ui tests

b1b6490

nikomatsakis reviewed Jun 6, 2017

View reviewed changes

nikomatsakis requested changes Jun 6, 2017

View reviewed changes

nikomatsakis approved these changes Jun 7, 2017

View reviewed changes

Cover all cases in closure errors

345b833

nikomatsakis reviewed Jun 7, 2017

View reviewed changes

bors merged commit 345b833 into rust-lang:master Jun 8, 2017

tommyip deleted the better_closure_msg branch June 8, 2017 11:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better closure error message #42443

Better closure error message #42443

tommyip commented Jun 5, 2017

nikomatsakis Jun 5, 2017

tommyip Jun 5, 2017

nikomatsakis Jun 5, 2017

tommyip commented Jun 6, 2017

nikomatsakis Jun 6, 2017

nikomatsakis Jun 6, 2017

tommyip Jun 6, 2017 •

edited

Loading

tommyip commented Jun 7, 2017 •

edited

Loading

nikomatsakis commented Jun 7, 2017

nikomatsakis commented Jun 7, 2017

tommyip commented Jun 7, 2017

tommyip commented Jun 7, 2017

nikomatsakis Jun 7, 2017

estebank Jun 7, 2017

nikomatsakis commented Jun 8, 2017

bors commented Jun 8, 2017

bors commented Jun 8, 2017

bors commented Jun 8, 2017

nikomatsakis commented Jun 8, 2017

Better closure error message #42443

Better closure error message #42443

Conversation

tommyip commented Jun 5, 2017

nikomatsakis Jun 5, 2017

Choose a reason for hiding this comment

tommyip Jun 5, 2017

Choose a reason for hiding this comment

nikomatsakis Jun 5, 2017

Choose a reason for hiding this comment

tommyip commented Jun 6, 2017

nikomatsakis Jun 6, 2017

Choose a reason for hiding this comment

nikomatsakis Jun 6, 2017

Choose a reason for hiding this comment

tommyip Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

tommyip commented Jun 7, 2017 • edited Loading

nikomatsakis commented Jun 7, 2017

nikomatsakis commented Jun 7, 2017

tommyip commented Jun 7, 2017

tommyip commented Jun 7, 2017

nikomatsakis Jun 7, 2017

Choose a reason for hiding this comment

estebank Jun 7, 2017

Choose a reason for hiding this comment

nikomatsakis commented Jun 8, 2017

bors commented Jun 8, 2017

bors commented Jun 8, 2017

bors commented Jun 8, 2017

nikomatsakis commented Jun 8, 2017

tommyip Jun 6, 2017 •

edited

Loading

tommyip commented Jun 7, 2017 •

edited

Loading