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

Error Message Overhaul #33240

Closed
1 of 10 tasks
nikomatsakis opened this issue Apr 27, 2016 · 53 comments
Closed
1 of 10 tasks

Error Message Overhaul #33240

nikomatsakis opened this issue Apr 27, 2016 · 53 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 27, 2016

PR #32756 began a gradual overhaul of our error message formatting. We would very much like feedback on error messages you think could use improvement, as well of course as any bugs or other problems with the new error formatting code. Please leave comments!

What follows is a list of known issues we plan to address. Likely many of these will be moved into distinct GitHub issues at some point.

Improvements to the overall error reporting mechanism:

  • New unit testing mechanism that can test colors and precise appearance: Add UI testing to compiler test suite #33000
  • Improve the treatment of tabs.
  • Subdiagnostics like note should not be colored green but rather just appear bold
  • Subdiagnostics like note should not use the --> indicator but rather >>>, and the underline should be -- and not ^^
    • basically, they do not have a primary span
  • Mandatory labels on primary spans?
  • Change from >>>> for secondary file names to something like ::: maybe?
  • When snippet labels are too long, potentially move them to a footnote of some kind

Specific error messages that need improvement:

  • Identify when you have the same span for multiple borrows in a loop and give a better message.
  • We special case closure captures due to unique loans, but we could give similar treatment to any loan coming through a capture.
  • Decide whether to thread the closure header span through the builder API convenience methods.
@ticki
Copy link
Contributor

ticki commented Apr 27, 2016

For anyone who want to play around with ideas related to the error formatting, you can use this.

@nikomatsakis
Copy link
Contributor Author

cc @jonathandturner

@ticki
Copy link
Contributor

ticki commented Apr 27, 2016

Could you add the following?

  • Customization of error formatting.
  • Unicode switch.
  • Syntax highlighting.

@sophiajt
Copy link
Contributor

@ticki

Customization of error formatting.

We'd talked some about maybe a "customization of error colors". What do you mean by error formatting?

Unicode switch.

Because we want errors to look and feel similar across platforms, the idea here is to pick a way to display the error that's friendly to even non-unicode environments (like default Windows). We can definitely revisit this one if modern Windows improvements, like Bash on Windows for example, open up the door to using more Unicode.

Syntax highlighting.

Oh right. Yeah, that could look good, depending on how we do it. Would be fun to experiment.

@GuillaumeGomez
Copy link
Member

Syntax highlighting.

I worked on it a while ago for rustc --explain. I came to the conclusion that the "best" way to do it would be through rustdoc. It just needs to add an "ascii" (don't know if it's the correct translation) output format. But for that, we need to rework rustdoc source code in order to easily add new output formats.

Improve the treatment of tabs.

I'm not sure to understand the issue here. Aren't they translated as whitespaces?

@sophiajt
Copy link
Contributor

@GuillaumeGomez the tabs issue is around treating the amount of space we print the tab the same as the amount of space we need to move under the source code to start underlining. The old code had some logic in there to line them up and we just need to port it over.

@GuillaumeGomez
Copy link
Member

Ok, so it's a port issue. Thanks for the explanation @jonathandturner!

@mhintz
Copy link

mhintz commented Apr 27, 2016

One thing that nags me a lot is the amount of space between distinct error messages in the printed console output. Often, because of whitespace in individual error messages, there is more visual whitespace within messages than between distinct ones. This makes it difficult, when scanning printed console output, to visually distinguish different messages, which makes fast visual parsing of the compiler output tricky.

When coupled with the fact that rustc often generates several error messages at once, some of which are follow-ons from one original message appearing at the top of the output, I often find myself scrolling upwards through a column of hard-to-scan text looking for a hard-to-spot relevant message.

A more clearly formatted beginning of the compiler output and clearer delineation between individual error messages would help a lot.

Thanks!

@sophiajt
Copy link
Contributor

@mhintz Do you have an example of the spacing issue you're talking about? Also, is this an issue with the previous error message style or the new message style?

@ticki
Copy link
Contributor

ticki commented Apr 27, 2016

We'd talked some about maybe a "customization of error colors". What do you mean by error formatting?

Well, making it possible to define the escapes introducing the various labels.

Oh right. Yeah, that could look good, depending on how we do it. Would be fun to experiment.

I thought about using a loosly coloured scheme, to avoid confusion with the labels otherwhere.

@ketsuban
Copy link
Contributor

Enum variants are associated items, which has some potentially surprising consequences if you attempt to access them through a type alias (including Self in trait implementations). Whether that's actually desired behaviour or not is #26264, but in #31168 I observed that the error is strikingly unhelpful.

A better error, I think, would be something like error: enum variants cannot be accessed through an alias, perhaps with a note suggesting the appropriate code change since we know the compiler can look up the name of the enum in question.

@ticki
Copy link
Contributor

ticki commented Apr 27, 2016

@ketsuban Wrong thread?

@ketsuban
Copy link
Contributor

No?

We would very much like feedback on error messages you think could use improvement

My issue was about an error message which isn't very clear, so I posted it here to garner attention since this is an issue for bringing attention to unhelpful error messages.

@mhintz
Copy link

mhintz commented Apr 27, 2016

@jonathandturner Just scrolled through the history over at #32756 and it looks like that PR takes care of my objection about spacing, by reformatting and separating nightly's current output. I also hadn't known about the Dybuk formatter, which looks pretty good (maybe even better than the PR).

I assumed it wasn't on the radar, since it hasn't been merged into nightly yet, and I saw the rustlang Twitter account post about it. Glad it's being worked on!

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Apr 27, 2016

  • Customization of error formatting.
  • Unicode switch.

I am sort of undecided about whether we want these. Customization is
a lot of area to stabilize -- we have to select a means of
customization etc, spec it out, support it, etc -- that I would prefer
to avoid, and it seems like a kind of niche request.

  • Syntax highlighting.

But this would be nice.

@nikomatsakis
Copy link
Contributor Author

I think that for syntax highlighting, I'd prefer to just find keywords and highlight them in bold (something very minimal). We have to be very wary of drawing attention away.

@GuillaumeGomez
Copy link
Member

@nikomatsakis: More or less than this?

@durka
Copy link
Contributor

durka commented Apr 27, 2016

Is #31173 covered here?

@danielpclark
Copy link

Wish this thread was open a few weeks ago. I once had an error message give me a rusctc --explain number which in no way pointed to my actual error. It turned out I'd left a semicolon on the end of one of my matchers without an explicit return. I don't remember the exact scenario but I'd assume a mutable variable with a default value was set and the matcher would change it if certain conditions were met and the mutable variable would always be the return value. I apologize that I'm guessing here and this may not have been the issue but it was specifically a semi-colon without an explicit return issue. The error messages in my ~/.bash_history file are E0308, E0507, E0277, E0433, E0080 so one of these only added to my confusion.

@nikomatsakis
Copy link
Contributor Author

Earlier I wrote:

Customization is a lot of area to stabilize -- we have to select a means of customization etc, spec it out, support it, etc -- that I would prefer to avoid, and it seems like a kind of niche request.

Ever since I wrote this, the wording I used has been bugging me. Even though I still more-or-less feel the same, I feel like saying this is a "niche" request sounds dismissive, which I did not intend. Let me just clarify that what I meant is that I suspect that it's a feature that few would take advantage of, and I'd be happiest if we can just find one output that everyone is relatively satisfied with.

bors added a commit that referenced this issue Apr 30, 2016
Overhaul borrowck error messages and compiler error formatting generally

This is a major overhaul of how the compiler reports errors. The primary goal is to be able to give many spans within the same overall context, such as this:

```
./borrow-errors.rs:73:17: 73:20: error: cannot borrow `*vec` as immutable because previous closure requires unique access [E0501]
70     let append = |e| {
                    ~~~ closure construction occurs here
71         vec.push(e)
           ~~~ previous borrow occurs due to use of `vec` in closure
72     };
73     let data = &vec[3];
                   ~~~ borrow occurs here
74 }
   ~ borrow from closure ends here
```

However, in the process we made a number of other changes:

- Removed the repetitive filenames from snippets and just give the line number.
- Color the line numbers blue so they "fade away"
- Remove the file name and line number from the error code suggestions since they don't seem to fit anymore. (This should probably happen in more places, like existing notes.)
- Newlines in between errors to help group them better.

This PR is not quite ready to land, but we thought it made sense to stop here and get some feedback from people at large. It'd be great if people can check out the branch and play with it. We'd be especially interested in hearing about cases that don't look good with the new formatting (I suspect they exist).

Here is a checklist of some pending work items for this PR. Some of them may be best left for follow-up PRs:

- [x] Accommodate multiple files in a `MultiSpan` (this should be easy)
  - In this case, we want to print filenames though.
- [x] Remove duplicate E0500 code.
- [x] Make the header message bold, rather than current hack that makes all errors/warnings bold
- [x] Update warning text color (yellow is hard to read w/ a white background)

Moved numerous follow-ups to: #33240

Joint work with @jonathandturner.

Fixes #3533
bors added a commit that referenced this issue Apr 30, 2016
Overhaul borrowck error messages and compiler error formatting generally

This is a major overhaul of how the compiler reports errors. The primary goal is to be able to give many spans within the same overall context, such as this:

```
./borrow-errors.rs:73:17: 73:20: error: cannot borrow `*vec` as immutable because previous closure requires unique access [E0501]
70     let append = |e| {
                    ~~~ closure construction occurs here
71         vec.push(e)
           ~~~ previous borrow occurs due to use of `vec` in closure
72     };
73     let data = &vec[3];
                   ~~~ borrow occurs here
74 }
   ~ borrow from closure ends here
```

However, in the process we made a number of other changes:

- Removed the repetitive filenames from snippets and just give the line number.
- Color the line numbers blue so they "fade away"
- Remove the file name and line number from the error code suggestions since they don't seem to fit anymore. (This should probably happen in more places, like existing notes.)
- Newlines in between errors to help group them better.

This PR is not quite ready to land, but we thought it made sense to stop here and get some feedback from people at large. It'd be great if people can check out the branch and play with it. We'd be especially interested in hearing about cases that don't look good with the new formatting (I suspect they exist).

Here is a checklist of some pending work items for this PR. Some of them may be best left for follow-up PRs:

- [x] Accommodate multiple files in a `MultiSpan` (this should be easy)
  - In this case, we want to print filenames though.
- [x] Remove duplicate E0500 code.
- [x] Make the header message bold, rather than current hack that makes all errors/warnings bold
- [x] Update warning text color (yellow is hard to read w/ a white background)

Moved numerous follow-ups to: #33240

Joint work with @jonathandturner.

Fixes #3533
eddyb added a commit to eddyb/rust that referenced this issue May 13, 2016
bors added a commit that referenced this issue May 13, 2016
@rrichardson
Copy link

Someone posted this into #rust and we solved the problem, but still pondered over the error:

Nightly on playpen, but same result (different formatting) on 1.8 or beta.

use std::io;
use std::sync::{Arc, RwLock};
use std::thread::{Builder as ThreadBuilder, JoinHandle};

fn main() {
    let data = vec!["I am data!".to_string()];
    let data_lock = Arc::new(RwLock::new(data));

    thread(data_lock.clone())
        .map(|t| t.join().expect("Thread failed"))
        .expect("Thread creation failed");
}

fn thread(lock: Arc<RwLock<Vec<String>>>) -> io::Result<JoinHandle<()>>  {
    let thread_lock = lock.clone();
    ThreadBuilder::new()
        .spawn(move || {
            match thread_lock.read() { // line 20
                Ok(data) => {
                    data.iter()
                        .map(|s| println!("{}", s))
                        .collect::<Vec<_>>();
                },
                _ => (),
            } // line 26
        })
}
error: `thread_lock` does not live long enough
  --> <anon>:20:19
20 |>             match thread_lock.read() {
   |>                   ^^^^^^^^^^^
note: reference must be valid for the destruction scope surrounding block at 18:23...
  --> <anon>:18:24
18 |>         .spawn(move || {
   |>                        ^
note: ...but borrowed value is only valid for the block at 18:23
  --> <anon>:18:24
18 |>         .spawn(move || {
   |>                        ^

This is solved by adding a semicolon to the end of the match block on line 26.

I'm not entirely sure what's going on here, I am guessing that the semicolon ensures that the result of the match doesn't try to be returned from the scope of the closure(?) even though the result of the match doesn't contain anything from thread_lock, it is ().

@nikomatsakis
Copy link
Contributor Author

@rrichardson that problem sounds lik it is due to the temporary lifetimes; in this case the match is the trailing unit in the block. See e.g. (closed) #22252 -- I feel like there remains an issue open on trying to address this in some nicer way but I've not found it just now. In any case, clearly the error message it can be better: it'd be nice to see if we can revise the rules in some way to make this not an error in the first place, but I'm not sure if that is possible.

@steveklabnik steveklabnik added the A-diagnostics Area: Messages for errors, warnings, and lints label Jul 25, 2016
@retep998
Copy link
Member

retep998 commented Aug 30, 2016

Oh good, now even more stuff is using the difficult to read blue text. (Also the help text isn't formatted in bright cyan the way help messages are supposed to be)

Come on, good defaults are really important!

Also, the Windows Console doesn't support bold, so I'd really appreciate it if instead you simply used white. Note that regular text is light gray by default, not white, so white would be the perfect choice here, making the text brighter and stand out, just like bold would.

Even with good defaults, I'd still really appreciate it if rustc had a way for me to choose which colors it uses for which thing. That way I can choose a nice palette of colors in my console and then tell rustc to use those colors.

@GuillaumeGomez
Copy link
Member

Using some env vars to define colors sounds like a good idea. :D

@sophiajt
Copy link
Contributor

@retep998 - agreed good defaults are important, which is why we took so long to stabilize and work on what the format should look like. tbh there wasn't a ton of feedback on what it looks like on Windows.

The colors are easy enough to change. We could potentially detect Windows and switch to something like:

cyan_secondary

Btw, here's what bright white looks like. I'd be afraid it would blend in too much with the code:

white_secondary

@retep998
Copy link
Member

retep998 commented Aug 31, 2016

@jonathandturner I'm saying to use bright white instead of bold, not instead of blue. If you look at images from other platforms you'll notice how certain text is bold. In conhost that text is not bold because there is no support for bold. Instead of making that text bold, make it bright white on Windows which achieves a similar effect.

@GuillaumeGomez Sure, anything is better than the status quo.

@sophiajt
Copy link
Contributor

@retep998 - I think you might have something in mind, though I'm not entirely following what you're saying. All blue text is bold text.

I'm certainly amenable to helping here. For example, like I said, we can detect Windows and make changes to the defaults.

@GuillaumeGomez Sure, anything is better than the status quo.

I think the best way to help "the status quo" is to help us create a solution that works for Windows.

@retep998
Copy link
Member

@jonathandturner Look at this screenshot someone posted earlier from a non-windows platform:

Notice how some of the white text is bold? That bold text is being generated by

try!(self.start_attr(term::Attr::Bold));
. Notice how it merely specifies bold and does not specify a color. As a result, because conhost doesn't support bold, that text does not stand out at all in any way. Making that text bright white would be an improvement.

As for a replacement for the "bright" blue currently being used for line numbers and stuff, cyan is the closest alternative there is. Unfortunately bright cyan is being used for help messages, but there's not many choices in the default console color palette that aren't too dark.

Also, the color for warnings really needs to be bright yellow on Windows. Dark yellow is just bleh. Specifically this bit of code https://github.com/rust-lang/rust/blob/master/src/librustc_errors/lib.rs#L735

sophiajt pushed a commit to sophiajt/rust that referenced this issue Sep 2, 2016
…ikomatsakis

Special case a few colors for Windows

As brought up on [this thread](rust-lang#33240 (comment)) the colors used in error messages on Windows can be difficult to read because of the lack of bold.

This PR makes a few changes to improve readability, namely:
* Rather than using BRIGHT_BLUE, on Windows we now use BRIGHT_CYAN, which is easier to read on black when you do not have bold
* We used BRIGHT_YELLOW rather than YELLOW, for the same reason
* Titles will be BRIGHT_WHITE now, to give the illusion of being bold

Some examples:

![warning](https://cloud.githubusercontent.com/assets/547158/18148466/9aa9bbe2-6f8e-11e6-927f-d0eec53cac32.PNG)

![error](https://cloud.githubusercontent.com/assets/547158/18148488/ba9fb186-6f8e-11e6-8d8e-e93d569f61de.PNG)

r? @nikomatsakis

cc @retep998
sophiajt pushed a commit to sophiajt/rust that referenced this issue Sep 2, 2016
…ikomatsakis

Special case a few colors for Windows

As brought up on [this thread](rust-lang#33240 (comment)) the colors used in error messages on Windows can be difficult to read because of the lack of bold.

This PR makes a few changes to improve readability, namely:
* Rather than using BRIGHT_BLUE, on Windows we now use BRIGHT_CYAN, which is easier to read on black when you do not have bold
* We used BRIGHT_YELLOW rather than YELLOW, for the same reason
* Titles will be BRIGHT_WHITE now, to give the illusion of being bold

Some examples:

![warning](https://cloud.githubusercontent.com/assets/547158/18148466/9aa9bbe2-6f8e-11e6-927f-d0eec53cac32.PNG)

![error](https://cloud.githubusercontent.com/assets/547158/18148488/ba9fb186-6f8e-11e6-8d8e-e93d569f61de.PNG)

r? @nikomatsakis

cc @retep998
sophiajt pushed a commit to sophiajt/rust that referenced this issue Sep 2, 2016
…ikomatsakis

Special case a few colors for Windows

As brought up on [this thread](rust-lang#33240 (comment)) the colors used in error messages on Windows can be difficult to read because of the lack of bold.

This PR makes a few changes to improve readability, namely:
* Rather than using BRIGHT_BLUE, on Windows we now use BRIGHT_CYAN, which is easier to read on black when you do not have bold
* We used BRIGHT_YELLOW rather than YELLOW, for the same reason
* Titles will be BRIGHT_WHITE now, to give the illusion of being bold

Some examples:

![warning](https://cloud.githubusercontent.com/assets/547158/18148466/9aa9bbe2-6f8e-11e6-927f-d0eec53cac32.PNG)

![error](https://cloud.githubusercontent.com/assets/547158/18148488/ba9fb186-6f8e-11e6-8d8e-e93d569f61de.PNG)

r? @nikomatsakis

cc @retep998
@estebank
Copy link
Contributor

Re: syntax highlighting, using rustdoc's syntax highlighter after a small refactor (internals discussion):

screen shot 2017-01-23 at 10 54 42

@sophiajt
Copy link
Contributor

@estebank - I like it 👍 Have you tried it with a white background?

@estebank
Copy link
Contributor

@jonathandturner without any customization (just changed the profile in my term):

@sophiajt
Copy link
Contributor

@estebank - yeah, that yellow isn't quite strong enough for white. In the compiler, albeit a bit hacky, we do this:

                if cfg!(windows) {
                    term::color::BRIGHT_YELLOW
                } else {
                    term::color::YELLOW
                }

@sophiajt
Copy link
Contributor

However you end up doing it, having something that works well on black and white backgrounds might be a good way of picking defaults.

@estebank
Copy link
Contributor

Agree.

What would you think of using (and avoiding abusing) a set background for specific tokens?

@sophiajt
Copy link
Contributor

sophiajt commented Jan 23, 2017

@estebank - let's move this discussion to a separate thread because lots of people are following this one.

Do you have a PR we can move it to?

@adamransom
Copy link
Contributor

adamransom commented Feb 17, 2017

I'm wondering if it's possible to only use the base colours, instead of the bright versions, especially when there is heavy use of bold at the same time. Most terminals allow an option for setting whether bold text appears in bright colours.

Also, some colour schemes (like Solarized) make heavy use of the bright colours for subtle background shades and this may cause the error output to look odd. Of course, full colour customisation is preferable for these types of cases.

@nikomatsakis
Copy link
Contributor Author

I feel like this bug has outlived its usefulness. I'm going to close it.

bors added a commit that referenced this issue May 7, 2017
Minor cleanup of UX guidelines.

I think this fixes #34808. It covers the [long error code explanations normalization] by linking to the RFC, and cleaning up the list where long diagnostics are defined. While the [error message overhaul] isn't covered directly, I'm not really sure that more than the [existing section] on the error/warning/help messages is warranted; the overhaul linked didn't really specify any new guidelines, primarily just changing the output format.

[Long error code explanations normalization]: https://github.com/rust-lang/rfcs/blob/master/text/1567-long-error-codes-explanation-normalization.md
[Error message overhaul]: #33240
[existing section]: https://github.com/rust-lang/rust/blob/master/src/doc/rustc-ux-guidelines.md#error-warning-help-note-messages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints
Projects
None yet
Development

No branches or pull requests