-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Multi-line errors #19870
Multi-line errors #19870
Conversation
We seriously need this in some form by 1.0. I absolutely love Rust but the error messages it vomits are completely nasty and I always find myself having to copy them into vim to manually line them up, which is very frustrating when I know that it can do this work itself, like GHC does with Haskell type errors. I can't imagine how much more frustrating it would be to a newcomer. This issue is compounded given the current type landscape where parameterized types explode in length (e.g. the various kinds of iterators). The primary target for compiler output should be humans. We shouldn't be bending over backwards to appease editors; we can create functionality on the side for them if need be, similar to git's porcelain flags:
|
Needs a rebase, and a reviewer. @mdinger do you know anyone who should review this? |
This looks great! r=me with a rebase. |
Rebased though I did modify some of the comments. I ran |
If compile-fail failed, then it didn't pass the test suite. compile-fail is only supposed to fail at compiling in a particular way. If it failed and printed out a test failure message, then you normally need to update the tests that failed. However, in this case, you’ll need to dig around in |
Yeah, I remember checking it before and it worked. I'll look into it. |
Rebased and passes I also rechecked vim support and it still seems fine. |
r? @cmr Let me know if this looks okay or not. I know it needs a rebase but I'd like to wait until I've heard back about it before I do. I think this will create a lot of merge conflicts and I don't want to continually rebase. |
So, the code looks good, but I'll leave the final r+ to @nikomatsakis or @nick29581, possibly after the alpha, since we're micromanaging the queue pretty hard already... |
Sounds good. Just let me know when to rebase so you can land it. |
format!("structure constructor specifies a \ | ||
structure of type `{}`, but this \ | ||
structure has type `{}`: {}", | ||
structure has type `{}`:\n {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks like an odd place for a newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was trying to fix this error type. It was probably printing out like this beforehand:
// Before
error: structure constructor specifies a structure of type: expected f32,
found int
// After
error: structure constructor specifies a structure of type
expected f32,
found int
I was trying to force all expected, found
patterns to be multi-line for consistency (except for a few which were easier to control).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how that lines up with error message in the code - where is the "but this structure has type" and where do the expected/found come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't have to give the entire error. Just a substring of the error because the regex is only looking for a match. That error is really long and I thought it might hit the make check
parsing width limit. So I trimmed it. The full script is here at playpen. The main part of the error which I pasted below:
<anon>:8:14: 8:20 error: structure constructor specifies a structure of type `Point<f32>`, but this structure has type `Point<int>`: expected f32, found int
<anon>:8 let pt = PointF {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see! Thanks for the explanation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but it's not obvious. A better, more thorough way would be much nicer.
Nice! I'm not sure about the approach though, it seems fragile and a recipe for inconsistent line breaks. Could we leave the error messges as they are and insert linebreaks by scanning for the expected and found substrings?. That would make it easier to customise error messages too. Also +1 on not landing till after the alpha release. |
While I agree that would be good, I don't know where to do the search/scanning. The myriad ways errors get printed out are very confusing. I could possibly do it but I'd need tips on where I need to look. |
In the end they should all go through Session::span_err or Session::err, I think. Certainly all the compiler ones. The libsyntax ones might go somewhere else, but there is probably either a common callee, or you could just ignore libsyntax errors to start with. |
token::get_name(trait_m.name), | ||
ty_err); | ||
} else { | ||
span_err!(tcx.sess, impl_m_span, E0053, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This throws a warning I think because E0053
was duplicated into this branch. Not sure how to avoid it.
I'll try and look into those |
5263eba
to
84ce667
Compare
It needs a rebase but it passes the testsuite again. It is more methodical now about enabling the multi-line errors. |
Rebased. This passed make check except for tidy which I fixed before rebase. Just waiting for a complete retest on tip. r? @nick29581 |
Passes make check. |
A quick review would be awesome because this needs a rebase with most compile-fail modifications. |
let first = Regex::new(r"[( ]expected").unwrap(); | ||
let second = Regex::new(r" found").unwrap(); | ||
let third = Regex::new( | ||
r"\((values differ|lifetime|cyclic type of infinite size)").unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: prefer to line break after the = rather than after the (. And/or break the string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want this trimmed? Raw strings have no escapes and breaking them can give surprising results when applied with a regex. Using non-raw strings also can give surprising results.
Technically I could do this but I decided against it because it makes it much less obvious what you're trying to do.
let s = r"\((values differ|lifetime|cyclic type of infinite size)";
// could also be
let v = vec![r"\((values differ",
r"lifetime",
r"cyclic type of infinite size)"].connect("|");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let third =
Regex::new(r"\((values differ|lifetime|cyclic type of infinite size)").unwrap();
second.find_iter(msg)) { | ||
// A `(` may be preceded by a space and it should be trimmed | ||
new_msg = new_msg + | ||
msg.slice(beg, pos1.0).trim_right() + // prefix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer [] slicing notation to slice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had that but it stopped working yesterday. I swapped to .slice()
to get it working again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be working again now, sorry about the churn
Looks great! Just to confirm, the error output looks the same as for the previous version? r=me with the minor comments fixed. |
I'll recheck with the changes and repost the output at the top. |
I'm very glad this is going through, thanks a ton @mdinger Just a quick question though, is there any reason this isn't using the |
I'm not sure. I think I tried to use it but it didn't compile and the other examples of regex I found didn't use the macro. So I just went ahead and used the regular. |
Did you see this? The advantage is that the regex gets compiled at compile time, so it's faster and if there are any issues you can get a compile-time error. Anyways it's not a big deal, just thought I'd bring it up. |
Yeah, I know. I just wasn't sure how to integrate it into the compiler (the compiler doesn't use cargo you know). It'd be an easy PR later too if someone knows how to add it. |
@nick29581 Updated the summary a little. The errors will look basically the same. The only real difference is that now, it is done systematically which allows manually opting in/out as desired. |
LGTM. Am I right to think that there is no way to turn off multi-line errors? Could you file a follow up issue for that please. Ping me for r+ when it's ready and rebased. |
@nick29581 rebased |
r? @nick29581 |
Passed I don't think there's a way to turn off the multi-line error checking thing. It's either every line or none. It depends though, it seems to ignore It's still only substring matches though so you don't need the entire message. I just copied them from the terminal though generally so they're pretty exact. |
@nick29581 Thanks for the review! |
#### Updated 1/12/2014 I updated the multi-line testcase to current but didn't modify the others. The spew code was broke by the `matches!` macro no longer working and I'm not interested in fixing the testcase. I additionally added one testcase below. Errors will in general look similar to below if the error is either `mismatched types` or a few other types. The rest are ignored. --- #### Extra testcase: ```rust pub trait Foo { type A; fn boo(&self) -> <Self as Foo>::A; } struct Bar; impl Foo for i32 { type A = u32; fn boo(&self) -> u32 { 42 } } fn foo1<I: Foo<A=Bar>>(x: I) { let _: Bar = x.boo(); } fn foo2<I: Foo>(x: I) { let _: Bar = x.boo(); } pub fn baz(x: &Foo<A=Bar>) { let _: Bar = x.boo(); } pub fn main() { let a = 42i32; foo1(a); baz(&a); } ``` #### Multi-line output: ```cmd $ ./rustc test3.rs test3.rs:20:18: 20:25 error: mismatched types: expected `Bar`, found `<I as Foo>::A` (expected struct `Bar`, found associated type) test3.rs:20 let _: Bar = x.boo(); ^~~~~~~ test3.rs:31:5: 31:9 error: type mismatch resolving `<i32 as Foo>::A == Bar`: expected u32, found struct `Bar` test3.rs:31 foo1(a); ^~~~ test3.rs:31:5: 31:9 note: required by `foo1` test3.rs:31 foo1(a); ^~~~ test3.rs:32:9: 32:11 error: type mismatch resolving `<i32 as Foo>::A == Bar`: expected u32, found struct `Bar` test3.rs:32 baz(&a); ^~ test3.rs:32:9: 32:11 note: required for the cast to the object type `Foo` test3.rs:32 baz(&a); ^~ error: aborting due to 3 previous errors ``` --- This is a continuation of #19203 which I apparently broke by force pushing after it was closed. I'm attempting to add multi-line errors where they are largely beneficial - to help differentiate different types in compiler messages. As before, this is still a simple fix. #### Testcase: ```rust struct S; fn test() -> Option<i32> { let s: S; s } fn test2() -> Option<i32> { Ok(7) // Should be Some(7) } impl Iterator for S { type Item = i32; fn next(&mut self) -> Result<i32, i32> { Ok(7) } } fn main(){ test(); test2(); } ``` --- #### Single-line playpen errors: ```cmd <anon>:6:5: 6:6 error: mismatched types: expected `core::option::Option<int>`, found `S` (expected enum core::option::Option, found struct S) <anon>:6 s ^ <anon>:10:5: 10:10 error: mismatched types: expected `core::option::Option<int>`, found `core::result::Result<_, _>` (expected enum core::option::Option, found enum core::result::Result) <anon>:10 Ok(7) // Should be Some(7) ^~~~~ <anon>:14:5: 14:55 error: method `next` has an incompatible type for trait: expected enum core::option::Option, found enum core::result::Result [E0053] <anon>:14 fn next(&mut self) -> Result<uint, uint> { Ok(7) } ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: aborting due to 3 previous errors playpen: application terminated with error code 101 ``` --- #### Multi-line errors: ```cmd $ ./rustc test.rs test.rs:6:5: 6:6 error: mismatched types: expected `core::option::Option<i32>`, found `S` (expected enum `core::option::Option`, found struct `S`) test.rs:6 s ^ test.rs:10:5: 10:10 error: mismatched types: expected `core::option::Option<i32>`, found `core::result::Result<_, _>` (expected enum `core::option::Option`, found enum `core::result::Result`) test.rs:10 Ok(7) // Should be Some(7) ^~~~~ test.rs:15:5: 15:53 error: method `next` has an incompatible type for trait: expected enum `core::option::Option`, found enum `core::result::Result` [E0053] test.rs:15 fn next(&mut self) -> Result<i32, i32> { Ok(7) } ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: aborting due to 3 previous errors ``` --- #### Positive notes * Vim worked fine with it: #19203 (comment) * `make check` didn't find any errors * Fixed *backtick* placement suggested by @p1start at #19203 (comment) #### Negative notes * Didn't check Emacs support but also wasn't provided a testcase... * Needs to be tested with macro errors but I don't have a good testcase yet * I would like to move the `E[0053]` earlier (see #19464 (comment)) but I don't know how * It might be better to indent the types slightly like so (but I don't know how): ```cmd test.rs:6:5: 6:6 error: mismatched types: expected `core::option::Option<int>`, found `S` (expected enum `core::option::Option`, found struct `S`) test.rs:6 s ``` * Deep whitespace indentation may be a bad idea because early wrapping will cause misalignment between lines #### Other * I thought that compiler flags or something else (environment variables maybe) might be required because of comments against it but now that seems too much of a burden for users and for too little gain. * There was concern that it will make large quantities of errors difficult to distinguish but I don't find that an issue. They both look awful and multi-line errors makes the types easier to understand. --- #### Single lined spew: ```cmd $ rustc test2.rs test2.rs:161:9: 170:10 error: method `next` has an incompatible type for trait: expected enum core::option::Option, found enum core::result::Result [E0053] test2.rs:161 fn next(&mut self) -> Result<&'a str, int> { test2.rs:162 self.curr = self.next; test2.rs:163 test2.rs:164 if let (Some(open), Some(close)) = Parens::find_parens(self.all, self.next) { test2.rs:165 self.next = if self.all.char_at(self.next) == '(' { close } test2.rs:166 else { open } ... test2.rs:164:21: 164:31 error: mismatched types: expected `core::result::Result<uint, int>`, found `core::option::Option<_>` (expected enum core::result::Result, found enum core::option::Option) test2.rs:164 if let (Some(open), Some(close)) = Parens::find_parens(self.all, self.next) { ^~~~~~~~~~ test2.rs:164:33: 164:44 error: mismatched types: expected `core::result::Result<uint, int>`, found `core::option::Option<_>` (expected enum core::result::Result, found enum core::option::Option) test2.rs:164 if let (Some(open), Some(close)) = Parens::find_parens(self.all, self.next) { ^~~~~~~~~~~ test2.rs:169:40: 169:76 error: mismatched types: expected `core::result::Result<&'a str, int>`, found `core::option::Option<&str>` (expected enum core::result::Result, found enum core::option::Option) test2.rs:169 if self.curr != self.len { Some(self.all[self.curr..self.next]) } else { None } ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ test2.rs:169:86: 169:90 error: mismatched types: expected `core::result::Result<&'a str, int>`, found `core::option::Option<_>` (expected enum core::result::Result, found enum core::option::Option) test2.rs:169 if self.curr != self.len { Some(self.all[self.curr..self.next]) } else { None } ^~~~ test2.rs:205:14: 205:18 error: mismatched types: expected `core::result::Result<uint, int>`, found `core::option::Option<uint>` (expected enum core::result::Result, found enum core::option::Option) test2.rs:205 (open, close) ^~~~ test2.rs:205:20: 205:25 error: mismatched types: expected `core::result::Result<uint, int>`, found `core::option::Option<uint>` (expected enum core::result::Result, found enum core::option::Option) test2.rs:205 (open, close) ^~~~~ test2.rs:210:21: 210:31 error: mismatched types: expected `core::result::Result<uint, int>`, found `core::option::Option<_>` (expected enum core::result::Result, found enum core::option::Option) test2.rs:210 if let (Some(open), _) = Parens::find_parens(self.all, 0) { ^~~~~~~~~~ test2.rs:210:13: 212:28 error: mismatched types: expected `core::option::Option<&'a int>`, found `core::option::Option<&str>` (expected int, found str) test2.rs:210 if let (Some(open), _) = Parens::find_parens(self.all, 0) { test2.rs:211 Some(self.all[0..open]) test2.rs:212 } else { None } test2.rs:299:48: 299:58 error: mismatched types: expected `Box<translate::Entity>`, found `collections::vec::Vec<_>` (expected box, found struct collections::vec::Vec) test2.rs:299 pub fn new() -> Entity { Entity::Group(Vec::new()) } ^~~~~~~~~~ test2.rs:359:51: 359:58 error: type `&mut Box<translate::Entity>` does not implement any method in scope named `push` test2.rs:359 Entity::Group(ref mut vec) => vec.push(e), ^~~~~~~ test2.rs:366:51: 366:85 error: type `&mut Box<translate::Entity>` does not implement any method in scope named `push` test2.rs:366 Entity::Group(ref mut vec) => vec.push(Entity::Inner(s.to_string())), ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: aborting due to 12 previous errors ``` --- #### Multi-line spew: ```cmd $ ./rustc test2.rs test2.rs:161:9: 170:10 error: method `next` has an incompatible type for trait: expected enum `core::option::Option`, found enum `core::result::Result` [E0053] test2.rs:161 fn next(&mut self) -> Result<&'a str, int> { test2.rs:162 self.curr = self.next; test2.rs:163 test2.rs:164 if let (Some(open), Some(close)) = Parens::find_parens(self.all, self.next) { test2.rs:165 self.next = if self.all.char_at(self.next) == '(' { close } test2.rs:166 else { open } ... test2.rs:164:21: 164:31 error: mismatched types: expected `core::result::Result<uint, int>`, found `core::option::Option<_>` (expected enum `core::result::Result`, found enum `core::option::Option`) test2.rs:164 if let (Some(open), Some(close)) = Parens::find_parens(self.all, self.next) { ^~~~~~~~~~ test2.rs:164:33: 164:44 error: mismatched types: expected `core::result::Result<uint, int>`, found `core::option::Option<_>` (expected enum `core::result::Result`, found enum `core::option::Option`) test2.rs:164 if let (Some(open), Some(close)) = Parens::find_parens(self.all, self.next) { ^~~~~~~~~~~ test2.rs:169:40: 169:76 error: mismatched types: expected `core::result::Result<&'a str, int>`, found `core::option::Option<&str>` (expected enum `core::result::Result`, found enum `core::option::Option`) test2.rs:169 if self.curr != self.len { Some(self.all[self.curr..self.next]) } else { None } ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ test2.rs:169:86: 169:90 error: mismatched types: expected `core::result::Result<&'a str, int>`, found `core::option::Option<_>` (expected enum `core::result::Result`, found enum `core::option::Option`) test2.rs:169 if self.curr != self.len { Some(self.all[self.curr..self.next]) } else { None } ^~~~ test2.rs:205:14: 205:18 error: mismatched types: expected `core::result::Result<uint, int>`, found `core::option::Option<uint>` (expected enum `core::result::Result`, found enum `core::option::Option`) test2.rs:205 (open, close) ^~~~ test2.rs:205:20: 205:25 error: mismatched types: expected `core::result::Result<uint, int>`, found `core::option::Option<uint>` (expected enum `core::result::Result`, found enum `core::option::Option`) test2.rs:205 (open, close) ^~~~~ test2.rs:210:21: 210:31 error: mismatched types: expected `core::result::Result<uint, int>`, found `core::option::Option<_>` (expected enum `core::result::Result`, found enum `core::option::Option`) test2.rs:210 if let (Some(open), _) = Parens::find_parens(self.all, 0) { ^~~~~~~~~~ test2.rs:210:13: 212:28 error: mismatched types: expected `core::option::Option<&'a int>`, found `core::option::Option<&str>` (expected int, found str) test2.rs:210 if let (Some(open), _) = Parens::find_parens(self.all, 0) { test2.rs:211 Some(self.all[0..open]) test2.rs:212 } else { None } test2.rs:229:57: 229:96 error: the trait `core::ops::Fn<(char,), bool>` is not implemented for the type `|char| -> bool` test2.rs:229 .map(|s| s.trim_chars(|c: char| c.is_whitespace())) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ test2.rs:238:46: 239:75 error: type `core::str::CharSplits<'_, |char| -> bool>` does not implement any method in scope named `filter_map` test2.rs:238 .filter_map(|s| if !s.is_empty() { Some(s.trim_chars('\'')) } test2.rs:239 else { None }) test2.rs:237:46: 237:91 error: the trait `core::ops::Fn<(char,), bool>` is not implemented for the type `|char| -> bool` test2.rs:237 let vec: Vec<&str> = value[].split(|c: char| matches!(c, '(' | ')' | ',')) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ test2.rs:238:65: 238:77 error: the type of this value must be known in this context test2.rs:238 .filter_map(|s| if !s.is_empty() { Some(s.trim_chars('\'')) } ^~~~~~~~~~~~ test2.rs:299:48: 299:58 error: mismatched types: expected `Box<translate::Entity>`, found `collections::vec::Vec<_>` (expected box, found struct `collections::vec::Vec`) test2.rs:299 pub fn new() -> Entity { Entity::Group(Vec::new()) } ^~~~~~~~~~ test2.rs:321:36: 322:65 error: type `core::str::CharSplits<'_, |char| -> bool>` does not implement any method in scope named `filter_map` test2.rs:321 .filter_map(|s| if !s.is_empty() { Some(s.trim_chars('\'')) } test2.rs:322 else { None }) test2.rs:320:36: 320:81 error: the trait `core::ops::Fn<(char,), bool>` is not implemented for the type `|char| -> bool` test2.rs:320 let vec: Vec<&str> = s.split(|c: char| matches!(c, '(' | ')' | ',')) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ test2.rs:321:55: 321:67 error: the type of this value must be known in this context test2.rs:321 .filter_map(|s| if !s.is_empty() { Some(s.trim_chars('\'')) } ^~~~~~~~~~~~ test2.rs:359:51: 359:58 error: type `&mut Box<translate::Entity>` does not implement any method in scope named `push` test2.rs:359 Entity::Group(ref mut vec) => vec.push(e), ^~~~~~~ test2.rs:366:51: 366:85 error: type `&mut Box<translate::Entity>` does not implement any method in scope named `push` test2.rs:366 Entity::Group(ref mut vec) => vec.push(Entity::Inner(s.to_string())), ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: aborting due to 24 previous errors ``` Closes #18946 #19464 cc @P1start @jakub- @tomjakubowski @kballard @chris-morgan
Updated 1/12/2014
I updated the multi-line testcase to current but didn't modify the others. The spew code was broke by the
matches!
macro no longer working and I'm not interested in fixing the testcase.I additionally added one testcase below.
Errors will in general look similar to below if the error is either
mismatched types
or a few other types. The rest are ignored.Extra testcase:
Multi-line output:
This is a continuation of #19203 which I apparently broke by force pushing after it was closed. I'm attempting to add multi-line errors where they are largely beneficial - to help differentiate different types in compiler messages. As before, this is still a simple fix.
Testcase:
Single-line playpen errors:
Multi-line errors:
Positive notes
make check
didn't find any errorsNegative notes
E[0053]
earlier (see Collect errors which would be better as multiline errors #19464 (comment)) but I don't know howOther
Single lined spew:
Multi-line spew:
Closes #18946 #19464
cc @P1start @jakub- @tomjakubowski @kballard @chris-morgan