Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add short error message-format #44636

Merged
merged 2 commits into from
Oct 25, 2017
Merged

Conversation

GuillaumeGomez
Copy link
Member

Fixes #42653.

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

@eddyb
Copy link
Member

eddyb commented Sep 16, 2017

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned eddyb Sep 16, 2017
@GuillaumeGomez GuillaumeGomez changed the title Add short message-format Add short error message-format Sep 17, 2017
@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2017
@nrc
Copy link
Member

nrc commented Sep 19, 2017

Rather than adding a new emitter, could you have the human-readable emitter take an argument for detail level - either short or verbose? It seems that we'd save a lot of code dup (and a macro) that way.

Could you put this behind a feature gate?

@GuillaumeGomez
Copy link
Member Author

Yes and yes. I was just thinking that we'd want to fully split both of the features but if not...

@GuillaumeGomez GuillaumeGomez force-pushed the little-error-msg branch 6 times, most recently from 8468a5f to a58becd Compare September 19, 2017 21:58
@GuillaumeGomez
Copy link
Member Author

Ok, now just remain to put it behind a feature flag.

@kennytm
Copy link
Member

kennytm commented Sep 20, 2017

Is it possible to make each error only occupy one line, similar to clang -fno-caret-diagnostics?

Mock up:

$DIR/short-error-format.rs:16:9: error[E0308]: mismatched types
$DIR/short-error-format.rs:18:7: error[E0599]: no method named `salut` found for type `u32` in the current scope
error: aborting due to 2 previous errors 

@eddyb
Copy link
Member

eddyb commented Sep 20, 2017

@kenny mismatched types is the worst error in the entire compiler, if you can't see the detail lines. I know it looks pretty but it has screwed over pretty much everyone who doesn't look at the console, including the official RLS extension for VS Code.
I only recently pulled off a hack to have the information available to kate in a manageable way.

@kennytm
Copy link
Member

kennytm commented Sep 20, 2017

@eddyb That sample is referring to the UI test in this PR https://github.com/GuillaumeGomez/rust/blob/a58becd60d7d7a965ed246cf8e290e2bd94043b9/src/test/ui/short-error-format.stderr:

error[E0308]: mismatched types
  --> $DIR/short-error-format.rs:16:9

error[E0599]: no method named `salut` found for type `u32` in the current scope
  --> $DIR/short-error-format.rs:18:7

error: aborting due to 2 previous errors

Improving E0308 could happen independent of this PR.

@eddyb
Copy link
Member

eddyb commented Sep 20, 2017

Oh yeah, in that case, I agree, the example output in #44636 (comment) is a subset of the lines I have from my errfilt conversion, but I also have labels/notes (within some limitations), e.g.:

src/librustc_trans/mir/lvalue.rs:97:62: error[E0308]: mismatched types
src/librustc_trans/mir/lvalue.rs:97:62: expected struct `rustc::ty::layout::FullLayout`, found enum `rustc::mir::tcx::LvalueTy`
src/librustc_trans/mir/lvalue.rs:97:62: note: expected type `rustc::ty::layout::FullLayout<'_>`
src/librustc_trans/mir/lvalue.rs:97:62: found type `rustc::mir::tcx::LvalueTy<'_>`

@GuillaumeGomez
Copy link
Member Author

So which output do you want? So I can do it once and for all. :)

@GuillaumeGomez GuillaumeGomez force-pushed the little-error-msg branch 3 times, most recently from d598684 to 30128d6 Compare September 21, 2017 17:47
@estebank
Copy link
Contributor

I feel if the idea is to use as little vertical space the output should be closer to:

$DIR/short-error-format.rs:16:9 - error[E0308]: mismatched types
$DIR/short-error-format.rs:18:7 - error[E0599]: no method named `salut` found for type `u32` in the current scope

error: aborting due to 2 previous errors

Beyond that, I wonder wether it'd be worth it to try to keep in the diagnostic machinery space for custom single line error messages that would work better for short output (and for text editors). May be I'll just go ahead and introduce it for while and see how much of a pain it becomes. If it is too much, we can rip it out. I do feel strongly against improving things for text editors in detriment of cli output.

@GuillaumeGomez
Copy link
Member Author

New output available!

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

Some nitpicks, but I'm ok with the PR. Leaving it to @nrc / @eddyb to give final r+.


None => ErrorOutputType::HumanReadable(color),

Some(arg) => {
early_error(ErrorOutputType::HumanReadable(color),
&format!("argument for --error-format must be human or json (instead \
was `{}`)",
&format!("argument for --error-format must be `human` or `json` or \
Copy link
Contributor

Choose a reason for hiding this comment

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

human, json or short

@@ -833,9 +841,13 @@ pub fn early_error(output: config::ErrorOutputType, msg: &str) -> ! {
let emitter: Box<Emitter> = match output {
config::ErrorOutputType::HumanReadable(color_config) => {
Box::new(EmitterWriter::stderr(color_config,
None))
None,
false))
Copy link
Contributor

Choose a reason for hiding this comment

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

Fits in single line?

@@ -846,9 +858,13 @@ pub fn early_warn(output: config::ErrorOutputType, msg: &str) {
let emitter: Box<Emitter> = match output {
config::ErrorOutputType::HumanReadable(color_config) => {
Box::new(EmitterWriter::stderr(color_config,
None))
None,
false))
Copy link
Contributor

Choose a reason for hiding this comment

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

Fits in single line?

}
(config::ErrorOutputType::HumanReadable(_), Some(dst)) => {
Box::new(EmitterWriter::new(dst,
Some(codemap.clone())))
Some(codemap.clone()),
false))
Copy link
Contributor

Choose a reason for hiding this comment

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

Fits in single line?

@GuillaumeGomez
Copy link
Member Author

The branch has been created on rustfmt so I point to it instead.

@GuillaumeGomez
Copy link
Member Author

We now have the following error:

[00:00:36] Submodule 'src/tools/rustfmt' (https://github.com/rust-lang-nursery/rustfmt.git) unregistered for path 'src/tools/rustfmt'
[00:00:36] Submodule 'src/tools/rustfmt' (https://github.com/rust-lang-nursery/rustfmt.git) registered for path 'src/tools/rustfmt'
[00:00:37] error: Server does not allow request for unadvertised object fc2c85d423a34086243f019c15e2d9cc2afcbb8f
[00:00:37] Fetched in submodule path 'src/tools/rustfmt', but it did not contain fc2c85d423a34086243f019c15e2d9cc2afcbb8f. Direct fetching of that commit failed.

Anyone has an idea?

@topecongiro
Copy link
Contributor

@GuillaumeGomez Could you please use e35e27659dec7a70d1e0e45be2c5f3a02b2c742c? I think I have erased fc2c85d423a34086243f019c15e2d9cc2afcbb8f while I pushed the update :/ I am sorry.

@GuillaumeGomez
Copy link
Member Author

Ok, let's hope it'll be the good one this time. :)

@GuillaumeGomez
Copy link
Member Author

New error and it doesn't get any better:

[00:02:06] extracting /checkout/obj/build/cache/2017-08-29/cargo-beta-x86_64-unknown-linux-gnu.tar.gz
[00:02:06] error: failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
[00:02:06]
[00:02:06] Caused by:
[00:02:06] patch for `rustfmt-nightly` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates
[00:02:06] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:06] Build completed unsuccessfully in 0:00:46

Anyone has an idea?

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 24, 2017
@GuillaumeGomez
Copy link
Member Author

Fixed.

@michaelwoerister
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 25, 2017

📌 Commit b46c014 has been approved by michaelwoerister

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 25, 2017
@bors
Copy link
Contributor

bors commented Oct 25, 2017

⌛ Testing commit b46c014 with merge f9d2416...

bors added a commit that referenced this pull request Oct 25, 2017
…ister

Add short error message-format

Fixes #42653.
@bors
Copy link
Contributor

bors commented Oct 25, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing f9d2416 to master...

@bors bors merged commit b46c014 into rust-lang:master Oct 25, 2017
@GuillaumeGomez GuillaumeGomez deleted the little-error-msg branch October 26, 2017 08:30
@alexcrichton
Copy link
Member

@nrc @GuillaumeGomez this landed without a feature flag, was that intentional? I assume not given #44636 (comment)?

@nrc
Copy link
Member

nrc commented Nov 15, 2017

I suppose this should be behind -Zunstable-features given that comment. I filed #45995

Thanks for noticing @alexcrichton !

@GuillaumeGomez
Copy link
Member Author

Hum indeed, it should had have a feature flag. I'll add it as soon as possible.

@vitiral
Copy link
Contributor

vitiral commented Feb 1, 2018

❤️ Thanks for fixing the bug I reported, looking forward to using this when it becomes available!

@vitiral
Copy link
Contributor

vitiral commented Feb 1, 2018

On this comment my preference would be something like:

src/librustc_trans/mir/lvalue.rs:97:62: error[E0308]: mismatched types. Expected struct `rustc::ty::layout::FullLayout`, found enum `rustc::mir::tcx::LvalueTy`

It's all in one line but that's probably okay. That's just, like, my opinion though man.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 31, 2018
…r-format, r=oli-obk

Stabilize short error format

r? @oli-obk

Added in rust-lang#44636
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.