Overhaul borrowck error messages and compiler error formatting generally #32756

Merged
merged 40 commits into from May 3, 2016

Projects

None yet
@nikomatsakis
Contributor
nikomatsakis commented Apr 5, 2016 edited

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:

  • Accommodate multiple files in a MultiSpan (this should be easy)
    • In this case, we want to print filenames though.
  • Remove duplicate E0500 code.
  • Make the header message bold, rather than current hack that makes all errors/warnings bold
  • 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

@nrc nrc was assigned by rust-highfive Apr 5, 2016
@rust-highfive
Collaborator

r? @nrc

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

@nikomatsakis
Contributor

Here is some before-and-after. We should do more shots, e.g. with dark background.

Before:

Before

After:

After

@nikomatsakis
Contributor

Another before-and-after pair, this time on a dark background:

Before:

Before

After:

After

@alexcrichton
Member

I'm super excited about this! Those snippets are really looking great to me.

One point is that in the past we've had mixed experience with lots of words colored differently than the default. In Cargo we recently switched from errors as all-red to just the word "error" in red. In the snippets you've pasted, this just applies to:

  • The entire filename is printed in red for the error, whereas before it was in the normal color and just "error" was in red. I personally prefer having as little in color as possible, but maybe the filename is contained enough here though?
  • The help messages for the borrows here at least are all in blue, which may not look the best on all terminals. That being said I like how it's distinct from both the error message itself as well as the code being printed.

Altogether I guess I'm just saying we probably shouldn't go too crazy in terms of colorizing words. I might revert back to today's behavior for the filename + error indicator, but the help messages I'd probably leave pretty similar to the way they are in your snippets.

@nikomatsakis
Contributor

Oh, cc @brson, with whom I've talked about this a plenty.

@brson
Contributor
brson commented Apr 6, 2016

Looks better to me. A big change but it'll probably just take time to adapt.

  • Things look kind of weird with the line numbers not lining up. Maybe I'll learn to like it. I understand it gives us more room.
  • Although I understand it's how emacs does it, putting the red on the first mention of the file/line number looks weird. When we talked about it it made a lot of sense when emacs does it, but then there was the 'wall of line numbers' where the first one was red, then the others were dark, fadded out. Here you've gone further and removed all the non-primary line information, which lessens the need to visually distinguish them. This colorization emphasizes the error range, instead of the word error. Probably better.
  • Removing the color from the help label makes it less distinct.
  • You have a blank line in your output. That works fine here when there are many lines per error, but will look very bad for a sequence of one-line errors.
@durka
Contributor
durka commented Apr 6, 2016

I noticed the carrots ^~ are gone. Not a big deal, but it did make it clear the span was on the line above, rather than the line below (but it's usually clear anyway). I guess you could just replace all the ~ with ^.

@killercup killercup referenced this pull request in ticki/dybuk Apr 6, 2016
Open

Rustc will offer JSON error logging #2

@killercup
Contributor

Very nice.

  • I really like that the filename is only given once. (Having up to 100 chars per line of code plus the file name meant I had to maximise the terminal pane showing rustc ouput on my laptop, which is annoying.)

  • Am I right in assuming the filename will be printed in yellow for warnings? That might be difficult to read on some terminals/editors with bright backgrounds.

  • I'm not sure why line numbers, the underline, and the help message are bold when they already have a different color.

  • Do we really want to print "help: run rustc --explain E0001…" every time? But I guess this is a discussion for another day.

  • Using ~ for underlines looks weird to me, because it's a character that's not at the top of the line. I think ^ looks better. To get really crazy: Why not add a unicode mode and do something like this? 😄

    const MAX_RANGE_BITS: u32 = 24;
    ⌞⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⌟
    
@ticki
Contributor
ticki commented Apr 6, 2016

This is very cool. It will essentially make dybuk outdated.

@ticki
Contributor
ticki commented Apr 6, 2016

On a seperate note, it should be possible to configure the style of the error messages.

@nikomatsakis
Contributor

@brson

You have a blank line in your output. That works fine here when there are many lines per error, but will look very bad for a sequence of one-line errors.

I was wondering about this. Do we have one-line errors? Most errors include at least a span, and usually more text.

@nikomatsakis
Contributor

@brson @alexcrichton

I'm torn about the colors on the filename. I agree that coloring the filename red doesn't look as good as it does in emacs, and I also agree it is less necessary if we remove the redundant filenames.

For reference, here is an image comparing what I see in emacs (which applies its own colors) vs what the compiler does today http://imgur.com/GZKJ4Pg. Somehow the emacs one looks nice to me, but the imitation thereof in this PR looks a little plodding. It might be that the column numbers are highlighted differently. At the same time, the emacs column numbers have quite a lot of colors, maybe a bit more than is needed. In any case, @brson's observions, combined with @alexcrichton's suggestion that we minimize color, are making think that perhaps we can find a new balance.

That said, the current colors arose out of a lot of experimentation -- it's surprisingly hard to find a combination that looks good and achieves a wide variety of goals:

  • clearly separating the labels from the user's source
  • directing the eye to the error message
  • indicating the kind of error
  • not making you want to claw your eyes out

Of course, you might not agree that the current colors achieve that balance. In any case, I don't consider the current choices optimal, and I definitely think there is room to play around more here.

(Does anyone know a good "ANSI editor", btw? It'd be helpful to be able to do mock-ups. I guess I can do it in emacs, maybe. Or maybe just write some custom program using term directly. The turnaround for building the compiler is kind of brutal here if all you are doing is seeing what happens when you make a label bold.)

@nikomatsakis
Contributor

Also, can we rely on unicode being available? Or auto-detect it? I don't want to have too many modes...but using unicode box-drawing characters would be cool. I remember @kmcallister (I think) had a PR on this that never quite landed.

@ticki
Contributor
ticki commented Apr 6, 2016

For comparison

Dybuk:

This PR:

@killercup
Contributor

I would be surprised if you could detect unicode support, @nikomatsakis, as it depends on the font being used (or available fallbacks, I guess.)

@ticki
Contributor
ticki commented Apr 6, 2016

Why not just have a variable, RUST_ERROR_UNICODE, to opt-in Unicode?

@GuillaumeGomez
Member

That's awesome! However, like others already said, why not replacing "~" by "^"?

Also, why not setting a different color for error lines? I think that having the filename with its own and unique color would improve the display and help to understand what we're reading more quickly.

@nikomatsakis
Contributor

@Ticki

Why not just have a variable, RUST_ERROR_UNICODE, to opt-in Unicode?

I'd much rather make it opt-out. I think the real question is what scheme we want to use for controlling rustc's "skin". We have some compile flags for using colors, which is an obvious precedent, but also doesn't seem that great; this is something that a user would want to decide, and probably uniformly. Maybe we should look for ~/.rustc.toml or something instead.

@nikomatsakis
Contributor

@Ticki yeah, I was thinking that maybe the thing to do is to put the error underneath the filename/line-number. In general I think it's probably a win to use more vertical space when formatting errors.

As long as emacs can still take me to the next error, I'm happy with that. =)

@frewsxcv
Member
frewsxcv commented Apr 6, 2016

I remember @kmcallister (I think) had a PR on this that never quite landed.

For reference, here's the pull request: #21406.

EDIT: And the follow-up issue: #24387.

EDITEDIT: Another relevant issue: #28902.

@nagisa
Contributor
nagisa commented Apr 6, 2016

I like the general appearance, here’s some notes:

Why not just have a variable, RUST_ERROR_UNICODE, to opt-in Unicode?

I'd much rather make it opt-out

The box drawing symbols exist since Unicode 1.1. Rust does not support systems with non-utf-8 locales anyway… the only problem is the Windows’ cmd which uses whatever weird codepages they happen to know and even weirder fonts which might not even support latin.

The cmd doesn’t use ANSI and thus must be reliably detected and worked around anyway, so…


That said, the current colors arose out of a lot of experimentation -- it's surprisingly hard to find a combination that looks good and achieves a wide variety of goals

Axe-actly! In my experience barely any default and non-trivial colouring works well with solarized, so we should probably either restrict ourselves to the most barebones escape codes and colours like colours 1-to-6 and inverse or allow configuration of them.


Solarized

looks hard to figure out which spans relate to which messages. What happens when these spans overlap?


Remove duplicate E0500 code.

Using struct_span_err_with_code method directly would help. Would become stringly typed though and otherwise opaque to the lint.

@nikomatsakis
Contributor

What happens when these spans overlap?

You're looking at it. The two labels there are attached to the same span.

@steveklabnik
Contributor

One more inevitable thing is that a lot of docs are going to be showing old errors. Not a showstopper for this PR, mind you, but something that needs to be considered to some degree.

@nagisa
Contributor
nagisa commented Apr 6, 2016

@nikomatsakis

You're looking at it. The two labels there are attached to the same span.

To me it looks like that’s two notes attached to two adjacent spans of length 1. One span on v and another on ). That’s not overlapping. By overlapping I mean something like

code code code
^^^^^^^^^ span1
     ~~~~~~~~~ span2
@aturon
Contributor
aturon commented Apr 6, 2016

@steveklabnik

One more inevitable thing is that a lot of docs are going to be showing old errors. Not a showstopper for this PR, mind you, but something that needs to be considered to some degree.

I wonder if there's any way to automate the inclusion of errors.

@nikomatsakis
Contributor

@nagisa

To me it looks like that’s two notes attached to two adjacent spans of length 1. One span on v and another on ). That’s not overlapping. By overlapping I mean something like

Yeah, that's actually true in this particular case, I didn't look closely enough. In any case, what the PR currently does is to union all ~. That said, one of the changes I wanted to consider was making more than one line of ~ for overlapping spans. In this version, we erred on the side of conciseness over precision, which might not be the right trade-off.

@nikomatsakis
Contributor

So what do people think about using unicode except for when running on windows, since apparently cmd doesn't support unicode?

@nagisa
Contributor
nagisa commented Apr 6, 2016

(There’s SetConsoleOutputCP which allows setting codepage to the UTF-8 compatible one, but there’s still an issue with cmd not doing font fallback)

@msiemens
Contributor
msiemens commented Apr 6, 2016

using unicode except for when running on windows, since apparently cmd doesn't support unicode

It's more complicated than that if you consider alternative shells on Windows like Msys2 Shell or ConEmu. For the latter the docs state:

ConEmu is a unicode application.

That means ConEmu is able to show unicode (e.g. UTF-8 or UTF-16) if the console application (cmd, java, perl, powershell, bash and so on) can produce unicode output.

(Source)

@jonathandturner
Contributor

@Ticki - I hadn't seen dybuk before but you got us brainstorming a revision that incorporates some of the features. It's got great readability-at-a-glance.

@bors
Contributor
bors commented Apr 6, 2016

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

@devnought

@msiemens Don't forget PowerShell!

@alexcrichton
Member

So what do people think about using unicode except for when running on windows, since apparently cmd doesn't support unicode?

I've found unicode on Unix at least to almost always work, or at least I've yet to see a bug report anywhere about something not working. That being said, none of our tools leverage unicode just yet, so it could perhaps simply be an unexplored space.

I also fear a little making the console story worse on Windows than it already is. We already don't colorize messages on MSYS terminals by default, and so far testing out unicode in a few terminals like cmd and powershell the story isn't so hot there either.

I guess my thoughts here are:

  • I like the idea of using unicode where possible, especially if it helps the readability of error messages.
  • If we don't use unicode some places, though, like some Windows environments, it's easy for these situations to get neglected and the errors get worse and worse over time (as there's two methods of printing errors)
  • Windows is already at a bit of a disadvantage (at least with MSYS terminals) so if we continue to widen the gap with Unix it may be a little unfortunate
@nrc
Contributor
nrc commented Apr 6, 2016

I'm against using unicode (though not strongly). We should strive to make the console output as universal as possible. I don't think unicode adds that much - maybe a little prettier, but not really more informative. I would rather be a little bit uglier but have 100% coverage. If we want pretty, we should be using something graphical - an IDE or the browser or some other GUI.

@DanielKeep
Contributor

To make this a little more concrete, here is Niko's first example with the underlines replaced with box-drawing characters and rendered on Windows.

To reproduce, just put the modified example in an UTF-8 encoded file, run chcp 65001 from cmd, then type boxdraw.txt. Rust uses the Unicode APIs to write text to the console, so encoding should not be a problem in and of itself.

Using "Raster Fonts":

Using "Consolas":

Using "Lucida Console":

Those are the only three fonts I can choose from. That said, I typically use ConEmu; here's what the text looks like using Source Code Pro:

The one thing I've learned from this is that box-drawing characters look pretty meh, all things considered. I also love how, depending on the font, the closure looks like it's an e inside a bag or something. There are other box-drawing characters, but support is bad (I don't know what the deal with U+254D is):

@Veedrac
Contributor
Veedrac commented Apr 7, 2016

This looks amazing. This one is confusing though:

34     (&mut v, &mut v);
             ~       ~~ first borrow ends here
             |       |
             |       second mutable borrow occurs here
             first mutable borrow occurs here

How about just

34     (&mut v, &mut v);
                      ~ first borrow ends here
                     ~ second mutable borrow occurs here
             ~ first mutable borrow occurs here

(± unicode)

@lambda
Contributor
lambda commented Apr 7, 2016

Ooh, what a pretty bikeshed to paint!

With that in mind, I really like the new layout, especially getting rid of filenames/line numbers on every line. I appreciate keeping them on the line with the main error for good Emacs (and presumably other editor) support. The new layout is a huge win in readability.

I feel the Unicode suggestion is "meh" and probably not worth the hassle dealing with font support, support for disabling, and Windows support; and it would be entirely appropriate as a separate patch series if someone did that.

If we did go with Unicode, I don't think you need the corners on the ends of each span to make it work; I think the following looks a little better:

./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

I'm not a big fan of the new color scheme. The colors seem a bit much. I think I prefer just having the error or warning highlighted, rather than the filename. And I don't think there's a good reason to highlight the span messages; just having them be bright, like the error message, would add enough contrast. It also feels like coloring the line numbers makes them stand out more rather than fade into the background.

I do miss the ^ on spans. Without it, the underline feels a bit too far down. The Unicode characters do show up a little higher, at least in the font I use, so don't necessarily need it.

I don't mind the spacing between the line numbers, though I can see how it would feel a little sparse. If you wanted to help tie them together a little, this might work and suggest vertical ellipses (or just use vertical ellipses in Unicode):

./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
@durka
Contributor
durka commented Apr 7, 2016

Or even

             ▼ first mutable borrow occurs here
                      ▼ first borrow ends here
34     (&mut v, &mut v);
                     ▲ second mutable borrow occurs here

...but it doesn't scale to more spans.

@zvxy
zvxy commented Apr 7, 2016

Will this also be done for unused_imports?

src\main.rs:8:5: 8:13 warning: unused import, #[warn(unused_imports)] on by default  
src\main.rs:8 use std::fmt;                                                          
                  ^~~~~~~~                                                           
src\main.rs:9:24: 9:31 warning: unused import, #[warn(unused_imports)] on by default 
src\main.rs:9 use std::collections::{HashSet, HashMap};                              
                                     ^~~~~~~                                         
src\main.rs:9:33: 9:40 warning: unused import, #[warn(unused_imports)] on by default 
src\main.rs:9 use std::collections::{HashSet, HashMap};                              
                                              ^~~~~~~                                

When I'm working on libraries my console always gets flooded with warnings unless I disable unused_imports.

Perhaps there's a better way to display these warning messages? For example, instead of showing the source it can just display a list of modules that were imported (for each file) but not used.

warning: unused imports, #[warn(unused_imports)] on by default  
src\main.rs:8 std::fmt                                                
src\main.rs:9 std::collections::{HashSet, HashMap}

or if HashSet is used but HashMap is not:

warning: unused imports, #[warn(unused_imports)] on by default  
src\main.rs:8 std::fmt                                                
src\main.rs:9 std::collections::HashMap

Editors should be able to take the linecode for each and do a lookup for linting so I don't think it'd be a big deal to not have the Span. Or if you want to condense it even further by concatenating the missing modules to one line per file:

warning: unused imports, #[warn(unused_imports)] on by default  
src\main.rs: std::fmt (8), std::collections::HashMap (9)
src\other.rs: std::collections::VecDeque (6)

What about the other warning messages?

unused_assignments, unused_variables, unused_mut, unreachable_code

@bstrie
Contributor
bstrie commented Apr 7, 2016

I think for now we should ignore Unicode. We're always going to need to have a non-Unicode option available anyway, so in the interest of reducing the surface area of this bikeshed let's save this debate for another time.

@dan-t
dan-t commented Apr 7, 2016

Looks really nice!

Regarding the coloring, I also seem to prefer the coloring on the error/warning/help/note/...
instead of the file name. It's somehow easier to associate the meaning with the color
and if there's less coloring, the colored parts stick more out.

@matthieu-m

Beware about coloring: I am myself partially color-blind, and I have worked with people having a complete color blindness (the guy would see red as black, so that red-on-black was just plain black to him).

Thus, given that:

  • allowing complete customization of colors is probably not worth it
  • brightness/boldness DOES help regardless of colors

I would suggest an option to disable coloring WITHOUT also disabling brightness/boldness, so that color-blind people who have difficulty with the selected scheme can still benefit from a "rich" output.

@nikomatsakis
Contributor

@matthieu-m

I would suggest an option to disable coloring WITHOUT also disabling brightness/boldness, so that color-blind people who have difficulty with the selected scheme can still benefit from a "rich" output.

I quite like this suggestion, but I think I would prefer to move that kind of control into a separate PR (maybe even an RFC). It seems to me that it'd be worth thinking a bit about how precisely we want to support "skinning" rustc output -- e.g., I would imagine that a ~/.rustc config file would be better than forcing people to add command-line options all the time (or maybe we can leverage cargo configuration).

@ticki
Contributor
ticki commented Apr 7, 2016

@nikomatsakis I don't bother recompiling it, could you give a new screenshot?

@nagisa
Contributor
nagisa commented Apr 7, 2016

@matthieu-m note that most terminal emulators already allow to switch out what exactly “red” or “blue” or “green” mean exactly at least while the regular 16-colour-pallette escape codes are being used. It also allows (potentially partially-) colour-blind people to pick the colours they consider “red” for what is considered “red” etc.

I find that to be a much more powerful configuration option than whatever rustc could provide through its own configuration file or command-line options. And it applies consistently to everything shown on the terminal emulator too.

@mrhota
Contributor
mrhota commented Apr 7, 2016

@nikomatsakis if you decide to go with a config file, it should go in ~/.config/rustc/ or something (and whatever the corresponding Windows and Mac dirs are) instead of ~.

@withoutboats
Contributor

Yugely in favor of only displaying the filename on the first line of the error; the repeating filename info is extremely distracting when trying to read errors.

@matthiasbeyer

To join this discussion: I love the proposed changes! Better error messages makes people happy! On the color discussion: I'd love to be able to configure all the colors (and the boldness etc)! I'd ship sane defaults, of course, but please do not focus on bright/dark terminals only, also test your color default settings against grey terminals.

I myself am using a xterm -bg grey18 -fg grey90 - which is perfect for me as it doesn't hurt my eyes when starring at the screen for 10 hours straight. So far I've only seen completely black, completely white and some yellowish screenshots in here - do also test out the colors between the extremes, please!

@andreastt
Contributor

I’m glad to see the machine readable file:linenumber:columnnumber format is being kept, at least for the offending error line.

@jonathandturner
Contributor

Wow, big thanks to everyone for all the good feedback. You can see the new version in the updates to this PR and in the screenshots below.

Our goals in this revision:

  • Create something that's easy to parse visually
  • Remove noise/unnecessary information
  • Present information in a way that works well for new developers, post-onboarding, and experienced developers without special configuration
  • Draw inspiration from Elm, Dybuk and other systems that have already improved on the kind of errors that Rust has.

Inspired by the feedback, we also put a set of constraints to make the end result fit these goals:

  • Multiple errors beside each other should be clearly separate and not muddled together
  • Each error should have a single place that draws the eye. This rules out two lines where the top isn't bold but the second is, because the "top" position competes with the "bold" emphasis.
  • Each error should have a "header" section that is visually distinct from the code section.
  • Code should visually stand out from text and other error messages, but without a separator or something else that stands out on its own.
  • Error messages should be just as readable when not using colors (eg some Windows, color-impaired readers, weird color schemes that we can't predict, or just people that turn colors off)
  • Be careful using “ascii art”. Instead look for ways to show the information concisely
  • Avoid sentence "notes" that use a lot of visual space but are almost identical between instances. Instead, present the notes during onboarding, and leave a small hint for people as they progress. This also means we shouldn't leave highly terse, cryptic sigils or codes, because post-onboarding users might still not remember how to use them; they still need a nudge.
  • Keep filename:line easy to spot for people who use editors that let them click on errors

screen shot 2016-04-07 at 1 36 41 pm

screen shot 2016-04-07 at 1 40 13 pm

We also came up with an alternative following the same constraints (here shown as a mockup):

screen shot 2016-04-07 at 1 44 17 pm

There's still a bit more to do:

  • Changing ~ for one of the suggestions above, or possibly just returning to ^~ style
  • Color configuration (following @nikomatsakis's suggestion, this should probably be a separate PR)
  • Move more errors to using this style. Once we go this direction, moving notes and notes-style errors to using the new structure will help with their readability also.
  • No doubt a list of corner cases that will need polish

That said, it should be getting to the point you can try it out and let us know what you think after using it on real projects.

@nagisa
Contributor
nagisa commented Apr 7, 2016

Oh, right, one thing I wanted to suggest for line numbers:

Instead of using |>, which to me personally looks way too invasive, why not consider changing the background instead? Similarly to what most editors do:

Screenshot of lines with different BG

EDIT: Even if we decide to not use different background, | would be better than |> IMO.

@jonathandturner
Contributor

A couple more screenshots.

When there are multiple code lines that don't have a message on them, we elide them with '...'

screen shot 2016-04-07 at 2 26 08 pm

In this example you can see an example close to @nikomatsakis's original example, where there are a number of notes on the source:

screen shot 2016-04-07 at 2 28 31 pm

@bstrie
Contributor
bstrie commented Apr 7, 2016

@jonathandturner I definitely like the error header between the dashes. My only concern with the new examples is a slight worry that someone might mistake |> as being a part of the code itself (for example, |> is the function composition operator in F#). Do you think we can get by without a visual separator between line numbers and code?

Also, if you're looking for subjective style feedback I would suggest mocking up examples before going to all the trouble of writing the code to generate them. :)

@bstrie
Contributor
bstrie commented Apr 7, 2016

Ah, I see nagisa beat me to the |> criticism. I like the suggestion of using inverted colors (though I'd like to see some mocked-up screenshots, and I wonder how it would look on Windows), and failing that I also think that | looks less visually heavy than |>.

@Veedrac
Contributor
Veedrac commented Apr 7, 2016

I don't want to be picky, but I just noticed the word "here" is needless noise and should be discouraged. Same for "occurs" and "is".

Compare

-- src/test/ui/borrow-errors.rs:82:18: 82:21 --
error: two closures require unique access to `vec` at the same time [E0524]
79 |>     let append = |e| {
   |>                  ~~~ first closure is constructed here
80 |>         vec.push(e)
   |>         ~~~ previous borrow occurs due to use of `vec` in closure
81 |>     };
82 |>     let append = |e| {
   |>                  ~~~ second closure is constructed here
83 |>         vec.push(e)
   |>         ~~~ borrow occurs due to use of `vec` in closure
84 |>     }
85 |> }
   |> ~ borrow from first closure ends here

with

-- src/test/ui/borrow-errors.rs:82:18: 82:21 --
error: two closures require unique access to `vec` at the same time [E0524]
79 |>     let append = |e| {
   |>                  ~~~ first closure constructed
80 |>         vec.push(e)
   |>         ~~~ previous borrow due to use of `vec` in closure
81 |>     };
82 |>     let append = |e| {
   |>                  ~~~ second closure constructed
83 |>         vec.push(e)
   |>         ~~~ borrow due to use of `vec` in closure
84 |>     }
85 |> }
   |> ~ borrow from first closure ends

or maybe even

-- src/test/ui/borrow-errors.rs:82:18: 82:21 --
error: two closures require unique access to `vec` at the same time [E0524]
79 |>     let append = |e| {
   |>                  ~~~ first closure
80 |>         vec.push(e)
   |>         ~~~ first closure borrows `vec`
81 |>     };
82 |>     let append = |e| {
   |>                  ~~~ second closure
83 |>         vec.push(e)
   |>         ~~~ second closure borrows `vec`
84 |>     }
85 |> }
   |> ~ borrow from first closure ends
@jonathandturner
Contributor

@bstrie

I suspect the separator will become second nature after you get used to it. We tried without any separator and the line number becomes too detached from the line, especially when you factor in the indent.

We couldn't use just a single pipe ('|') because that would look like valid Rust. The |> isn't in Rust, so it looks different and separate.

@bstrie
Contributor
bstrie commented Apr 7, 2016

@Veedrac On the other hand, maximally terse phrasing is somewhat less "friendly", if our goal here is also to make error messages less foreboding to newcomers. This isn't an argument in favor of extreme verbosity, but reading/flowing nicely is something to keep in mind.

@bstrie
Contributor
bstrie commented Apr 7, 2016

@jonathandturner Another thing I've noticed: the spans are very precise ("82:18: 82:21"), but the examples given here show a much larger range of code, with no visual indication as to which hint corresponds to the actual error as the compiler encountered it. Do you think it worth calling out one specific hint (in this case, the one on line 82) as indicating the "actual" error?

@alexcrichton alexcrichton referenced this pull request in rust-lang/cargo Apr 8, 2016
Merged

Add support for `cargo --explain` #2551

@eddyb
Member
eddyb commented Apr 8, 2016

@bstrie The last example @Veedrac provided changes the phrasing from passive voice to active voice which feels friendlier to me, although I could be biased (among other things, by being a non-native speaker).

@mitaa
Contributor
mitaa commented Apr 8, 2016

I see that RenderSpan::EndSpan(..) rendering has been removed - I wonder how the example given by @llogiq in #28124 would/could look like now? I think I'd prefer ^ over ~ here, otherwise for normal spans I don't really have a preference.

For some lints, the user experience would be greatly enhanced if we could report a span with multiple highlights. E.g. in a unicode lint, we currently report every matching unicode sequence within a string, which can be very verbose.

To reduce the clutter, it would be great if we could somehow advise rustc to report the string once, but highlight a given list of spans within the string. For example, this could look like:

uni.rs:5:20: "This string contàìns multìple non nfc sequenc̀es."
                              ^^       ^                  ^

Similarly, this feature could be used with a good number of other error message to get more compact, readable output.

The |> seems fine to me.

@dan-t
dan-t commented Apr 8, 2016

I like @Veedrac last version and would only change borrow from first closure ends to first closure borrow ends:

-- src/test/ui/borrow-errors.rs:82:18: 82:21 --
error: two closures require unique access to `vec` at the same time [E0524]
79 |>     let append = |e| {
   |>                  ~~~ first closure
80 |>         vec.push(e)
   |>         ~~~ first closure borrows `vec`
81 |>     };
82 |>     let append = |e| {
   |>                  ~~~ second closure
83 |>         vec.push(e)
   |>         ~~~ second closure borrows `vec`
84 |>     }
85 |> }
   |> ~ first closure borrow ends

The other thing is, if the redundant vec could be left out, because the variable name is already marked:

-- src/test/ui/borrow-errors.rs:82:18: 82:21 --
error: two closures require unique access to `vec` at the same time [E0524]
79 |>     let append = |e| {
   |>                  ~~~ first closure
80 |>         vec.push(e)
   |>         ~~~ first closure borrows
81 |>     };
82 |>     let append = |e| {
   |>                  ~~~ second closure
83 |>         vec.push(e)
   |>         ~~~ second closure borrows
84 |>     }
85 |> }
   |> ~ first closure borrow ends
@ticki
Contributor
ticki commented Apr 8, 2016

Perhaps a little radical, but I propose something like this:

Code can be found here, I encourage you to run it and post a screenshot of how it looks with your color scheme.

@ticki
Contributor
ticki commented Apr 8, 2016

@dan-t, that is out of the scope of this PR. It doesn't deal with the error messages, only the style of the errors.

@Aatch
Contributor
Aatch commented Apr 8, 2016

I'd like to pipe up and say that I tend to have issues with unicode characters in my terminal, I should probably fix that, but it does make looking at output that makes heavy use of drawing characters annoying. A way of turning use of them off (preferably an environment variable) would be nice.

@ticki
Contributor
ticki commented Apr 8, 2016

@Aatch

One thing I have noticed about many of the proposals here is that they have the following template in common:

<a>[file path]<b>[start]<c>[end]<d>[line number]<e>[code]<f>[space before error marker]<g>[error marker][error marker]...[error marker]<h>[error]...

Making the <a>, <b>, etc. customizable will allow each user to define its own error style, and would, perhaps, be less controversial.

@ticki
Contributor
ticki commented Apr 8, 2016

Furthermore, it is a must-have for all modern terminals to support Unicode box-drawing characters (which my example makes use of). Even many non-Unicode terminals (e.g., older terminals, Windows terminals, etc.), supports Unicode box-drawing characters.

@llogiq
Contributor
llogiq commented Apr 8, 2016

@Ticki I know of enough examples where misconfigured terminals horribly mangle drawing characters. As an option, fine, but I think we shouldn't rely on them.

@ticki
Contributor
ticki commented Apr 8, 2016

w/o Unicode:

@phil-opp
Contributor
phil-opp commented Apr 8, 2016

I really like the suggestion from @Ticki (with or without Unicode). Especially:

  • the at between file name and row/column information.
    I think the …/borrow‑error.rs:90:21: 90:24 format is hard to read. (Alternatively, a bit more spacing would be nice: …/borrow‑error.rs : 90:21 : 90:24.)
  • the background color for the line numbers (instead of a |> separator)
@dan-t
dan-t commented Apr 8, 2016

Like @llogiq said @Ticki, even if it's 2016, text encodings are still a pain and if the output of the
compiler gets parsed by several tools it's still a good idea to keep it as simple as possible.

@dan-t
dan-t commented Apr 8, 2016

@Ticki I like your last w/o unicode one, only the | in the line number row seems unnecessary.

@ticki
Contributor
ticki commented Apr 8, 2016

@dan-t That is why you add a machine readable option and several other option for configuring the output.

@ticki
Contributor
ticki commented Apr 8, 2016

@dan-t Originally, I thought that too, but it is just odd with an empty space. The | solves that by denoting a continuation of the previous line.

@nagisa
Contributor
nagisa commented Apr 8, 2016

There’s escape sequences for underlining text already. Might want to use them.

I’m pretty strongly against using Unicode, at least by default, I think. I’m pretty confident that implementing it will only serve to alienate some new users while, subjectively, being somewhat more beautiful.

If we adopt any unicode-by-default in error messages, I foresee an issue every week or month about people with lack of fonts/codepages/etc seeing weird output. Who knows how many more people would not even bother reporting it before dropping Rust altogether.

@DanielKeep
Contributor

There’s escape sequences for underlining text already.

As far as I know, you can't handle these on Windows.

You know, the longer this goes on, the more I'm inclined to think the real solution is do whatever's easy and straightforward, then add support for emitting compiler messages in JSON or something so the bike-shedding can happen elsewhere. Between colour, line number separators, box drawing, span merging, and context lines... it's only a matter of time before someone suggests supporting HTML terminals or sixel output.

...please, no one get any ideas.

Edit: As noted by Ticki, JSON error output is already present. You just need to read the compiler source code to find out about it, then use -Z unstable-options --error-format human! Can't imagine why, when I went looking for this, I was completely unable to find it! cough :)

@ticki
Contributor
ticki commented Apr 8, 2016

@nagisa

There’s escape sequences for underlining text already. Might want to use them.

Yes, but you cannot control the color of those. They simply inherit it from the foreground color.

@ticki
Contributor
ticki commented Apr 8, 2016

@DanielKeep

then add support for emitting compiler messages in JSON

That is already possible, see #30711.

Between colour, line number separators, box drawing, span merging, and context lines... it's only a matter of time before someone suggests supporting HTML terminals or sixel output.

That is pretty much a slippery slope argument. Currently, the error output is very hard to read, which is something you can fix without taking it to the extreme. We already do use ANSI codes in the output. We are merely debating in what way it should be used in the future.

@ticki
Contributor
ticki commented Apr 8, 2016

Neither do I think Unicode is strictly necessary. I think they look fine without them, so I wouldn't mind if we used plain ASCII in the error output.

@lambda
Contributor
lambda commented Apr 8, 2016

I like Ticki's suggestion; for me, either the Unicode or non-Unicode versions work, but I understand the concerns of those with terminals that may not render box-drawing characters properly. I definitely like the more understated colors of this version.

The one tweak I would make is that if using a character like |, or : as I suggested earlier, to tie the line numbers together, they should be aligned on the rightmost column. That will work the best when not all line numbers have the same number of digits:

 98
  |
 99
  |
100
  |
101
@Veedrac
Contributor
Veedrac commented Apr 8, 2016

@Ticki You asked for screenshots, so here:

error_ascii

I'm skipping the Unicode one since nobody seems to be pushing it any more.

I'm not a fan of the line number background. The grayscale error text doesn't work too well on my terminal either; I use low contrast colors, so the grayscale isn't far off from my text color.

Personally, I'd just stick with what we have, apply @lambda's suggestion (|> is noisy), and color the highlighted region. I'd shorten the text too, of course.

error_bikeshed

@ticki
Contributor
ticki commented Apr 8, 2016

@Veedrac: The background could easily be fixed by setting inverted and faint GRM.

@GuillaumeGomez
Member

@Veedrac: I prefer second screenshot. However, I prefer '|' instead of ':' at the beginning of the lines and '^' instead of '~'. ^^'

@ticki
Contributor
ticki commented Apr 8, 2016

I honestly liked the first screenshot more (modulo the line number background, which should be adjusted by setting invert and faint).

@bors
Contributor
bors commented Apr 19, 2016

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

@nikomatsakis
Contributor
nikomatsakis commented Apr 20, 2016 edited

So we did a lot of iteration on this branch and we now feel ready to actually land this branch. This doesn't mean that we're done -- we expect to keep iterating -- but we think we reached a good enough point where it's a solid improvement on the status quo. (We've been dogfooding this for personal projects now for some time.)

@jonathandturner is going to do a writeup of where we landed in terms of the output. We tried to collect in the main points of feedback.

@nrc, note that the commit 40bfad2 and before are actually part of #33020, which annoyingly has not landed yet. =) The commits are not standalone, but I did try to group things up into logical changes, and to order them with the most interesting stuff first.

@nikomatsakis nikomatsakis changed the title from [WIP] Overhaul borrowck error messages and compiler error formatting generally to Overhaul borrowck error messages and compiler error formatting generally Apr 20, 2016
@nikomatsakis
Contributor

I wrote this:

I did try to group things up into logical changes, and to order them with the most interesting stuff first.

Of course, github reorders things according to its own fancy. Sigh.

@jonathandturner
Contributor
jonathandturner commented Apr 20, 2016 edited

Again, a big thanks for all the comments :)

Like @nikomatsakis says this is a "good point" for people to start trying it out and giving us feedback. Building on where we were in the last update and on the comments we got, here's where we are today.

White:
screen shot 2016-04-20 at 1 05 28 pm

Black:
screen shot 2016-04-20 at 1 29 18 pm

A quick rundown of the anatomy of the message:

  • We show the relevant source related to the error, showing a little extra source between errors or eliding lines to help you focus on the error
  • The primary span of the error is now highlighted with ^^^ and is colored to match the level (error/warning). Additionally, the source above the primary span is colored to match as well, which came from suggestions on this thread
  • Additional notes are shown with a simple --- highlight.
  • We use a |> separator between the line number and source line that helps you, at a glance, know where source code begins (even when not using colors). We tried some of the alternate approaches mentioned in the thread, but after dogfooding we came back to Dybuk's style
  • We split up the header to multiple lines, so that the error/warning message stands out and can be used as a separator when you have multiple messages. The position of the primary span is on the second line. One gotcha here that is to experiment with how this impacts editors and clicking on errors. @nikomatsakis has it working in emacs, but we'd like to test other editors as well.

In short, we went for polish on the original idea and focused on quick readability that works equally well in non-unicode, non-colored environment.

To keep polishing, we wanted to get this out in front of people who could use it for a while in their daily work and point out what still needs work after you've spent a little time with the new format.

@wycats
Contributor
wycats commented Apr 20, 2016

Big kudos to @nikomatsakis and @jonathandturner for polishing this so hard. "Design a highly technical message using only monospace fonts, 16 colors, and ASCII-7" is an incredibly hard design challenge, and one that is very easy to abandon. The success at finding balance here is astonishing. Good work!

@jonathandturner
Contributor

It's worth noting, too, that @wycats also volunteered a lot of time to send us feedback on various iterations and give us the benefit of his newspaper background. Kudos for that 🎉 🎉 🎉

@nikomatsakis
Contributor

@nrc found some minor errors in make check; hopefully they're fixed, though I'm not 100% sure yet. Still worth reviewing I think.

@nrc
Contributor
nrc commented Apr 21, 2016

I'm starting technical review, might take a while, there's a lot of code.

On the presentation, I'd like to see less bold - I think it makes it harder to read on most consoles (both less legible and more distracting). I would only use bold for the main message, not for the other labels and not for anything that is in colour.

@nrc nrc commented on the diff Apr 21, 2016
src/libsyntax/codemap.rs
#[derive(Clone)]
pub struct MultiSpan {
- pub spans: Vec<Span>
+ primary_spans: Vec<Span>,
+ span_labels: Vec<(Span, String)>,
@nrc
nrc Apr 21, 2016 edited Contributor

could MultiSpan be a Vec<SpanLabel>? Seems odd to have these two similar but distinct data structures,

@nikomatsakis
nikomatsakis Apr 21, 2016 Contributor

Do you mean "can it just have a field that is a `Vec"? I tried that at first but ultimately I found it cleaner this way -- the MultiSpan basically represents the "raw material" from which we will make up a snippet.

At first I tried to have the push-span method update a Vec<SpanLabel> but the code was pretty messy that way. When you first push, the label is None but level is primary, then it becomes some, and if another label is added to that same span (not that this seems likely), you would have to go find if there was a previous span with the same value, so as to mark it primary, etc. Not a huge thing, but this felt nicer. I guess if you add same span multiple times, only one might be primary, but that also seems a bit odd -- how do we know which message is the "primary" one? If we were to go that route, I would want to have the push_label take a "primary/secondary" boolean argument instead.

Bah. Sorry if I'm not being clear. TL;DR is that I think there is room to iterate here, we should think about how we want end-users to "build up" the information needed to render a snippet. I found that just Vec<SpanLabel> didn't seem right, but I don't know that what is currently there is right either.

(I don't like the name MultiSpan though.)

@nrc nrc commented on an outdated diff Apr 21, 2016
src/libsyntax/codemap.rs
#[derive(Clone)]
pub struct MultiSpan {
- pub spans: Vec<Span>
+ primary_spans: Vec<Span>,
+ span_labels: Vec<(Span, String)>,
+}
+
+#[derive(Clone, Debug)]
+pub struct SpanLabel {
+ /// the span we are going to include in the final snippet
+ pub span: Span,
+
+ /// is this a primary span? This is the "locus" of the message,
+ /// and is indicated with a `^^^^` underline, versus `----`
+ pub is_primary: bool,
+
+ /// what label should we attach to this span (if any)?
@nrc
nrc Apr 21, 2016 Contributor

Nit: comments should be proper sentences - start with capitals, end with periods.

@nrc nrc commented on an outdated diff Apr 21, 2016
src/libsyntax/codemap.rs
}
}
- /// Inserts the given span into itself, for use with `end_highlight_lines`.
- pub fn push_trim(&mut self, mut sp: Span) {
- let mut prev = mk_sp(BytePos(0), BytePos(0));
+ pub fn push_primary_span(&mut self, span: Span) {
@nrc
nrc Apr 21, 2016 Contributor

This function doesn't really seem to pull its weight

@ticki
Contributor
ticki commented Apr 30, 2016

Dumb dumb bors.

@jonathandturner
Contributor

@bors r=nrc

@bors
Contributor
bors commented Apr 30, 2016

📌 Commit c3a1696 has been approved by nrc

@bors
Contributor
bors commented Apr 30, 2016

⌛️ Testing commit c3a1696 with merge b1c6070...

@bors bors added a commit that referenced this pull request Apr 30, 2016
@bors bors Auto merge of #32756 - nikomatsakis:borrowck-snippet, r=nrc
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
b1c6070
@bors
Contributor
bors commented Apr 30, 2016

💔 Test failed - auto-linux-64-opt-rustbuild

@alexcrichton
Member

Failure looks legitimate, and may be related to #32445

@bors
Contributor
bors commented May 2, 2016

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

@nikomatsakis
Contributor

@alexcrichton yeah, it looks legit, though I can't reproduce locally. I hadn't thought of #32445 though, could be connected.

@nikomatsakis
Contributor

I take it back. I can reproduce: I just didn't know that make check-docs was not a subset of make check.

@nikomatsakis
Contributor

The problem is an assertion failure in the snippet rendering code. I'm currently rebuilding with -g so I can tell which one :)

nikomatsakis and others added some commits Apr 20, 2016
@nikomatsakis nikomatsakis revamp MultiSpan and introduce new snippet code
MultiSpan model is now:

- set of primary spans
- set of span+label pairs

Primary spans render with `^^^`, secondary spans with `---`.

Labels are placed next to the `^^^` or `---` marker as appropriate.
a20ee76
@nikomatsakis nikomatsakis update test cases to reflect new messages e416518
@nikomatsakis nikomatsakis thread tighter span for closures around
Track the span corresponding to the `|...|` part of the closure.
1ff1887
@nikomatsakis nikomatsakis add borrowck info inline in main snippet
This uses the new `span_label` APIs
5b150cf
@nikomatsakis nikomatsakis refactor to use new snippet code and model
Major changes:
- Remove old snippet rendering code and use the new stuff.
- Introduce `span_label` method to add a label
- Remove EndSpan mode and replace with a fn to get the last
  character of a span.
- Stop using `Option<MultiSpan>` and just use an empty `MultiSpan`
- and probably a bunch of other stuff :)
11dc974
@nikomatsakis nikomatsakis adapt JSON to new model
Each Span now carries a `is_primary` boolean along with an optional
label. If there are multiple labels for a span, it will appear multiple
times.
e7c7a18
@nikomatsakis nikomatsakis use new `note_expected_found` API
This API pulls the "expected type foo, found type bar" out after the
main snippet. There are some other places where it makes sense, but this
is a start.
d9a947c
@nikomatsakis nikomatsakis replace fileline_{help,note} with {help,note}
The extra filename and line was mainly there to keep the indentation
relative to the main snippet; now that this doesn't include
filename/line-number as a prefix, it is distracted.
489a6c9
@nikomatsakis nikomatsakis WIP factor out RudimentaryEmitter 41a652e
@nikomatsakis nikomatsakis change errors from Yellow to Magenta
The Yellow text is very hard to read with a white background.
71c6f81
@nikomatsakis nikomatsakis delete the json-errors test
It's primary purpose was to check that json worked at all,
but compiletest does that now.
4714394
@nikomatsakis nikomatsakis rewrite span-length to include strings
It is way easier to copy-and-paste strings from the output
than to figure out how to reproduce them from first
principles.
9d022f2
@nikomatsakis nikomatsakis refactor the Emitter trait
There is now a CoreEmitter that everything desugars to, but without
losing any information. Also remove RenderSpan::FileLine. This lets the
rustc_driver tests build.
1067850
@nikomatsakis nikomatsakis fix error message in librustc_driver tests 8013eeb
@nikomatsakis nikomatsakis Nit: comments should be uppercase letter 5adfe5b
@nikomatsakis nikomatsakis Fix whitespace 9a9c9af
@nikomatsakis nikomatsakis Do not import variants from RenderedLineKind e56121c
@nikomatsakis nikomatsakis Nit: do not import variants from Style d58a4be
@nikomatsakis nikomatsakis Nit: do not use RLK d5529f0
@nikomatsakis nikomatsakis Nit: address various style nits f6496cd
@nikomatsakis nikomatsakis only emit `^` at the start of a multi-line error
as a result, simplify elision code
1fdbfcd
@nikomatsakis nikomatsakis Nit: in emitter.rs 94841be
@nikomatsakis nikomatsakis Nit: use last_mut better 24f4b15
@nikomatsakis nikomatsakis Nit: add comment 883b969
@nikomatsakis nikomatsakis Nit: remove push_primary_span, which was never called 5db4d62
@nikomatsakis nikomatsakis fix tests better ba12ed0
@nikomatsakis nikomatsakis Nit: use Range::contains 8a9ad72
@nikomatsakis nikomatsakis fix snippet tests MORE! 790043b
@nikomatsakis nikomatsakis change color of warning to YELLOW 89d086b
@nikomatsakis nikomatsakis move "lint level defined here" into secondary note
It does not help you to understand the error, just explains why you are
seeing it, so it is clearly secondary.
49dfac4
@jonathandturner @nikomatsakis jonathandturner Add back in a 'old school' error format 84cb56f
@jonathandturner @nikomatsakis jonathandturner Finish up with 'old school' error mode 79f61a4
@jonathandturner @nikomatsakis jonathandturner Fix up error-pattern style test 5974e5b
@nikomatsakis nikomatsakis fix rebase flaws 2ba5fac
@nikomatsakis nikomatsakis update unit tests 95576b8
@jonathandturner @nikomatsakis jonathandturner Fix unicode test to use original error format f359aa2
@nikomatsakis nikomatsakis patch travis failure 64e0819
@nikomatsakis nikomatsakis do not fail if len(rendered_lines) is == 1
also handle more rendered-lines
9d151a7
@nikomatsakis nikomatsakis avoid double panic db8a9a9
@nikomatsakis nikomatsakis assert we get at least two rendered lines back
9355a91
@nikomatsakis
Contributor

@bors r=nrc

@bors
Contributor
bors commented May 2, 2016

📌 Commit 9355a91 has been approved by nrc

@Manishearth Manishearth added a commit to Manishearth/rust that referenced this pull request May 2, 2016
@Manishearth Manishearth Rollup merge of #32756 - nikomatsakis:borrowck-snippet, r=nrc
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: rust-lang#33240

Joint work with @jonathandturner.

Fixes rust-lang#3533
df984bd
@bors bors added a commit that referenced this pull request May 2, 2016
@bors bors Auto merge of #33354 - Manishearth:rollup, r=Manishearth
Rollup of 14 pull requests

- Successful merges: #32756, #33129, #33225, #33260, #33309, #33320, #33323, #33324, #33325, #33330, #33332, #33334, #33335, #33346
- Failed merges:
a80321d
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this pull request May 2, 2016
@Manishearth Manishearth Rollup merge of #32756 - nikomatsakis:borrowck-snippet, r=nrc
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: rust-lang#33240

Joint work with @jonathandturner.

Fixes rust-lang#3533
be83adc
@bors bors added a commit that referenced this pull request May 2, 2016
@bors bors Auto merge of #33354 - Manishearth:rollup, r=Manishearth
Rollup of 14 pull requests

- Successful merges: #32756, #33129, #33225, #33260, #33309, #33320, #33323, #33324, #33325, #33330, #33332, #33334, #33335, #33346
- Failed merges:
96e5676
@bors
Contributor
bors commented May 2, 2016

⌛️ Testing commit 9355a91 with merge e6180dc...

@bors bors added a commit that referenced this pull request May 2, 2016
@bors bors Auto merge of #32756 - nikomatsakis:borrowck-snippet, r=nrc
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
e6180dc
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this pull request May 2, 2016
@Manishearth Manishearth Rollup merge of #32756 - nikomatsakis:borrowck-snippet, r=nrc
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: rust-lang#33240

Joint work with @jonathandturner.

Fixes rust-lang#3533
2d78e34
@bors
Contributor
bors commented May 2, 2016

⛄️ The build was interrupted to prioritize another pull request.

@bors bors added a commit that referenced this pull request May 2, 2016
@bors bors Auto merge of #33354 - Manishearth:rollup, r=Manishearth
Rollup of 14 pull requests

- Successful merges: #32756, #33129, #33225, #33260, #33309, #33320, #33323, #33324, #33325, #33330, #33332, #33334, #33335, #33346
- Failed merges:
61e3171
@bors
Contributor
bors commented May 2, 2016

⌛️ Testing commit 9355a91 with merge 78a20e2...

@bors bors added a commit that referenced this pull request May 2, 2016
@bors bors Auto merge of #32756 - nikomatsakis:borrowck-snippet, r=nrc
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
78a20e2
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this pull request May 2, 2016
@Manishearth Manishearth Rollup merge of #32756 - nikomatsakis:borrowck-snippet, r=nrc
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: rust-lang#33240

Joint work with @jonathandturner.

Fixes rust-lang#3533
97d1666
@bors
Contributor
bors commented May 2, 2016

⛄️ The build was interrupted to prioritize another pull request.

@bors bors added a commit that referenced this pull request May 2, 2016
@bors bors Auto merge of #33354 - Manishearth:rollup, r=Manishearth
Rollup of 14 pull requests

- Successful merges: #32756, #33129, #33225, #33260, #33309, #33320, #33323, #33324, #33325, #33330, #33332, #33334, #33335, #33346
- Failed merges:
e44aefa
@bors
Contributor
bors commented May 2, 2016

⌛️ Testing commit 9355a91 with merge ce4176d...

@bors bors added a commit that referenced this pull request May 2, 2016
@bors bors Auto merge of #32756 - nikomatsakis:borrowck-snippet, r=nrc
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
ce4176d
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this pull request May 2, 2016
@Manishearth Manishearth Rollup merge of #32756 - nikomatsakis:borrowck-snippet, r=nrc
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: rust-lang#33240

Joint work with @jonathandturner.

Fixes rust-lang#3533
5ccc698
@bors
Contributor
bors commented May 2, 2016

⛄️ The build was interrupted to prioritize another pull request.

@bors bors added a commit that referenced this pull request May 2, 2016
@bors bors Auto merge of #33354 - Manishearth:rollup, r=Manishearth
Rollup of 14 pull requests

- Successful merges: #32756, #33129, #33225, #33260, #33309, #33320, #33323, #33324, #33325, #33330, #33332, #33334, #33335, #33346
- Failed merges:
8653d33
@bors
Contributor
bors commented May 3, 2016

⌛️ Testing commit 9355a91 with merge 44b3cd8...

@bors bors added a commit that referenced this pull request May 3, 2016
@bors bors Auto merge of #32756 - nikomatsakis:borrowck-snippet, r=nrc
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
44b3cd8
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this pull request May 3, 2016
@Manishearth Manishearth Rollup merge of #32756 - nikomatsakis:borrowck-snippet, r=nrc
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: rust-lang#33240

Joint work with @jonathandturner.

Fixes rust-lang#3533
40199f6
@bors bors added a commit that referenced this pull request May 3, 2016
@bors bors Auto merge of #33354 - Manishearth:rollup, r=Manishearth
Rollup of 14 pull requests

- Successful merges: #32756, #33129, #33225, #33260, #33309, #33320, #33323, #33324, #33325, #33330, #33332, #33334, #33335, #33346
- Failed merges:
43c5fef
@bors bors merged commit 9355a91 into rust-lang:master May 3, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@ticki
Contributor
ticki commented May 3, 2016

🎉

@nikomatsakis nikomatsakis deleted the nikomatsakis:borrowck-snippet branch May 3, 2016
@joshtriplett
Member

I really like the new approach posted by @jonathandturner in #32756 (comment) , particularly the method of highlighting the primary error.

Two remaining issues with it, one minor, one major:

  • Minor: please do consider dropping the |> marker on non-code lines, and only keeping it next to the line numbers, as in the approach shown as "alternative" in #32756 (comment) . I think this would help highlight and distinguish source lines from surrounding explanatory lines.
  • Major: putting a marker before the file:line:column: indicator will break many editors/IDEs, which expect that to appear starting in the first column. A quick test with vim's default settings shows that it treats the --> as part of the filename. I do think it looks great for terminal use, just not for use in an editor. Perhaps detect if stdout is a terminal, use this format if so, and if not, drop that marker?
@nikomatsakis
Contributor

@joshtriplett

putting a marker before the file:line:column: indicator will break many editors/IDEs, which expect that to appear starting in the first column.

This is really a sticking point. I'm not really sure what's the best fix here. I think that the current format with the --> coming second is easier to read: I've spent a lot of time A/B testing as well as asking around and I feel pretty confident about that. There are a couple of reasons for this:

  • It puts the primary data (error, message) up front
  • It avoids the problem that the (sometimes very long) filename pushes the message way over into another "horizontal column" than the examples that come belonw
    • I think this is (partly, no doubt) what led us to putting filenames before every line before, so as to keep things kind of horizontally aligned

OTOH, I really want to enable "low-tech" editor integration. One thing we've debated about is having a "third mode". For example, perhaps we DO use an environment variable (RUST_ERROR_FORMAT) that could select default/json/header, where header is the new mode. If you have header, it looks the same as normal except that it adds:

filename:line:col: [error|warning]: message

in front of the error. This is intended for your editor to match on, basically. (The downside of this is that the message would appear twice, which is a shame.) The reason I've come to favor an env var for this purpose is that one can easily set it in the editor's setup and have it "propagate down" to compilations executed by the editor, but not affect compilations done by hand on the console.

But I guess the other question is how easy it will be to adjust editor modes to the new format as it exists today. For example, I opened this PR on the emacs mode rust-lang/rust-mode#154. If we can just get editors to either use JSON or else use the default format, that might be best.

@ticki
Contributor
ticki commented May 4, 2016

I think that this form of integration is hacky. IDEs should prefer JSON.

@joshtriplett
Member

@nikomatsakis I absolutely agree that this format is more usable by humans, just not by existing editors.

I don't think it's reasonable to require an environment variable for this; error message parsing and handling should work out of the box. Right now, I have no Rust support in vim (because the Vim mode for Rust isn't packaged in Debian), and I regularly use :set makeprg=cargo and :make build; I can also log the output of a build and run vim -q build.log. I would argue that those should all continue to work; this shouldn't require special editor configuration. And changing existing editors doesn't suffice; there's a long tail of editors people prefer, and any programmer's editor that parses error messages expects this convention.

Having an environment variable and command-line option for explicit handling, or for IDEs/editors that want to implement special support for Rust, seems like a good idea. However, I would suggest that the default should be "auto", which would imply "normal" if stderr is a TTY, or "header" if it isn't.

@ticki Current generation IDEs with specifically designed support for Rust might, but we can't write off decades of programmer's editors and the conventions they expect. The standard error message format is file:line: or file:line:column:.

@nagisa
Contributor
nagisa commented May 4, 2016 edited

The standard error message format is file:line: or file:line:column:.

It is only a de facto convention, not standard. There’s a bunch of things that do not honour the convention as well.

@ticki
Contributor
ticki commented May 4, 2016

@joshtriplett

@ticki Current generation IDEs with specifically designed support for Rust might, but we can't write off decades of programmer's editors and the conventions they expect. The standard error message format is file:line: or file:line:column:.

Yeah, most compilers use that, so it wouldn't make sense to drop it. I simply think that for Rust-specific IDEs, there should be more emphasis on JSON, since it is strictly less ambigious and spec-able

@joshtriplett
Member

@ticki Absolutely agreed; anything doing more than the most basic parsing should use the JSON format.

@nagisa Half the world runs smoothly because of de-facto conventions. The default configuration shouldn't break people's preferred editors.

Again, we're not talking about changing the default format for terminal use; we're talking about automatically detecting when output goes to a log and adding a line to simplify parsing.

@nikomatsakis One other aspect of the "header" format: it needs to not include the :line:column portion after the filename on the marker line, because otherwise the error will show up twice, once in /path/to/file:line:column and once in "--> /path/to/file:line:column. One test case for the "header" format should be to write build output to a log, runvim -q log, repeatedly hit:cn[enter]`, and make sure the list of errors traversed only includes the header lines, not any other part of the error message.

@nikomatsakis
Contributor

Rather than discussing this "header format" on this closed PR, I propose we move the conversation here: http://internals.rust-lang.org/t/editor-compatibility-and-the-new-error-format/3435

I tried to summarize the salient points.

@bors bors added a commit that referenced this pull request May 8, 2016
@bors bors Auto merge of #33369 - nikomatsakis:graceful-empty-span, r=jntrnr
degrade gracefully with empty spans

In #32756, we solved the final test failure, but digging more into it the handling of that scenario could be better. The error was caused by an empty span supplied by the parser representing EOF. This patch checks that we cope more gracefully with such spans:

r? @jonathandturner
40a4c64
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this pull request May 8, 2016
@Manishearth Manishearth Rollup merge of #33369 - nikomatsakis:graceful-empty-span, r=jntrnr
degrade gracefully with empty spans

In rust-lang#32756, we solved the final test failure, but digging more into it the handling of that scenario could be better. The error was caused by an empty span supplied by the parser representing EOF. This patch checks that we cope more gracefully with such spans:

r? @jonathandturner
eafec60
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this pull request May 8, 2016
@Manishearth Manishearth Rollup merge of #33369 - nikomatsakis:graceful-empty-span, r=jntrnr
degrade gracefully with empty spans

In rust-lang#32756, we solved the final test failure, but digging more into it the handling of that scenario could be better. The error was caused by an empty span supplied by the parser representing EOF. This patch checks that we cope more gracefully with such spans:

r? @jonathandturner
ea7185c
@Manishearth Manishearth added a commit to Manishearth/rust that referenced this pull request May 8, 2016
@Manishearth Manishearth Rollup merge of #33369 - nikomatsakis:graceful-empty-span, r=jntrnr
degrade gracefully with empty spans

In rust-lang#32756, we solved the final test failure, but digging more into it the handling of that scenario could be better. The error was caused by an empty span supplied by the parser representing EOF. This patch checks that we cope more gracefully with such spans:

r? @jonathandturner
dfa5245
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment