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

Centralize and clean type error reporting #34907

Merged
merged 14 commits into from Jul 28, 2016

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Jul 18, 2016

Refactors the code that handles type errors to be cleaner and fixes various edge cases.

This made the already-bad "type mismatch resolving" error message somewhat uglier. I want to fix that in another commit before this PR is merged.

Fixes #31173

r? @jonathandturner, cc @nikomatsakis

@arielb1
Copy link
Contributor Author

arielb1 commented Jul 18, 2016

Few more errors to modernize:

expected constant integer for repeat count, but mismatched types
array length constant evaluation error: mismatched types
implemented const has an incompatible type for trait

Will fix these tomorrow.

@arielb1
Copy link
Contributor Author

arielb1 commented Jul 18, 2016

All type errors now look like:

// newskool
error: mismatched types [--explain E0308]
 --> ../build-debug-assertions/fail.rs:3:32
  |>
3 |>     let x: Option<Option<_>> = y;
  |>                                ^ expected enum `std::option::Option`, found enum `std::result::Result`
note: expected type `std::option::Option<std::option::Option<_>>`
note:    found type `std::option::Option<std::result::Result<_, ()>>`




// oldskool
../build-debug-assertions/fail.rs:3:32: 3:33 error: mismatched types [E0308]
../build-debug-assertions/fail.rs:3     let x: Option<Option<_>> = y;
                                                                   ^
../build-debug-assertions/fail.rs:3:32: 3:33 help: run `rustc --explain E0308` to see a detailed explanation
../build-debug-assertions/fail.rs:3:32: 3:33 note: expected type `std::option::Option<std::option::Option<_>>` 
../build-debug-assertions/fail.rs:3:32: 3:33 note:    found type `std::option::Option<std::result::Result<_, ()>>` 
../build-debug-assertions/fail.rs:3:32: 3:33 note: expected enum `std::option::Option`, found enum `std::result::Result` 
../build-debug-assertions/fail.rs:3     let x: Option<Option<_>> = y;

@@ -16,7 +16,9 @@ struct Baz;

impl Foo for Baz {
fn bar(&mut self, other: &Foo) {}
//~^ ERROR method `bar` has an incompatible type for trait: values differ in mutability [E0053]
Copy link
Contributor

Choose a reason for hiding this comment

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

it's hard for me to tell from this diff: has the "values differ in mutability" part of the message been entirely removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the error message above ^. It is now a label in new-skool and a single note in old-skool.

@nikomatsakis
Copy link
Contributor

So I'm 👍 on removing the hack around newlines and centralizing the error reporting infrastructure here to go through a more common set of methods. That said, it seems like the new messages may be less specific and quite possibly less helpful than the old ones.

It might be though that if we combined this patch with something that gives intelligent diffs, that would be the best of both worlds.

Basically I think we want to avoid presenting people with two types and having them find the difference: so if we used to point out that the problem has to do with mutability, but now we make you figure it out for yourself, that could be a regression in usability. I also think that we calling everything "mismatched types" might be confused for people, if we can give a more specific indicator of where the type problem is.

Also, @arielb1, you mention this:

This made the already-bad "type mismatch resolving" error message somewhat uglier. I want to fix that in another commit before this PR is merged.

How does it look now? Can you give an example of what got uglier?

@arielb1 arielb1 force-pushed the found-parse-error branch 2 times, most recently from a44b5bf to 7966739 Compare July 20, 2016 22:15
@arielb1
Copy link
Contributor Author

arielb1 commented Jul 20, 2016

ready for review. I would like @nikomatsakis to look at the last commit (efe1a68).

@arielb1 arielb1 force-pushed the found-parse-error branch 8 times, most recently from 32fd204 to 5b2cf69 Compare July 21, 2016 15:14
@arielb1
Copy link
Contributor Author

arielb1 commented Jul 21, 2016

added a commit to fix the RFC1592 warnings (#33242, #33243) - they are there since 1.10 and I am planning to make more modifications to traits::error_reporting.

Unfortunately, projection errors do not come with a nice set of
mismatched types. This is because the type equality check occurs
within a higher-ranked context. Therefore, only the type error
is reported. This is ugly but was always the situation.

I will introduce better errors for the lower-ranked case in
another commit.

Fixes the last known occurence of rust-lang#31173
Refactor constant evaluation to use a single error reporting function
that reports a type-error-like message.

Also, unify all error codes with the "constant evaluation error" message
to just E0080, and similarly for a few other duplicate codes. The old
situation was a total mess, and now that we have *something* we can
further iterate on the UX.
The type equation in projection takes place under a binder and a snapshot, which
we can't easily take types out of. Instead, when encountering a projection error,
try to re-do the projection and find the type error then.

This fails to produce a sane type error when the failure was a "leak_check" failure.
I can't think of a sane way to show *these*, so I just left them use the old crappy
representation, and added a test to make sure we don't break them.
@arielb1
Copy link
Contributor Author

arielb1 commented Jul 24, 2016

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 24, 2016

📌 Commit 717e392 has been approved by nikomatsakis

@arielb1
Copy link
Contributor Author

arielb1 commented Jul 24, 2016

@bors retry

@arielb1
Copy link
Contributor Author

arielb1 commented Jul 24, 2016

@bors r-

@arielb1
Copy link
Contributor Author

arielb1 commented Jul 24, 2016

@bors retry r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 24, 2016

📌 Commit 717e392 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 25, 2016

⌛ Testing commit 717e392 with merge d398d9c...

@bors
Copy link
Contributor

bors commented Jul 25, 2016

💔 Test failed - auto-linux-cross-opt

@alexcrichton
Copy link
Member

@bors: retry

weeps

On Sun, Jul 24, 2016 at 10:08 PM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-cross-opt
https://buildbot.rust-lang.org/builders/auto-linux-cross-opt/builds/3104


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34907 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95PxFHLDN_ToYfmLuFhGWjiCn64Olks5qZES7gaJpZM4JPMZs
.

@bors
Copy link
Contributor

bors commented Jul 25, 2016

⌛ Testing commit 717e392 with merge eccf954...

@bors
Copy link
Contributor

bors commented Jul 25, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@alexcrichton
Copy link
Member

@bors: retry

On Sun, Jul 24, 2016 at 10:43 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt-rustbuild/builds/1924


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34907 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95HAJjINvrKQNlfGpLA7U_uCqR80kks5qZEz8gaJpZM4JPMZs
.

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Jul 25, 2016

⌛ Testing commit 717e392 with merge 084ebe7...

@bors
Copy link
Contributor

bors commented Jul 25, 2016

💔 Test failed - auto-win-gnu-32-opt

@alexcrichton
Copy link
Member

@bors: retry

On Mon, Jul 25, 2016 at 2:55 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt/builds/5069


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34907 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95Es23zS8rmNWCtgWwy2c_g4tY609ks5qZTC5gaJpZM4JPMZs
.

@bors
Copy link
Contributor

bors commented Jul 26, 2016

⌛ Testing commit 717e392 with merge 727c4dc...

@bors
Copy link
Contributor

bors commented Jul 26, 2016

💔 Test failed - auto-win-msvc-64-opt

@alexcrichton
Copy link
Member

@bors: retry

On Tue, Jul 26, 2016 at 3:13 AM, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-opt
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt/builds/5067


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34907 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95D0fOiaZG2vDG9bH59DTjnEGTYosks5qZd3ZgaJpZM4JPMZs
.

@bors
Copy link
Contributor

bors commented Jul 27, 2016

⌛ Testing commit 717e392 with merge f2e59cc...

bors added a commit that referenced this pull request Jul 27, 2016
Centralize and clean type error reporting

Refactors the code that handles type errors to be cleaner and fixes various edge cases.

This made the already-bad "type mismatch resolving" error message somewhat uglier. I want to fix that in another commit before this PR is merged.

Fixes #31173

r? @jonathandturner, cc @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE when attempting to split error message that contains " found" before "expected" into lines
6 participants