Style changes to compiler messages

[wafflespeanut]

fixes #3533

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[wafflespeanut]

@Manishearth A lot of things need to be clarified! Feel free to burn it down :)

[wafflespeanut]

The space is added only to separate the span from the file name (for visibility).
The colon has been replaced with an arrow, because we already use it to separate the line number from the character number.

[Manishearth]

I think the first space shouldn't be there. Many editors let you say open foo.rs:1:2 on the command line and will open focused on that line. It's easier to copy-paste if there is no space.

[nrc]

I would not use -> since we use it for function types/signatures. I'd use to or ... or keep the :

[wafflespeanut]

I've put this here only for a preview. If we're gonna avoid the file path (in a similar fashion) for the other messages too, then we can put it directly into the function.

But, if we're going for relative paths, then this won't look nice, and this should be reverted back. So, what's the plan?

[steveklabnik]

I really like the relative paths because printing the full path leaks paths that don't exist, ie, to the buildbot that the stdlib was built on. This is annoying both because it is a nonexistant path, but because it includes "slave", which is really awkward.

[wafflespeanut]

Agreed! I'm happy about relative paths :)

[Manishearth]

One slight issue with relative paths is that when cargo spits out errors for multiple deps it's confusing which dep is being talked about. Though I'm not sure if this will still be an issue.

[nrc]

I like absolute paths when building locally - I know I can always copy and paste and it will work - with relative paths I have to be in the right place, which with a complex build system doesn't always happen.

[Manishearth]

(Also, I think the /slave/foo thing only happens in assertions, not in compiler errors. In compiler errors that only turns up when a Rust PR fails buildbot which is okay)

[wafflespeanut]

Since our idea is to drop displaying file paths for the latter part of the span, we'd be reverting the patches made for #11715 and some of its sub-issues. Is that okay?

[Manishearth]

We'll be preserving the old behavior via a command line flag, so this stays in the old-behavior method?

[wafflespeanut]

Now, a single-line error message looks like, (yeah, it's funny)

/home/wafflespeant/Desktop/single-line-err.rs: 3:1 -> 3:2 
  error: expected one of `.`, `;`, or an operator, found `}`
    }
    ^

... and a multi-line warning looks like,

/home/wafflespeant/Desktop/multi-line-warn.rs: 1:1 -> 8:2 
  warning: trait `foo` should have a camel case name such as `Foo`, #[warn(non_camel_case_types)] on by default
    trait foo {
        fn blah1();
        fn blah2();
        fn blah3();
        fn blah4();
        fn blah5();
    ...

Thoughts?

[wafflespeanut]

Oh no. Look at the travis log!

[Manishearth]

That's compiletest not being able to handle the new formatting. We can either add a flag to preserve the old formatting (which we need to do anyway, simple fix) and have compiletest use that, or we can fix up compiletest to handle the new formatting.

[Manishearth]

bikeshed:

• Should the single line warning be made into a two line warning? (My opinion: No)
• Should we be putting the warning on a new line in any situation? (My opinion: Meh)
• How much indentation? If we're not putting the warnings on a new line 4 spaces sounds right. 2+2 when the warning is on a new line (2 for the warning, 4 for the code)?

cc @bstrie @steveklabnik @nrc @frewsxcv

[nrc]

@Manishearth not sure how this relates to @wafflespeanut's example above, so I may be misunderstanding, but...

• I like having the file info on a different line to the error/warning - makes it easy to scan for errors
• when we have a block of code, I like the code to not be indented at all to make it easy to compare to the code in the file. Alternatively, having line numbers is sometimes useful. If it is just a snippet, I don't care, but consistency is important.

What does an error message with a macro expansion trace look like?

[Manishearth]

The reason I think it should be indented is to visually separate code from error. We already do that with colors, but I think this would be clearer -- easier to see errors when scanning the log

[nrc]

If the error is indented, then being not indented would give visual separation (in theory, you'd have to have a big example to check).

[wafflespeanut]

@nrc Here's macro expansion

/home/wafflespeanut/Desktop/macro-expand.rs: 2:14 -> 2:25 
  error: expected a literal
        println!(asasdasddas);
                 ^~~~~~~~~~~
<std macros>: 1:33 -> 1:58 note: in this expansion of concat!
<std macros>: 2:25 -> 2:56 note: in this expansion of format_args!
<std macros>: 1:23 -> 1:60 note: in this expansion of print! (defined in <std macros>)
/home/wafflespeanut/Desktop/macro-expand.rs: 2:5 -> 2:27 note: in this expansion of println! (defined in <std macros>)

  error: aborting due to previous error

[wafflespeanut]

@nrc

I like the code to not be indented at all to make it easy to compare to the code in the file.

Um, we only have a few lines of code. Others are replaced with the ellipsis anyway, right?

Alternatively, having line numbers is sometimes useful.

The first line in any compiler message shows the entire span where something bad has occurred. Only the other few lines don't have the line numbers. Should we really need the line numbers for all the lines?

[evincarofautumn]

For what it’s worth, there is a GNU standard for source locations in diagnostic messages, which some tools (Emacs?) support out of the box for jumping to and highlighting locations:

path:line:column: message
path:line.column1-column2: message
path:line1-line2: message
path:line1.column1-line2.column2: message

It extends easily:

path:line:column: brief (e.g., displayed in status bar)
  explanation
  source code
  &c.

Also, re. relative vs. absolute paths, errors could follow the behaviour of Make’s --print-directory/-w (presumably with a flag). You can have pretty error messages for humans with relative paths, but tools can still deduce absolute paths.

[bstrie]

Tagging with T-Tools as I figure it's worth having the Tools team meet to discuss this.

[bstrie]

Personally I feel like rather than have a switch for "legacy error message mode", we should just move forward with nicely-formatted and deliberately-unstable human-readable error messages, with an optional switch to produce machine-readable error messages. That way we can support tools nicely (nicer than the current error messages) while also allowing us to evolve the typical error messages without feeling like everything has to be perfect immediately.

For example, I like Elm's use of color in error message to direct the reader's eye (https://www.reddit.com/r/rust/comments/3ti20g/not_rust_specific_compilers_as_assistants_elm_016/), but that's not something that we need to bikeshed immediately. For now just removing the redundant filename to fix #3533 would be a win on its own.

[brendanzab]

@evincarofautumn: GNU paths are very handy. One of the small things that annoys me about Elm's errors is the lack of those paths. They don't need to be repeated constantly like rustc currently does, but it would be a shame to lose them in the interests of going too far towards hiding complexity. I guess with machine-readable errors in the future, it might become easier to make 'beginner' and 'advanced' front-ends for error formatting (that may not be desirable though).

[brendanzab]

Ugh - your right @bstrie, this is not really the place for bikeshedding this stuff. Sorry!

[bstrie]

@wafflespeanut, I tend to agree with those advocating the GNU style for formatting the file/line/column readout, if only because it's a widely-used precedent and we could spend forever bikeshedding alternatives. For now I think it's still a huge win to just remove the redundant info on every single line and at last put #3533 to bed.

[frewsxcv]

👍 for anything that makes the messages consume less horizontal space

[brson]

I'd like to see an example of serious spew after this change. It's hard to judge the true effect of this from tiny snippets.

Adding another line to every error with a span will decrease the overall number of errors per screen.

[brson]

I think this is more a @rust-lang/compiler bug than @rust-lang/tools

[arielb1]

@brson

The error message already splits to multiple lines because the pathname is so long.

[brendanzab]

Yeah, the addition of new lines, whilst a valid concern, is offset by the fact that many of us have to make our terminals super wide to accommodate the long paths.

[pnkfelix]

triage: I-nominated

Nominating for discussion at compiler team meeting.

(Basically, even if I reviewed this patch and were 100% happy with its effects, I would not feel comfortable introducing these changes without first discussing with the rest of the compiler team.)

[wafflespeanut]

@pnkfelix Agreed. Also, (for reference), there were a lot of suggestions at /r/rust.

[nrc]

@wafflespeanut could you provide an example with lots of errors please? (as requested by @brson above).

[nikomatsakis]

I definitely think we should try to follow a standard of some kind for indicating the file/line-number etc :) I was under the impression that we already did, or at least I've seen similar formatting from a number of tools and emacs seems to recognize it "out of the box".

I also think we should not be afraid to change the precise formatting of our messages. I consider it unstable and targeting humans, not tools. But I guess no reason to change willy nilly either.

[pnkfelix]

@wafflespeanut just so this does not get lost in the shuffle: we need a legacy output flag; it was previously described as "a flag to preserve the old formatting", but it might be easier to think of as an output format that is:

• "stable" (i.e. forward-compatible, though preferably also somewhat extensible),
• not necessarily pretty for humans, but
• easy-to-parse for computers

[Manishearth]

I definitely think we should try to follow a standard of some kind for indicating the file/line-number etc :) I was under the impression that we already did

Yeah, the standard is file:line:col. We already follow that; so does this PR. There doesn't seem to be a fixed standard for ranges (we do file:line:col: line2:col2, waffles' PR does file:line:col -> line2:col2)

we need a legacy output flag;

Agreed. Though for tools in the long run I think we should provide structured (JSON) output or something.

/me idly proposes "spanhandler plugins"

[bors]

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

[nikomatsakis]

@Manishearth

There doesn't seem to be a fixed standard for ranges

Well, there are probably many fixed standards. The one that was cited earlier by @evincarofautumn is this GNU page, which proposes:

sourcefile:line1.column1-line2.column2: message
sourcefile:line1.column1-column2: message
sourcefile:line1-line2: message

Interestingly, we don't seem to do any of those today, and yet emacs correctly highlights regions for me, so it must understand some superset of those formats. Also, that web page is a bit weird, since it seems to indicate that line.col OR line:col is fine, implying that e.g. sourcefile:line:col-line:col: would be ok too.

[wafflespeanut]

@brson @nrc "spew of errors" ...

/home/wafflespeanut/Desktop/errors.rs: 22:13 -> 22:14 
  error: unresolved name `b`. Did you mean `d`? [E0425]
        let z = b;
                 ^
/home/wafflespeanut/Desktop/errors.rs: 22:13 -> 22:14 help: run `rustc --explain E0425` to see a detailed explanation
/home/wafflespeanut/Desktop/errors.rs: 5:9 -> 5:10 
  error: mismatched types:
 expected `Foobar`,
    found `_`
(expected struct `Foobar`,
    found integral variable) [E0308]
        a = 5;
            ^
/home/wafflespeanut/Desktop/errors.rs: 5:9 -> 5:10 help: run `rustc --explain E0308` to see a detailed explanation
/home/wafflespeanut/Desktop/errors.rs: 9:11 -> 9:17 
  error: no method named `blah` found for type `&str` in the current scope
            b.blah()
              ^~~~~~
/home/wafflespeanut/Desktop/errors.rs: 19:13 -> 19:14 
  error: mismatched types:
 expected `&str`,
    found `_`
(expected &-ptr,
    found integral variable) [E0308]
            k = 5;
                 ^
/home/wafflespeanut/Desktop/errors.rs: 19:13 -> 19:14 help: run `rustc --explain E0308` to see a detailed explanation
/home/wafflespeanut/Desktop/errors.rs: 23:22 -> 23:23 
  error: the trait `core::fmt::Debug` is not implemented for the type `core::str::Chars<'_>` [E0277]
        println!("{:?}", d);
                          ^
<std macros>: 2:25 -> 2:56 note: in this expansion of format_args!
<std macros>: 3:1 -> 3:54 note: in this expansion of print! (defined in <std macros>)
/home/wafflespeanut/Desktop/errors.rs: 23:5 -> 23:25 note: in this expansion of println! (defined in <std macros>)
/home/wafflespeanut/Desktop/errors.rs: 23:22 -> 23:23 help: run `rustc --explain E0277` to see a detailed explanation
/home/wafflespeanut/Desktop/errors.rs: 23:22 -> 23:23 note: `core::str::Chars<'_>` cannot be formatted using `:?`; if it is defined in your crate, add `#[derive(Debug)]` or manually implement it
/home/wafflespeanut/Desktop/errors.rs: 23:22 -> 23:23 note: required by `core::fmt::Debug::fmt`

  error: aborting due to 5 previous errors

(Note that there are some off-by-one errors while displaying the span, which can be fixed. Also, this applies only to errors - I haven't added newline to the help & note)

And, I agree that we need the legacy output flag. I'll update this soon :)

[eddyb]

@wafflespeanut AFAICT this just breaks error listing completely in at least the editors I use.
The file name, line numbers and error have to be on the same line.

Already type errors are almost useless since the actual type mismatch isn't part of the error message.

Maybe the "nicer" multi-line output forms should be restricted to terminal stdout?

[Manishearth]

Most editors can be fed a per-language regex for how to decode more complex data from error messages.

[eddyb]

@Manishearth how many of those work with multiple lines per error message?

[Manishearth]

No idea. I don't use shortform error listings much.

I think here you should just configure your editor to use the legacy output. Editor-friendly output is more often than not human-unfriendly; and one of the core issues this PR was trying to address was that having everything on one line is incredibly ugly and hard for humans to read on split terminals.

We should still ensure that things like jump-to-line on a full error listing work on the new system, though. (Since that's easy to get right without impacting readability)

[brson]

Interestingly, we don't seem to do any of those today, and yet emacs correctly highlights regions for me, so it must understand some superset of those formats. Also, that web page is a bit weird, since it seems to indicate that line.col OR line:col is fine, implying that e.g. sourcefile:line:col-line:col: would be ok too.

ISTR that I implemented the range output, and specifically did not follow the standards because the thing that we currently implement worked correctly with emacs whereas others I tried did not.

[nikomatsakis]

I think I am r+ on the intention here but r- on the specific details of this PR. I think we should evolve it more before we land. What I think I would really like to see (some of which is in this PR):

• not repeating the filename/line number before every line of the snippet
• visually grouping together the related errors/notes/helps in a nice way (this should be enabled by @nrc's changes to the compiler)

That said, I find the exact output here to have some shortcomings and be not quite ready to land. For example, it seems somewhat inconsistent to have error messages begin on a new line and help messages not. Also, the -> kind of looks like Rust code, and it won't work "out of the box" with emacs (or any other editor, as far as I know), so that seems like a step backwards.

TL;DR we should iterate on this more, but it's going in a good direction -- and we should take care to ensure that editors continue to work well. :)

[nrc]

We discussed this at the compiler meeting, conclusions:

• we are happy to pursue improvements to the formatting of error messages in general, and I am happy to mentor any such effort,
• we must not break editors which parse our error messages (we can't expect them to use JSON error messages like IDEs, and there is a de facto standard for error formats which we should adhere to, even if that makes error messages less ideal),
• there was some discussion about exactly what would be an improvement, it is therefore probably better to make PRs which tackle a single change at a time,
• in particular
  • grouping errors together with notes/help/etc. seems like a big win,
  • removing paths from the snippet margins probably a good thing,
  • removing line numbers from snippets is debatable,
  • using -> rather than : is a non-starter because of editors
• we don't want to take changes that are big enough (or breaking enough for editors) that require a legacy error format.

[wafflespeanut]

Awesome! Thanks :)