Turn on new errors and json mode #35401

sophiajt · 2016-08-05T23:13:43Z

This PR is a big-switch, but on a well-worn path:

Turns on new errors by default (and removes old skool)
Moves json output from behind a flag

New errors by default

The RFC for new errors landed and as part of that we wanted some bake time. It's now had a few weeks + all the time leading up to the RFC of people banging on it. We've also had editors updating to the new format and expect more to follow.

We also have an issue on old skool that needs to be fixed as more errors are switched to the new style, but it seems silly to fix old skool errors when we fully intend to throw the switch in the near future.

This makes it lean towards "why not just throw the switch now, rather than waiting a couple more weeks?" I only know of vim that wanted to try to parse the new format but were not sure how, and I think we can reach out to them and work out something in the 8 weeks before this would appear in a stable release.

JSON mode enabled

We've hashed out stabilizing JSON output, and it seems like people are relatively happy making what we have v1 and then likely adding to it in the future. The idea is that we'd maintain backward compatibility and just add new fields as needed. We'll also work on a separate output format that'd be better suited for interactive tools like IDES (since JSON message can get a little long depending on the error).

This PR stabilizes JSON mode, allowing its use without -Z unstable-options

Combined, this gives editors two ways to support errors going forward: parsing the new error format or using the JSON mode. By moving JSON to stable, we can also add support to Cargo, which plugin authors tell us does help simplify their support story.

r? @nikomatsakis
cc @rust-lang/tools

Closes #34826

GuillaumeGomez · 2016-08-06T13:44:29Z

👍

alexcrichton · 2016-08-07T00:09:00Z

src/test/run-make/unicode-input/Makefile

-all: $(DOTEST)
-
-dotest:
-	# check that we don't ICE on unicode input, issue #11178


How come this test was removed? Seems like some pieces may still be relevant?

It's redundant with the UI unicode test (which tests the same thing in a much better way)

That said, there's something odd with how unicode is handled by both new and old mode, so that's worth exploring, though that exploration shouldn't block this but rather be something on our todo list

Ok, so long as it's still being tested sounds good to me

Actually, I'm just going to file "write more unicode tests" and put it on myself. We need more than 1 or 2 anyways, which is all we really had before.

Put #35442 on myself to add a few more tests in the future.

alexcrichton · 2016-08-07T00:10:32Z

I'm in favor! In terms of a timeline this will move into Beta on 2016-08-18, two weeks from the past Friday. That gives around 8 weeks to revert this decision (if we need to), but also just more time to detect bugs.

cc @nrc and @vadimcn specifically as well

arielb1 · 2016-08-07T13:29:13Z

error[E0433]: failed to resolve. Could not find `FormatMode` in `errors::snippet`

    --> src/libsyntax/parse/lexer/mod.rs:1690:49

     |

1690 |                                                 errors::snippet::FormatMode::EnvironmentSelected);

     |                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

nikomatsakis · 2016-08-08T13:36:14Z

@jonathandturner I'm in favor of throwing the switch, but I do have a question -- I haven't looked at your new "errors" crate, how hard do you think it would be to avoid flushing output in the middle of a message (or at least in the middle of the two line header?). I think that would make emacs integration much nicer and it seems like it may help VI too, though that's more speculative.

nikomatsakis · 2016-08-08T13:40:54Z

r=me, do we want to wait to hear from anyone else? @nrc / @vadimcn ?

sophiajt · 2016-08-08T14:51:46Z

@nikomatsakis - is there a way to test the current version to see how much is buffering? Here's how I emit to the destination:

    for line in rendered_buffer {
        for part in line {
            dst.apply_style(lvl.clone(), part.style)?;
            write!(dst, "{}", part.text)?;
            dst.reset_attrs()?;
        }
        write!(dst, "\n")?;
    }

...so I'm not flushing manually. I don't know if write! flushes on carriage return, though.

alexcrichton · 2016-08-08T16:01:09Z

@jonathandturner

That unfortunately won't buffer an entire error because the default behavior is to only buffer as much that's seen until a newline, so that'd flush after each line. Also note that it's only possible to line-buffer on Unix because on Windows the current way we implement color is as a state of the console, not a property of the output stream.

All that's basically to say though that it's nontrivial and probably shouldn't happen in this PR at any rate, and otherwise we can track it over at #32486.

I think it's also ok to move forward as @nrc has been in favor on other threads (I believe) and @vadimcn's main concern was proving out editor support which @jonathandturner indicates is well underway. As a result, I'll...

@bors: r=nikomatsakis c350ec7

matklad · 2016-08-08T16:24:46Z

Thanks! Where can I see examples of this new format?

We already have a test for it, but it was added around this discussion. Has anything changed since then?

And just some general info about how IntelliJ Rust consumes compiler/cargo output:

at the moment we show compiler's text output to the user and hyperlink error messages to the locations in the file (the user can cycle through this error much like `C-x ``)
we don't show compiler errors directly in the editor, so we don't have a plan to use JSON format at the moment.

sophiajt · 2016-08-08T16:32:15Z

@matklad - You can see what the look like in the RFC. You can also play with them, if you have a build of the nightly compiler, by running the compiler with RUST_NEW_ERROR_FORMAT=true.

RUST_NEW_ERROR_FORMAT=true rustc <my file>

You can also set it in the environment.

The new format is very similar to the last time we discussed it, though with minor changes like the column separator|> now becoming |.

nrc · 2016-08-08T20:10:24Z

Sorry, I showed my approval with a GitHub emoji, it seems that is not reliable as an indicator. Anyway, I strongly approve!

nikomatsakis · 2016-08-08T22:40:00Z

That unfortunately won't buffer an entire error because the default behavior is to only buffer as much that's seen until a newline, so that'd flush after each line.

I remembered later that I had come to the conclusion that the idea of buffering was kind of too much of a hack, useful as it may be, and it'd probably be better to pursue trying to make JSON convenient.

bors · 2016-08-09T03:52:16Z

⌛ Testing commit c350ec7 with merge beb9d61...

bors · 2016-08-09T04:51:00Z

💔 Test failed - auto-win-msvc-64-cargotest

bruno-medeiros · 2016-08-09T14:06:23Z

From the point of view of RustDT, the end goal is to parse the JSON output itself, as that allows much better control of how to understand the error model and display it (particularly display it in a graphical way, in the editor itself). When I next work on this, I'd rather start supporting the JSON output straight away, not much point spending time working on parsing this next textual non-JSON output. If RustDT doesn't get there before the new output goes live, there will still be an option to display the old format, right?

nikomatsakis · 2016-08-09T14:12:14Z

@bruno-medeiros

If RustDT doesn't get there before the new output goes live, there will still be an option to display the old format, right?

This PR removes that code, actually.

sophiajt · 2016-08-09T14:22:23Z

I'd rather start supporting the JSON output straight away, not much point spending time working on parsing this next textual non-JSON output.

Agreed, that sounds like a good goal. The hope is that by enabling JSON with this PR, and giving plugin authors ~8 weeks to update to it, that there will be enough time to update before people see it in stable.

nikomatsakis · 2016-08-09T14:54:44Z

I think a big problem for JSON is the fact that cargo can't emit JSON output. That is, if I were going to integrate cargo support into emacs, I would want to parse the full cargo output. But I guess one could get started by passing RUSTFLAGS to cargo and then dealing with the mix of JSON etc that will come out.

sophiajt · 2016-08-09T14:56:43Z

@nikomatsakis - I was talking to @alexcrichton about the JSON output, and he says he's very much for cargo emitting JSON once it is stabilized. I'm going to meet with him today to see if we can switch things over.

sophiajt · 2016-08-09T17:14:46Z

@bors r+ p=1

bors · 2016-08-09T17:14:47Z

📌 Commit 9b510ba has been approved by jonathandturner

alexcrichton · 2016-08-09T20:43:30Z

Added a reference to #34826 which this closes

bors · 2016-08-09T21:04:54Z

⌛ Testing commit 9b510ba with merge 576f766...

@nikomatsakis

…jonathandturner Turn on new errors and json mode This PR is a big-switch, but on a well-worn path: * Turns on new errors by default (and removes old skool) * Moves json output from behind a flag The RFC for new errors [landed](rust-lang/rfcs#1644) and as part of that we wanted some bake time. It's now had a few weeks + all the time leading up to the RFC of people banging on it. We've also had [editors updating to the new format](https://github.com/saviorisdead/RustyCode/pull/159) and expect more to follow. We also have an [issue on old skool](#35330) that needs to be fixed as more errors are switched to the new style, but it seems silly to fix old skool errors when we fully intend to throw the switch in the near future. This makes it lean towards "why not just throw the switch now, rather than waiting a couple more weeks?" I only know of vim that wanted to try to parse the new format but were not sure how, and I think we can reach out to them and work out something in the 8 weeks before this would appear in a stable release. We've [hashed out](#35330) stabilizing JSON output, and it seems like people are relatively happy making what we have v1 and then likely adding to it in the future. The idea is that we'd maintain backward compatibility and just add new fields as needed. We'll also work on a separate output format that'd be better suited for interactive tools like IDES (since JSON message can get a little long depending on the error). This PR stabilizes JSON mode, allowing its use without `-Z unstable-options` Combined, this gives editors two ways to support errors going forward: parsing the new error format or using the JSON mode. By moving JSON to stable, we can also add support to Cargo, which plugin authors tell us does help simplify their support story. r? @nikomatsakis cc @rust-lang/tools Closes #34826

bors · 2016-08-09T23:40:39Z

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

sophiajt · 2016-08-09T23:51:19Z

@bors retry

sophiajt · 2016-08-09T23:51:37Z

@bors force

sophiajt · 2016-08-10T00:00:50Z

@bors force

(at Niko's request, though I don't think bors likes me anymore)

bors · 2016-08-10T00:00:56Z

bors · 2016-08-10T02:12:47Z

rust-highfive assigned nikomatsakis Aug 5, 2016

sophiajt changed the title ~~Turn on new errors, json mode. Remove duplicate unicode test~~ Turn on new errors and json mode Aug 5, 2016

alexcrichton reviewed Aug 7, 2016
View reviewed changes

Jonathan Turner added 2 commits August 7, 2016 07:46

Turn on new errors, json mode. Remove duplicate unicode test

fad4f32

Fix old call in lexer tests

c350ec7

sophiajt force-pushed the enable_json_and_new_errors branch from ce79a22 to c350ec7 Compare August 7, 2016 14:50

This was referenced Aug 8, 2016

Rust error format updates and IDE support PistonDevelopers/VisualRust#229

Closed

Rust error format updates and IDE support intellij-rust/intellij-rust#583

Closed

Update cargo SHA to latest cargo

9b510ba

brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 9, 2016

bors merged commit 9b510ba into rust-lang:master Aug 10, 2016

trixnz mentioned this pull request Aug 10, 2016

Add support for --error-format now that it has been stablized rust-lang/cargo#2982

Closed

futile mentioned this pull request Aug 10, 2016

Support new Rust error format flycheck/flycheck#1035

Closed

ExpHP mentioned this pull request Aug 17, 2016

Add support for new error format rust-lang/rust.vim#99

Merged

sophiajt deleted the enable_json_and_new_errors branch August 18, 2016 15:57

lambda-fairy mentioned this pull request Dec 4, 2016

Old school errors not showing labels as notes #35330

Closed

alexcrichton mentioned this pull request Feb 2, 2017

Tracking issue for JSON errors #33261

Closed

oli-obk mentioned this pull request Feb 22, 2017

Refactor suggestions into a structured form internally #39973

Closed

GuillaumeGomez mentioned this pull request Feb 6, 2018

Put back "try with rustc --explain E0XXX" #48041

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn on new errors and json mode #35401

Turn on new errors and json mode #35401

sophiajt commented Aug 5, 2016 •

edited by alexcrichton

Loading

GuillaumeGomez commented Aug 6, 2016

alexcrichton Aug 7, 2016

sophiajt Aug 7, 2016 •

edited

Loading

alexcrichton Aug 7, 2016

sophiajt Aug 7, 2016

sophiajt Aug 7, 2016

alexcrichton commented Aug 7, 2016

arielb1 commented Aug 7, 2016

nikomatsakis commented Aug 8, 2016

nikomatsakis commented Aug 8, 2016

sophiajt commented Aug 8, 2016

alexcrichton commented Aug 8, 2016

matklad commented Aug 8, 2016

sophiajt commented Aug 8, 2016

nrc commented Aug 8, 2016

nikomatsakis commented Aug 8, 2016

bors commented Aug 9, 2016

bors commented Aug 9, 2016

bruno-medeiros commented Aug 9, 2016

nikomatsakis commented Aug 9, 2016

sophiajt commented Aug 9, 2016

nikomatsakis commented Aug 9, 2016

sophiajt commented Aug 9, 2016

sophiajt commented Aug 9, 2016

bors commented Aug 9, 2016

alexcrichton commented Aug 9, 2016

bors commented Aug 9, 2016

bors commented Aug 9, 2016

sophiajt commented Aug 9, 2016

sophiajt commented Aug 9, 2016

sophiajt commented Aug 10, 2016

bors commented Aug 10, 2016

bors commented Aug 10, 2016

Turn on new errors and json mode #35401

Turn on new errors and json mode #35401

Conversation

sophiajt commented Aug 5, 2016 • edited by alexcrichton Loading

New errors by default

JSON mode enabled

GuillaumeGomez commented Aug 6, 2016

alexcrichton Aug 7, 2016

Choose a reason for hiding this comment

sophiajt Aug 7, 2016 • edited Loading

Choose a reason for hiding this comment

alexcrichton Aug 7, 2016

Choose a reason for hiding this comment

sophiajt Aug 7, 2016

Choose a reason for hiding this comment

sophiajt Aug 7, 2016

Choose a reason for hiding this comment

alexcrichton commented Aug 7, 2016

arielb1 commented Aug 7, 2016

nikomatsakis commented Aug 8, 2016

nikomatsakis commented Aug 8, 2016

sophiajt commented Aug 8, 2016

alexcrichton commented Aug 8, 2016

matklad commented Aug 8, 2016

sophiajt commented Aug 8, 2016

nrc commented Aug 8, 2016

nikomatsakis commented Aug 8, 2016

bors commented Aug 9, 2016

bors commented Aug 9, 2016

bruno-medeiros commented Aug 9, 2016

nikomatsakis commented Aug 9, 2016

sophiajt commented Aug 9, 2016

nikomatsakis commented Aug 9, 2016

sophiajt commented Aug 9, 2016

sophiajt commented Aug 9, 2016

bors commented Aug 9, 2016

alexcrichton commented Aug 9, 2016

bors commented Aug 9, 2016

bors commented Aug 9, 2016

sophiajt commented Aug 9, 2016

sophiajt commented Aug 9, 2016

sophiajt commented Aug 10, 2016

bors commented Aug 10, 2016

bors commented Aug 10, 2016

sophiajt commented Aug 5, 2016 •

edited by alexcrichton

Loading

sophiajt Aug 7, 2016 •

edited

Loading