Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show type parameter name and definition in type mismatch error messages #66014

Merged
merged 4 commits into from
Nov 6, 2019

Conversation

dkadashev
Copy link
Contributor

Fixes #47319

r? estebank

@rust-highfive
Copy link
Collaborator

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 1, 2019
@dkadashev
Copy link
Contributor Author

Obviously I've failed to put correct r? there. Not sure how to fix that now.

@estebank can you reviews this please. @davidtwco sorry for the false alarm :)

@davidtwco
Copy link
Member

r? @estebank

@rust-highfive rust-highfive assigned estebank and unassigned davidtwco Nov 1, 2019
@bors
Copy link
Contributor

bors commented Nov 1, 2019

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

Part of rust-lang#47319.

This just adds type parameter name to type mismatch error message, so
e.g. the following:

```
expected enum `std::option::Option`, found type parameter
```

becomes

```
expected enum `std::option::Option`, found type parameter `T`
```
Fixes rust-lang#47319.

Shows the type parameter definition(s) on type mismatch errors so the
context is clearer. Pretty much changes the following:

```
LL |     bar1(t);
   |          ^
   |          |
   |          expected enum `std::option::Option`, found type parameter `T`
```

into:

```
LL | fn foo1<T>(t: T) {
   |         - this type parameter
LL |     bar1(t);
   |          ^
   |          |
   |          expected enum `std::option::Option`, found type parameter `T`
```
Update the tests to reflect changes to how type mismatch errors are
reported (two previous commits).
Type parameters are referenced in the error message after the previous
few commits (using span label). But when the main error message already
references the very same type parameter it becomes clumsy. Do not show
the additional label in this case as per code review comment by
@estebank.

Also this contains a small style fix.
@dkadashev dkadashev force-pushed the 47319-type-param-def-location branch from 972664f to 774e60b Compare November 2, 2019 07:55
@estebank
Copy link
Contributor

estebank commented Nov 4, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Nov 4, 2019

📌 Commit 774e60b has been approved by estebank

@bors bors 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 Nov 4, 2019
Centril added a commit to Centril/rust that referenced this pull request Nov 5, 2019
…tion, r=estebank

Show type parameter name and definition in type mismatch error messages

Fixes rust-lang#47319

r? estebank
Centril added a commit to Centril/rust that referenced this pull request Nov 6, 2019
…tion, r=estebank

Show type parameter name and definition in type mismatch error messages

Fixes rust-lang#47319

r? estebank
bors added a commit that referenced this pull request Nov 6, 2019
Rollup of 11 pull requests

Successful merges:

 - #65892 (Remove `PartialEq` and `Eq` from the `SpecialDerives`.)
 - #66014 (Show type parameter name and definition in type mismatch error messages )
 - #66027 (Move has_panic_handler to query)
 - #66054 (syntax: Avoid span arithmetic for delimiter tokens)
 - #66068 (use silent emitter for rustdoc highlighting pass)
 - #66081 (let caller of check_ptr_access_align control the error message)
 - #66093 (Do not ICE with a precision flag in formatting str and no format arguments)
 - #66098 (Detect `::` -> `:` typo when involving turbofish)
 - #66101 (Tweak type mismatch caused by break on tail expr)
 - #66106 (Fix typo in explanation of `E0080`)
 - #66115 (rustc: remove "GlobalMetaData" dead code from hir::map::definitions.)

Failed merges:

r? @ghost
@bors bors merged commit 774e60b into rust-lang:master Nov 6, 2019
tesuji added a commit to tesuji/rust-clippy that referenced this pull request Nov 6, 2019
@dkadashev dkadashev deleted the 47319-type-param-def-location branch November 6, 2019 06:37
@@ -5,7 +5,9 @@ LL | fn foo<A: Debug>(&self, a: &A, b: &impl Debug);
| -- type in trait
...
LL | fn foo<B: Debug>(&self, a: &impl Debug, b: &B) { }
| ^^^^^^^^^^^ expected type parameter, found a different type parameter
| - ^^^^^^^^^^^ expected type parameter `B`, found type parameter `impl Debug`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling this "type parameter impl Debug" is a little confusing. What about "opaque type impl Debug", or something similar instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be reasonable. This output is just a consequence of opaque types being represented as type parameters and having to proactively account for them. Personally I would love to split them into two to avoid a lot of special casing we end up doing, like the one we would have to do here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing "type parameter" in error message.
6 participants