Friendlier error message for closure argument type mismatch by tirr-c · Pull Request #44735 · rust-lang/rust

tirr-c · 2017-09-21T09:18:29Z

test.rs:

fnmain(){foo(|_:i32, _:usize| ());}fnfoo<F>(_:F)whereF:Fn(&str,usize){}

Before:

error[E0281]: type mismatch: `[closure@test.rs: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 · 2017-09-21T09:18:43Z

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-09-21T09:28:08Z

You're still matching against the expected/found, which could lead to this bogusly triggering when one of the arguments is a tuple - when you have something like fn((u32, u32)): Fn((u32,)).
There's no reason to do so - you have the trait-refs in expected_trait_ref and actual_trait_ref.
You can get the argument length of each argument in this way:
// skipping the binder is ok because we only care if this is a tuplelet arg_len = matchX_trait_ref.skip_binder().substs.type_at(1){ ty::TyTuple(ref tys) => Some(tys.len()), _ => None};

tirr-c

I have a few questions. I've contributed to Rust a little, but still not sure how to write compiler code...

tirr-c · 2017-09-21T09:23:22Z

Is this code unreachable? I mean, will report_closure_arg_mismatch be called even when one of the Fn traits has zero arguments?

Why aren't you calling report_type_argument_mismatch?

I think you won't see this error message - no arguments, no place for a type error.

tirr-c · 2017-09-21T09:27:21Z

Is it fine to use standard panic macro like panic! or unreachable! to signal compiler bugs? (panic! here is a mistake, I should have used unreachable! here)

An unreachable!() is ok-ish for local things, but it is recommended to use bug! or span_bug!(span, "error") - span_bug also displays the span of the error, which makes for easier debugging.

arielb1 · 2017-09-21T12:06:02Z

You should display the "takes" even when you don't have a span - aka fn pointers:
fnfoo(f:fn((u32,u32))){ a.iter().map(f)}

I think you should add a test for each case with fn pointer - arg count mismatch, same arg count but type-error, higher-ranked error, F: Fn<u32> bad bound.

Higher-ranked test for fn pointers:
fnfoo<F:Fn(*mut&u32)>(_f:F){}fnbar<'a>(f:fn(*mut&'au32)){foo(f);}
Higher-ranked test for closures: src/test/ui/mismatched_types/closure-mismatch.rs (already exists)
Just check that the error message is sane.

Can I use note here?

With respect to the span specifically, probably just something like this would be fine:
let found_span = found_span.unwrap_or(span);
i.e., if we don't have a better span, display both messages at the same place.

nikomatsakis · 2017-09-21T13:07:11Z

@tirr-c this looks awesome btw!

tirr-c · 2017-09-21T14:24:56Z

Tried @arielb1's test code, and I get two errors.

#![feature(unboxed_closures)]fnmain(){}fnfoo<F:Fn(*mut&u32)>(_f:F){}fnbar<'a>(f:fn(*mut&'au32)){foo(f);}

(ignore feature line there)

error[E0631]: type mismatch in closure arguments
--> test.rs:5:35
|
5 | fn bar<'a>(f: fn(*mut &'a u32)) { foo(f); }
| ^^^
| |
| expected argument of type `*mut &u32`
| takes argument of type `*mut &'a u32`
|
= note: required by `foo`
error[E0271]: type mismatch resolving `for<'r> <fn(*mut &'a u32) as std::ops::FnOnce<(*mut &'r u32,)>>::Output == ()`
--> test.rs:5:35
|
5 | fn bar<'a>(f: fn(*mut &'a u32)) { foo(f); }
| ^^^ expected bound lifetime parameter, found concrete lifetime
|
= note: required by `foo`
error: aborting due to 2 previous errors

Is this okay?

arielb1 · 2017-09-21T14:28:36Z

Is this okay?

Your error message says "closure" for the function pointer. You should make it say "function" (you can use expected_trait_ty.is_closure() to check whether it's a function.

arielb1 · 2017-09-21T14:37:29Z

Other than that the error messages look sane. Just be sure to add a UI test so I can see them before giving the r+.

Otherwise LGTM.

tirr-c · 2017-09-21T16:14:56Z

I've removed the test mismatched_types/E0281 because most of the E0281 errors are replaced by E0631 which is added in this PR, and mismatched_types/closure-arg-type-mismatch contains tests for both E0281 and E0631.

arielb1 · 2017-09-21T16:19:50Z

If E0281 still exists, please write a test that reproduces it. That's the Fn<usize> test.

arielb1 · 2017-09-21T16:22:24Z

This erases the binder. I don't like this error message.

@arielb1 the expected and found signatures are in the labels now, I'm not sure if that was the case when you left this comment.

arielb1 · 2017-09-21T16:23:40Z

Please add the test for E0281 back - all error codes are supposed to have a test for them, and you can similarly add a test for E0631 with the name E0631.rs. We also need to think of how to handle closure-mismatch.

arielb1 · 2017-09-21T18:27:50Z

[01:05:30] failures:
[01:05:30] [01:05:30] ---- /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index (line 4928) stdout ----
[01:05:30] error[E0631]: type mismatch in closure arguments
[01:05:30] --> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:7:5
[01:05:30] |
[01:05:30] 7 | foo(|y: String| { });
[01:05:30] | ^^^ --------------- takes argument of type `std::string::String`
[01:05:30] | |
[01:05:30] | expected argument of type `usize`
[01:05:30] |
[01:05:30] = note: required by `foo`
[01:05:30] [01:05:30] thread 'rustc' panicked at 'Some expected error codes were not found: ["E0281"]', /checkout/src/librustdoc/test.rs:286:8
[01:05:30] [01:05:30] [01:05:30] failures:
[01:05:30] /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index (line 4928)
[01:05:30] [01:05:30] test result: FAILED. 659 passed; 1 failed; 19 ignored; 0 measured; 0 filtered out
[01:05:30] [01:05:30]

there's another doctest for E0281 in

rust/src/librustc/diagnostics.rs

Line 1141 in 1b55d19

E0281:r##"

, you need to fix it

nikomatsakis · 2017-09-21T19:04:42Z

r? @arielb1

tirr-c · 2017-09-21T19:08:07Z

Should pass the test, but the documentation needs to be rewritten now I think...

arielb1 · 2017-09-21T19:08:41Z

now you just need to use fn in the error messages.

tirr-c · 2017-09-22T13:50:41Z

error[E0631]: type mismatch in closure arguments
--> src/test/ui/mismatched_types/closure-mismatch.rs:18:5
|
18 | baz(|_| ());
| ^^^ ------ found signature of fn(_) -> _
| |
| expected signature of for<'r> fn(&'r ()) -> _
|
= note: required because of the requirements on the impl of `Foo` for `[closure@src/test/ui/mismatched_types/closure-mismatch.rs:18:9: 18:15]`
= note: required by `baz`

tirr-c · 2017-09-22T14:18:58Z

Error message from the test for Fn<usize> became worse.

Before:

error[E0593]: closure takes 0 arguments but 1 argument is required
--> src/test/ui/mismatched_types/closure-arg-type-mismatch.rs:26:5
|
26 | bar(|| {});
| ^^^ ----- takes 0 arguments
| |
| expected closure that takes 1 argument
|
= note: required by `bar`

After:

error[E0281]: type mismatch: `[closure@src/test/ui/mismatched_types/closure-arg-type-mismatch.rs:26:9: 26:14]` implements the trait `std::ops::Fn<()>`, but the trait `std::ops::Fn<usize>` is required
--> src/test/ui/mismatched_types/closure-arg-type-mismatch.rs:26:5
|
26 | bar(|| {});
| ^^^ ----- implements `std::ops::Fn<()>`
| |
| expected usize, found ()
| requires `std::ops::Fn<usize>`
|
= note: required by `bar`

tirr-c · 2017-09-22T16:13:53Z

error[E0631]: type mismatch in closure arguments
--> E0631.rs:17:5
|
17 | foo(|_: isize| {});
| ^^^ ------------- found signature of fn(isize) -> _
| |
| expected signature of fn(usize) -> _
|
= note: required by `foo`
error[E0631]: type mismatch in closure arguments
--> E0631.rs:18:5
|
18 | bar(|_: isize| {});
| ^^^ ------------- found signature of fn(isize) -> _
| |
| expected signature of fn(usize) -> _
|
= note: required by `bar`

E0281, the original error code, is totally replaced by E0631. I left E0281 in the error code index, though.

bors · 2017-09-22T17:21:27Z

☔ The latest upstream changes (presumably #44691) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2017-09-22T19:51:09Z

Nit: we should enclose the signature in backticks:
+ | ^^^^^^ -------------- found signature of `for<'r> fn(&'r str) -> _`

Fixesrust-lang#42143. E0281 is totally replaced by E0631. UI tests are updated accordingly.

arielb1 · 2017-09-24T12:24:24Z

That's cool.

@bors r+

bors · 2017-09-24T12:24:25Z

📌 Commit 1bfbfb2 has been approved by arielb1

bors · 2017-09-26T05:02:09Z

⌛ Testing commit 1bfbfb2 with merge 4b8bf39...

@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: `[closure@test.rs: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

bors · 2017-09-26T09:18:58Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 4b8bf39 to master...

rust-highfive assigned nikomatsakis Sep 21, 2017

arielb1 reviewed Sep 21, 2017
View reviewed changes

tirr-c commented Sep 21, 2017
View reviewed changes

arielb1 reviewed Sep 21, 2017
View reviewed changes

alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 21, 2017

rust-highfive assigned arielb1 and unassigned nikomatsakis Sep 21, 2017

tirr-c force-pushed the issue-42143 branch from 6764934 to 135545aCompare September 22, 2017 16:24

tirr-c force-pushed the issue-42143 branch from 135545a to 29e5db0Compare September 22, 2017 17:43

nikomatsakis reviewed Sep 22, 2017
View reviewed changes

tirr-c force-pushed the issue-42143 branch from 29e5db0 to cc56688Compare September 23, 2017 01:14

Print fn signature when there is closure argument type mismatch
1bfbfb2
Fixesrust-lang#42143. E0281 is totally replaced by E0631. UI tests are updated accordingly.

tirr-c force-pushed the issue-42143 branch from cc56688 to 1bfbfb2Compare September 23, 2017 01:15

bors merged commit 1bfbfb2 into rust-lang:masterSep 26, 2017

bors mentioned this pull request Sep 26, 2017
implement pattern-binding-modes RFC #44614
Merged

estebank mentioned this pull request Nov 27, 2017
Error about number of arguments, when number is correct but type is wrong #46295
Closed

Uh oh!

Uh oh!

Conversation

tirr-c commented Sep 21, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Sep 21, 2017

Uh oh!

arielb1Sep 21, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tirr-c left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1Sep 21, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Sep 21, 2017

Uh oh!

tirr-c commented Sep 21, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arielb1 commented Sep 21, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arielb1 commented Sep 21, 2017

Uh oh!

tirr-c commented Sep 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1 commented Sep 21, 2017

Uh oh!

arielb1 commented Sep 21, 2017

Uh oh!

nikomatsakis commented Sep 21, 2017

Uh oh!

tirr-c commented Sep 21, 2017

Uh oh!

arielb1 commented Sep 21, 2017

Uh oh!

tirr-c commented Sep 22, 2017

Uh oh!

tirr-c commented Sep 22, 2017

Uh oh!

tirr-c commented Sep 22, 2017

Uh oh!

bors commented Sep 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1 commented Sep 24, 2017

Uh oh!

bors commented Sep 24, 2017

tirr-c commented Sep 21, 2017•
edited
Loading

arielb1Sep 21, 2017•
edited
Loading

arielb1Sep 21, 2017•
edited
Loading

tirr-c commented Sep 21, 2017•
edited
Loading

arielb1 commented Sep 21, 2017•
edited
Loading