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

Improve extended errors (rustc --explain). #24143

Merged
merged 2 commits into from
Apr 8, 2015
Merged

Improve extended errors (rustc --explain). #24143

merged 2 commits into from
Apr 8, 2015

Conversation

michaelsproul
Copy link
Contributor

I've taken another look at extended errors - fixing up the printing and adding a few more for match expressions.

With regards to printing, the previous behaviour was to just print the error message string directly, despite it containing indentation which caused it to overflow the standard terminal width of 80 columns (try rustc --explain E0004). The first approach I considered was to strip the leading whitespace from each line and lay out the text dynamically, inserting spaces in between. This approach became quite messy when taking multi-paragraph errors into account (and seemed overkill). The approach I settled on removes the indentation in the string itself and begins each message with a newline that is stripped before printing.

I feel like complete extended errors would be nice to have for 1.0.0 and I'm happy to spearhead an effort to get them written. Brian got me onto writing them at an SF meetup and I think it shouldn't be too hard to get the remaining 80 or so written with the help of people who don't really work on compiler innards.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

the input to a match expression. To match against NaN values, you should instead use
the `is_nan` method in a guard, as in: x if x.is_nan() => ...
E0003: r##"
Not-a-Number (NaN) values can not be compared for equality and hence can never
Copy link
Contributor

Choose a reason for hiding this comment

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

s/can not/cannot/

@pnkfelix
Copy link
Member

pnkfelix commented Apr 7, 2015

@michaelsproul Is this commit just changing the code formatting and where the line breaks fall? (Or at least intended to solely to change that?)

Or are there changing to the content mixed in?

Update: Well, clearly ad2e506 has changes to the contents. I guess my question above is solely about commit 19e9828

@michaelsproul
Copy link
Contributor Author

@pnkfelix: Yeah 19e9828 doesn't change the content at all, it just reflows it in some places.

@pnkfelix
Copy link
Member

pnkfelix commented Apr 7, 2015

r=me after the example in ad2e506 is fixed in some manner (see my earlier comment).

@michaelsproul
Copy link
Contributor Author

Fixed the &Option<u32> thing and adopted the "cannot"s, although I managed to lose your comment in the process of force pushing (whoops).

Thanks for reviewing 😄

@pnkfelix
Copy link
Member

pnkfelix commented Apr 7, 2015

@michaelsproul the point from my earlier comment was that i was not sure whether the provided suggested revision was quite right ... although now I see that you have addressed the most important point (that op_num changed its type in the revision), so I guess you probably did see the original comment.

So, okay.

@pnkfelix
Copy link
Member

pnkfelix commented Apr 7, 2015

@bors r+ 039a553 rollup

@michaelsproul
Copy link
Contributor Author

@pnkfelix: Yeah, I think the use of a Copy type makes the op_num example less clear than it could be. Perhaps something like this would serve us better:

// Before.
match Some("hi".to_string()) {
    ref op_string_ref @ Some(ref s) => ...
    None => ...
}

// After.
match Some("hi".to_string()) {
    Some(ref s) => {
        let opt_string_ref = &Some(&s);
        ...
    }
    None => ...
}

This makes a bit more sense, with op_string_ref : &Option<&String>.

If you like this change I could push it and we could approve the new commit before bors jumps on the old one (or it could wait for the next PR).

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Apr 8, 2015
…kfelix

I've taken another look at extended errors - fixing up the printing and adding a few more for match expressions.

With regards to printing, the previous behaviour was to just print the error message string directly, despite it containing indentation which caused it to overflow the standard terminal width of 80 columns (try `rustc --explain E0004`). The first approach I considered was to strip the leading whitespace from each line and lay out the text dynamically, inserting spaces in between. This approach became quite messy when taking multi-paragraph errors into account (and seemed overkill). The approach I settled on removes the indentation in the string itself and begins each message with a newline that is stripped before printing.

I feel like complete extended errors would be nice to have for 1.0.0 and I'm happy to spearhead an effort to get them written. Brian got me onto writing them at an SF meetup and I think it shouldn't be too hard to get the remaining 80 or so written with the help of people who don't really work on compiler innards.
bors added a commit that referenced this pull request Apr 8, 2015
@bors bors merged commit 039a553 into rust-lang:master Apr 8, 2015
@brson
Copy link
Contributor

brson commented Apr 13, 2015

Awesome! This is a really important polish area for 1.0. Anything you can do to stir up more support for this effort would be great.

Manishearth added a commit to Manishearth/rust that referenced this pull request Apr 17, 2015
 I've updated the diagnostic registration plugin so that it validates error descriptions. An error description is only valid if it starts and ends with a newline, and contains no more than 80 characters per line.

The plugin forced me to fix E0005 and E0006 which had escaped manual attention!

I've also added errors for E0267, E0268, E0296, whilst updating E0303 as per discussion in rust-lang#24143.

cc rust-lang#24407
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.

6 participants