Fix wrong report on closure args mismatch when a ref is expected but not found #42270

Emilgardis · 2017-05-27T21:41:00Z

Also adds test for this case.

rust-highfive · 2017-05-27T21:41:12Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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.

arielb1 · 2017-05-29T10:50:56Z

@Emilgardis

I would personally prefer to issue the error message when you find that the obligation & closure trait refs are function traits whose first arguments are tuples of different arg-counts, rather than trying to match against type errors.

nikomatsakis · 2017-06-01T19:57:07Z

I think this is the same thing that @arielb1 was saying, but I feel like we could do a lot better with this error message. That is, ideally, we would tell the user that the closure is declared with argument type (u32, u32), but the iterator is providing a &(u32, u32). It's a bit tricky to find just the right phrasing here but I think we can do it. :) @Emilgardis -- are you up for trying to go the extra mile here?

I think the error message we want is probably something like this:

error[E0593]: type mismatch in closure arguments
 --> src/main.rs:4:30
  |
4 |     let b: Vec<_> = a.iter().map(|x: (u32, u32)| 45).collect(); 
  |                              ^^^ ------------------ takes a `(u32, u32)`
  |                              |
  |                              expected closure that takes a `&(u32, u32)`

nikomatsakis · 2017-06-02T17:43:51Z

src/librustc/traits/error_reporting.rs

@@ -684,24 +684,36 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                                                   expected_found.found,
                                                   expected_trait_ty.is_closure())
                } else if let &TypeError::Sorts(ref expected_found) = e {
-                    let expected = if let ty::TyTuple(tys, _) = expected_found.expected.sty {
+                    let expected_len = if let ty::TyTuple(tys, _) = expected_found.expected.sty {


Huh. I am a bit dubious about this if in the first-place. I guess I have to run the code -- what happens if you just remove this logic altogether, though? It feels very odd to me to have to do all this matching.

(In other words, the logic here feels quite suspect, even before your changes.)

Emilgardis · 2017-06-02T21:46:49Z

I agree with what has been commented here. I'll get to the implementation most likely tomorrow. Should I open a new pr for it or just force push a new commit?

Mark-Simulacrum · 2017-06-02T22:17:05Z

Force push is probably fine in this case since changes are small; in general just a normal push would be best

nikomatsakis · 2017-06-05T16:55:41Z

@Emilgardis force push is good -- thanks!

carols10cents · 2017-06-12T12:13:53Z

Hi @Emilgardis! Just a friendly ping to keep this on your radar :)

nikomatsakis · 2017-06-15T16:45:47Z

@Emilgardis do you think you know how to do the better error message, or do you want a few tips?

Emilgardis · 2017-06-17T13:44:13Z

Sorry for the delay, I have a unpushed commit on my local branch, ~~I've removed the match @nikomatsakis suspected to be uneeded. So far no bugs regarding that change.~~ EDIT:: this change removed E0593 completely.

I've also implemented E0604, and I'm now trying to get a correct output of the types that where mismatched. i.e show &(u32, u32) instead of std::ops::FnOnce<(&(u32, u32),)> (the latter is Display from a ty::Binder<ty::sty::TraitRef<'tcx>>

Emilgardis · 2017-06-18T08:05:48Z

I realised that the error:

[E0604]: type mismatch in closure arguments
 --> src/main.rs:4:30
  |
4 |     let b: Vec<_> = a.iter().map(|x: (u32, u32)| 45).collect(); 
  |                              ^^^ ------------------ takes a `(u32, u32)`
  |                              |
  |                              expected closure that takes a `&(u32, u32)`

Is awfully closely related to E0281, should I remove my local E0604 and just change E0281 to give prettier output and have it be called on code like the example above instead of E0604?

281 is currently

fn foo<F>(_m: F) where F: Fn(usize) {}
----------
error[E0281]: type mismatch: `[closure@src/lib.rs:11:13: 11:26]` implements the trait `core::ops::FnOnce<(isize,)>`, but the trait `core::ops::FnOnce<(usize,)>` is required
  --> src/lib.rs:11:9
   |
11 |         foo(|y: isize| ());
   |         ^^^ ------------- implements `core::ops::FnOnce<(isize,)>`
   |         |
   |         requires `core::ops::FnOnce<(usize,)>`
   |         expected usize, found isize

arielb1 · 2017-06-20T20:16:38Z

Do you need any help @Emilgardis? Feel free to ask me on IRC (I'm arielb1 or arielby).

nikomatsakis · 2017-06-26T15:29:21Z

@Emilgardis it's always a tough call where to use a distinct error number. I think in this case it is worth keeping a distinct error number, though. Basically this is because we can give a more specialized --explain description targeting the particular case of closure arguments. Also, there is a sort of distinction here: the more general mismatched types refers to the value itself, whereas this is saying that the types in the trait that the closure implements don't match.

(Also, the current infrastructure makes it difficult to combine error messages in any case.)

nikomatsakis · 2017-06-26T15:29:55Z

@Emilgardis sounds like you have something working? I guess you didn't yet push those commits? The preview looks very appealing though!

arielb1 · 2017-06-27T20:44:53Z

@Emilgardis I couldn't find you on IRC. Are you still interested in working on this? Should I get this done myself?

Emilgardis · 2017-06-28T21:27:28Z

Sorry for not being active, I'll get to this tomorrow (in 7-10 hours). I'm Emilgardis on mozilla

aidanhs · 2017-07-05T17:36:15Z

Hi @Emilgardis, did you get a chance to come back to this?

Emilgardis · 2017-07-06T10:13:21Z

Yes, I have a working branch on temp-issue-42143. I'll get to rebasing it onto changes upstream today regarding a new error code

nikomatsakis · 2017-07-06T16:03:38Z

@Emilgardis awesome :)

when a ref is expected but not found. Adds new error E0622 Fixes rust-lang#42143

Emilgardis · 2017-07-07T01:05:46Z

There we go. Should be all fine.

I'll address some concerns I have in a review.

Emilgardis · 2017-07-07T01:06:36Z

src/test/ui/mismatched_types/closure-arg-type-mismatch.stderr

+  --> $DIR/closure-arg-type-mismatch.rs:16:27
+   |
+16 |         let d1 = a.iter().map(|x: &(u16,u16)| 45);
+   |                           ^^^ ------------------ takes a `for<'r> std::ops::FnMut<(&'r (u16, u16),)>`


Should this binder/lifetime be displayed?

Emilgardis · 2017-07-07T01:07:58Z

src/test/ui/mismatched_types/closure-arg-type-mismatch.stderr

+15 |         let b = a.iter().map(|x: (u32, u32)| 45);
+   |                          ^^^ ------------------ takes a `std::ops::FnMut<((u32, u32),)>`
+   |                          |
+   |                          expected closure that takes a `std::ops::FnMut<(&(u32, u32),)>`


I'm thinking about implementing a hint telling the user to add a &/&mut to the closure

Also, I couldn't find a way (except for "parsing") to convert Fn*<([&]({T} ... ))> (meaning what is displayed here as FnMut<(&(u32, u32),)>) into a simple &(u32, u32) as @nikomatsakis suggested.

New error code is E0623 Also cleanup/fix ui test

Emilgardis · 2017-07-07T14:36:38Z

I'm not sure how to proceed. E0281 is very similar to E0623. Infact, what I can see the two errors are almost always the same.

nikomatsakis · 2017-07-07T16:59:14Z

src/librustc/traits/error_reporting.rs

+        let mut err = struct_span_err!(self.tcx.sess, span, E0623,
+            "type mismatch in closure arguments");
+        if let Some(sp) = found_span {
+            err.span_label(span, format!("expected closure that takes a `{}`", found));


we should be able to extract the types of the arguments by examining the substs of the expected_ref and found trait-refs. Something like:

let expected_args = expected_ref.map_bound_ref(|trait_ref| trait_ref.substs.type_at(1));

But it might need to be type_at(0). That will give you a Binder<Ty<'tcx>>, which you should be able to use with {} I think.

nikomatsakis

This is looking good! Last step is just to extract the types, and not print the whole trait, left some comments, let me know how it goes (feel free to reach out on IRC etc)

nikomatsakis · 2017-07-11T16:46:42Z

@Emilgardis ping?

arielb1 · 2017-07-18T12:46:57Z

@Emilgardis ping. I'll be on vacation for the rest of the week, but if you need any help you can talk with @nikomatsakis .

arielb1 · 2017-07-25T23:08:56Z

Closing due to inactivity. Feel free to reopen when you can find me.

@nikomatsakis

Friendlier error message for closure argument type mismatch Rebased #42270. Fixes #42143. --- `test.rs`: ```rust fn main() { foo(|_: i32, _: usize| ()); } fn foo<F>(_: F) where F: Fn(&str, usize) {} ``` Before: ``` error[E0281]: type mismatch: `[[email protected]:2:9: 2:30]` implements the trait `std::ops::Fn<(i32, usize)>`, but the trait `for<'r> std::ops::Fn<(&'r str, usize)>` is required --> test.rs:2:5 | 2 | foo(|_: i32, _: usize| ()); | ^^^ --------------------- implements `std::ops::Fn<(i32, usize)>` | | | expected &str, found i32 | requires `for<'r> std::ops::Fn<(&'r str, usize)>` | = note: required by `foo` ``` After (early): ``` error[E0631]: type mismatch in closure arguments --> test.rs:2:5 | 2 | foo(|_: i32, _: usize| ()); | ^^^ --------------------- takes arguments of type `i32` and `usize` | | | expected arguments of type `&str` and `usize` | = note: required by `foo` ``` After (current): ``` error[E0631]: type mismatch in closure arguments --> test.rs:2:5 | 2 | foo(|_: i32, _: usize| ()); | ^^^ --------------------- found signature of `fn(i32, usize) -> _` | | | expected signature of `for<'r> fn(&'r str, usize) -> _` | = note: required by `foo` ``` ~~Compiler output has been changed, and a few tests are failing. Help me writing/fixing tests!~~ r? @nikomatsakis

rust-highfive assigned nikomatsakis May 27, 2017

Emilgardis mentioned this pull request May 27, 2017

Wrong error message with closures on nightly #42143

Closed

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

alexcrichton 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 Jun 1, 2017

nikomatsakis reviewed Jun 2, 2017

View reviewed changes

Emilgardis changed the title ~~Fixes #42143~~ Fix wrong report on closure args mismatch when a ref is expected but not found Jun 5, 2017

Fix wrong report on closure args mismatch

994d76d

when a ref is expected but not found. Adds new error E0622 Fixes rust-lang#42143

Emilgardis force-pushed the issue-42143 branch from 9b838a7 to 994d76d Compare July 7, 2017 01:04

Emilgardis commented Jul 7, 2017

View reviewed changes

Emilgardis force-pushed the issue-42143 branch from 708015d to 91ad3cc Compare July 7, 2017 02:49

Emilgardis force-pushed the issue-42143 branch 2 times, most recently from f2b7256 to a41f93f Compare July 7, 2017 03:17

Fix trailing whitespace and duplicate error code.

74cb9b9

New error code is E0623 Also cleanup/fix ui test

Emilgardis force-pushed the issue-42143 branch from a41f93f to 74cb9b9 Compare July 7, 2017 03:20

nikomatsakis reviewed Jul 7, 2017

View reviewed changes

nikomatsakis requested changes Jul 7, 2017

View reviewed changes

arielb1 closed this Jul 25, 2017

tirr-c mentioned this pull request Sep 21, 2017

Friendlier error message for closure argument type mismatch #44735

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wrong report on closure args mismatch when a ref is expected but not found #42270

Fix wrong report on closure args mismatch when a ref is expected but not found #42270

Emilgardis commented May 27, 2017 •

edited by Mark-Simulacrum

Loading

rust-highfive commented May 27, 2017

arielb1 commented May 29, 2017 •

edited

Loading

nikomatsakis commented Jun 1, 2017 •

edited

Loading

nikomatsakis Jun 2, 2017

nikomatsakis Jun 2, 2017

Emilgardis commented Jun 2, 2017

Mark-Simulacrum commented Jun 2, 2017

nikomatsakis commented Jun 5, 2017

carols10cents commented Jun 12, 2017

nikomatsakis commented Jun 15, 2017

Emilgardis commented Jun 17, 2017 •

edited

Loading

Emilgardis commented Jun 18, 2017 •

edited

Loading

arielb1 commented Jun 20, 2017

nikomatsakis commented Jun 26, 2017

nikomatsakis commented Jun 26, 2017

arielb1 commented Jun 27, 2017

Emilgardis commented Jun 28, 2017 •

edited

Loading

aidanhs commented Jul 5, 2017

Emilgardis commented Jul 6, 2017 •

edited

Loading

nikomatsakis commented Jul 6, 2017

Emilgardis commented Jul 7, 2017 •

edited

Loading

Emilgardis Jul 7, 2017

Emilgardis Jul 7, 2017

Emilgardis Jul 7, 2017

Emilgardis commented Jul 7, 2017

nikomatsakis Jul 7, 2017 •

edited

Loading

nikomatsakis left a comment

nikomatsakis commented Jul 11, 2017

arielb1 commented Jul 18, 2017

arielb1 commented Jul 25, 2017

Fix wrong report on closure args mismatch when a ref is expected but not found #42270

Fix wrong report on closure args mismatch when a ref is expected but not found #42270

Conversation

Emilgardis commented May 27, 2017 • edited by Mark-Simulacrum Loading

rust-highfive commented May 27, 2017

arielb1 commented May 29, 2017 • edited Loading

nikomatsakis commented Jun 1, 2017 • edited Loading

nikomatsakis Jun 2, 2017

Choose a reason for hiding this comment

nikomatsakis Jun 2, 2017

Choose a reason for hiding this comment

Emilgardis commented Jun 2, 2017

Mark-Simulacrum commented Jun 2, 2017

nikomatsakis commented Jun 5, 2017

carols10cents commented Jun 12, 2017

nikomatsakis commented Jun 15, 2017

Emilgardis commented Jun 17, 2017 • edited Loading

Emilgardis commented Jun 18, 2017 • edited Loading

arielb1 commented Jun 20, 2017

nikomatsakis commented Jun 26, 2017

nikomatsakis commented Jun 26, 2017

arielb1 commented Jun 27, 2017

Emilgardis commented Jun 28, 2017 • edited Loading

aidanhs commented Jul 5, 2017

Emilgardis commented Jul 6, 2017 • edited Loading

nikomatsakis commented Jul 6, 2017

Emilgardis commented Jul 7, 2017 • edited Loading

Emilgardis Jul 7, 2017

Choose a reason for hiding this comment

Emilgardis Jul 7, 2017

Choose a reason for hiding this comment

Emilgardis Jul 7, 2017

Choose a reason for hiding this comment

Emilgardis commented Jul 7, 2017

nikomatsakis Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis commented Jul 11, 2017

arielb1 commented Jul 18, 2017

arielb1 commented Jul 25, 2017

Emilgardis commented May 27, 2017 •

edited by Mark-Simulacrum

Loading

arielb1 commented May 29, 2017 •

edited

Loading

nikomatsakis commented Jun 1, 2017 •

edited

Loading

Emilgardis commented Jun 17, 2017 •

edited

Loading

Emilgardis commented Jun 18, 2017 •

edited

Loading

Emilgardis commented Jun 28, 2017 •

edited

Loading

Emilgardis commented Jul 6, 2017 •

edited

Loading

Emilgardis commented Jul 7, 2017 •

edited

Loading

nikomatsakis Jul 7, 2017 •

edited

Loading