Better error for missing tuple pattern in args #45069

sinkuu · 2017-10-06T16:14:59Z

Before:

error[E0593]: closure takes 2 arguments but 1 argument is required
 --> test.rs:5:40
  |
5 |     let it = v.into_iter().enumerate().map(|i, x| i);
  |                                        ^^^ -------- takes 2 arguments
  |                                        |
  |                                        expected closure that takes 1 argument

After:

error[E0593]: closure takes 2 arguments but a 2-tuple is required
 --> test.rs:5:40
  |
5 |     let it = v.into_iter().enumerate().map(|i, x| i);
  |                                        ^^^ ------ takes 2 arguments
  |                                        |
  |                                        expected closure that takes a 2-tuple

rust-highfive · 2017-10-06T16:15:06Z

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.

eddyb · 2017-10-06T16:59:22Z

r? @nikomatsakis

nikomatsakis · 2017-10-06T20:09:33Z

src/librustc/traits/error_reporting.rs

+                        None
+                    };
+
+                    // FIXME(#44150): Expand this to "N args expected but a N-tuple found."


Why does this say "FIXME"?

This PR improves message for "expected |(x, y)|, found |x, y|" case but not "expected |x, y|, found |(x, y)|" case. In the latter case, found_trait_ref should be FnMut<((_, _),)>, but it becomes FnMut<(_,)> and I don't know how to fix.

@nikomatsakis, I believe @sinkuu is referring to the following case:

18 | f(|(x, y)| x + y); | ^ -------- takes a single tuple as an argument | | | | | help: consider changing to: `|x, y|` | | | expected closure that takes 2 arguments

We should create an issue after this PR lands to track that case.

nikomatsakis · 2017-10-06T20:20:40Z

This seems like a great thing to fix. I am wondering though if we should tweak the output. I personally found the new message a bit hard to parse, and I think it could be confusing. Instead of changing the actual text of the error message, I wonder if it'd be better to offer a suggestion. I am imagining something like this:

In the event that N parameters were found
But 1 parameter was expected, and it is a tuple of arity N
Then offer a suggestion like "a N-tuple was expected, consider changing the closure signature like this: |(a, b)|

I imagine it looking something like this in the end:

error[E0593]: closure takes 2 arguments but 1 argument is required
 --> test.rs:5:40
  |
5 |     let it = v.into_iter().enumerate().map(|i, x| i);
  |                                        ^^^ ------ takes 2 arguments
  |                                        |
  |                                        expected closure that takes 1 argument
  = help: consider changing the closure signature like this:
5 |     let it = v.into_iter().enumerate().map(|(i, x|) i);

Thoughts @estebank?

To do this, you would basically invoke span_suggestion on the DiagnosticBuilder returned by arg_count_mismatch (or at the end of that function).

We could also keep things just as they are, but also include the suggestion. I'm not sure which is better, but I definitely the suggestion is a good idea, since I think it will have a more obvious meaning to people than talking about 2-tuples.

nikomatsakis · 2017-10-06T20:21:55Z

Another option is to do something more dramatic:

error[E0593]: closure takes multiple arguments, but a tuple argument is required
 --> test.rs:5:40
  |
5 |     let it = v.into_iter().enumerate().map(|i, x| i);
  |                                        ^^^ ------ consider changing to `|(i, x)|`
  |                                        |
  |                                        expected closure that takes 1 argument, a 2-tuple

sinkuu · 2017-10-07T06:18:32Z

Added suggestions.

nikomatsakis

This is awesome! What do you think about my suggest changes to the error message wording?

cc @estebank -- this is definitely the kind of thing we should have a style guide for!

nikomatsakis · 2017-10-09T13:54:07Z

src/librustc/traits/error_reporting.rs

+                Cow::from(args_str(found))
+            },
+            if expected_tuple.is_some() {
+                Cow::from("a tuple argument")


I think I figured out what is bothering me about this wording. The construction is not quite parallel. It says "multiple arguments" are expected, but then says "but a tuple argument", which is not obviously the opposite. Also, our usual construction is "expected X, but found Y", whereas the message here says "found X, but Y is required" (pre-existing, I know, but I still find it a bit surprising).

Perhaps this?

expected a closure that takes a single tuple as argument but found a closure that takes N distinct arguments

Maybe that's too wordy. We could tweak it, though it goes a bit away from the "expected X but found Y" form:

expected closure to take a single tuple as argument but it takes N distinct arguments

or, maybe better:

closure is expected to take a single tuple as argument, but it takes N distinct arguments

(I would probably change the existing message that doesn't mention tuples to match, as well.)

It looks better, thanks! Reworded the message to the last one.

nikomatsakis · 2017-10-09T13:58:30Z

src/test/ui/mismatched_types/closure-arg-count.stderr

+  --> $DIR/closure-arg-count.rs:21:53
+   |
+21 |     let _it = vec![1, 2, 3].into_iter().enumerate().map(|i: usize, x| i);
+   |                                                     ^^^ ------------- help: consider changing to: `|(i, x): (usize, _)|`


this looks great

nikomatsakis · 2017-10-10T19:58:04Z

src/test/ui/mismatched_types/closure-arg-count.stderr

+20 |     let _it = vec![1, 2, 3].into_iter().enumerate().map(|i, x| i);
+   |                                                     ^^^ ------ help: consider changing to: `|(i, x)|`
+   |                                                     |
+   |                                                     expected closure that takes 1 argument, a 2-tuple


This is looking really good! I hope you don't mind me nitpicking some more though. =)

Reading this "expected closure that takes 1 argument, a 2-tuple" -- which I think I may even have initially suggested -- I see now that it is actually ambiguous. In particular, the intention was something like "that takes 1 argument, which is a 2-tuple", but it could also be parsed as a list, like "that takes 1 argument and a 2-tuple". In fact, this second interpretation is what I got this time, and I had to re-read it a few times to understand what was meant.

Therefore, I suggest:

"expected closure that takes a single tuple as argument"

which echoes the wording from above

estebank · 2017-10-10T20:23:42Z

src/librustc/traits/error_reporting.rs

+                        None
+                    };
+
+                    // FIXME(#44150): Expand this to "N args expected but a N-tuple found."


@nikomatsakis, I believe @sinkuu is referring to the following case:

18 | f(|(x, y)| x + y); | ^ -------- takes a single tuple as an argument | | | | | help: consider changing to: `|x, y|` | | | expected closure that takes 2 arguments

We should create an issue after this PR lands to track that case.

estebank · 2017-10-10T20:26:54Z

src/test/ui/mismatched_types/closure-arg-count.stderr

+  --> $DIR/closure-arg-count.rs:20:53
+   |
+20 |     let _it = vec![1, 2, 3].into_iter().enumerate().map(|i, x| i);
+   |                                                     ^^^ ------ help: consider changing to: `|(i, x)|`


Could you modify the suggestion text to be freestanding without the snippet? Something along the lines of consider changing the closure to accept a tuple. rustc is not the only way these messages can be presented, so they should make sense even when the suggested code is not presented next to it.

nikomatsakis · 2017-10-13T15:27:10Z

@bors r+

bors · 2017-10-13T15:27:11Z

📌 Commit f577847 has been approved by nikomatsakis

bors · 2017-10-13T20:14:44Z

⌛ Testing commit f577847 with merge 02a24db...

Better error for missing tuple pattern in args #44150 Before: ``` error[E0593]: closure takes 2 arguments but 1 argument is required --> test.rs:5:40 | 5 | let it = v.into_iter().enumerate().map(|i, x| i); | ^^^ -------- takes 2 arguments | | | expected closure that takes 1 argument ``` After: ``` error[E0593]: closure takes 2 arguments but a 2-tuple is required --> test.rs:5:40 | 5 | let it = v.into_iter().enumerate().map(|i, x| i); | ^^^ ------ takes 2 arguments | | | expected closure that takes a 2-tuple ```

bors · 2017-10-13T22:56:48Z

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

ExpHP · 2017-10-24T23:07:06Z

Seeing this error message actually brightened up my day. So many times have I made this mistake...
Thanks for adding this.

sinkuu added 2 commits October 7, 2017 00:02

Better error for missing tuple pattern in args (rust-lang#44150)

8782d0f

Rename variables

b6eef82

rust-highfive assigned eddyb Oct 6, 2017

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

shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 6, 2017

nikomatsakis reviewed Oct 6, 2017

View reviewed changes

Add suggestion

c74bda1

nikomatsakis requested changes Oct 9, 2017

View reviewed changes

Reword

fa14f79

nikomatsakis requested changes Oct 10, 2017

View reviewed changes

estebank requested changes Oct 10, 2017

View reviewed changes

Reword

f577847

nikomatsakis approved these changes Oct 13, 2017

View reviewed changes

kennytm 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2017

estebank approved these changes Oct 13, 2017

View reviewed changes

bors merged commit f577847 into rust-lang:master Oct 13, 2017

bors mentioned this pull request Oct 13, 2017

new rules for merging expected and supplied types in closure signatures #45072

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better error for missing tuple pattern in args #45069

Better error for missing tuple pattern in args #45069

sinkuu commented Oct 6, 2017

rust-highfive commented Oct 6, 2017

eddyb commented Oct 6, 2017

nikomatsakis Oct 6, 2017

sinkuu Oct 7, 2017 •

edited

Loading

estebank Oct 10, 2017

nikomatsakis commented Oct 6, 2017

nikomatsakis commented Oct 6, 2017

sinkuu commented Oct 7, 2017

nikomatsakis left a comment

nikomatsakis Oct 9, 2017 •

edited

Loading

sinkuu Oct 10, 2017

nikomatsakis Oct 9, 2017

nikomatsakis Oct 10, 2017

estebank Oct 10, 2017

estebank Oct 10, 2017 •

edited

Loading

nikomatsakis commented Oct 13, 2017

bors commented Oct 13, 2017

bors commented Oct 13, 2017

bors commented Oct 13, 2017

ExpHP commented Oct 24, 2017

Better error for missing tuple pattern in args #45069

Better error for missing tuple pattern in args #45069

Conversation

sinkuu commented Oct 6, 2017

rust-highfive commented Oct 6, 2017

eddyb commented Oct 6, 2017

nikomatsakis Oct 6, 2017

Choose a reason for hiding this comment

sinkuu Oct 7, 2017 • edited Loading

Choose a reason for hiding this comment

estebank Oct 10, 2017

Choose a reason for hiding this comment

nikomatsakis commented Oct 6, 2017

nikomatsakis commented Oct 6, 2017

sinkuu commented Oct 7, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

sinkuu Oct 10, 2017

Choose a reason for hiding this comment

nikomatsakis Oct 9, 2017

Choose a reason for hiding this comment

nikomatsakis Oct 10, 2017

Choose a reason for hiding this comment

estebank Oct 10, 2017

Choose a reason for hiding this comment

estebank Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

nikomatsakis commented Oct 13, 2017

bors commented Oct 13, 2017

bors commented Oct 13, 2017

bors commented Oct 13, 2017

ExpHP commented Oct 24, 2017

sinkuu Oct 7, 2017 •

edited

Loading

nikomatsakis Oct 9, 2017 •

edited

Loading

estebank Oct 10, 2017 •

edited

Loading