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

Stabilize short error format #49546

Merged
merged 2 commits into from
May 31, 2018

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Mar 31, 2018

r? @oli-obk

Added in #44636

@oli-obk
Copy link
Contributor

oli-obk commented Apr 1, 2018

Does this have a tracking issue?

@GuillaumeGomez
Copy link
Member Author

No, it's just a format I added a long time ago. I think it's time to stabilize it now.

@oli-obk oli-obk added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-diagnostics Working group: Diagnostics labels Apr 2, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Apr 2, 2018

@rfcbot fcp merge

This is a simple addition that is very useful for simple IDEs without rls support or just getting a good overview of all errors without the "noise" of details. I don't know of any open questions about it, except that some errors display very badly (e.g. mismatched types doesn't show the types). This is true for rls rendering, too, and we don't give guarantees on diagnostic output stability, so we can fix it in the future.

An output example can be found in https://github.com/GuillaumeGomez/rust/blob/3ac4f6d7a41caebdf612c079d0bf13b0f54bee74/src/test/ui/short-error-format.stderr

@rfcbot
Copy link

rfcbot commented Apr 2, 2018

Team member @oli-obk has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Apr 2, 2018
@pietroalbini pietroalbini added the relnotes Marks issues that should be documented in the release notes of the next release. label Apr 3, 2018
@nrc
Copy link
Member

nrc commented Apr 4, 2018

@rfcbot concern weight

Is this pulling its weight? Are people and/or tools using it? Do we have reports that it is satisfying their needs properly and is not just a stop gap for them?

@nrc
Copy link
Member

nrc commented Apr 4, 2018

@rfcbot concern details

Many errors don't make sense without the details (the secondary diagnostics), or at least are not very clear. Is this a problem here? Would the short form be better if it included these, but excluded the code snippets and some other stuff?

(Having registered both these concerns, my inclination is still to stabilise, however, I do want to check we're doing the right thing).

Also cc @rust-lang/dev-tools

@GuillaumeGomez
Copy link
Member Author

Is this pulling its weight? Are people and/or tools using it? Do we have reports that it is satisfying their needs properly and is not just a stop gap for them?

I know a few who are using it yes (the ones who asked for it). They use it to avoid getting huge rustc output mainly, not for tooling.

@petrochenkov
Copy link
Contributor

I tried it today and yesterday and I like it, in general.
Some errors are not adapted for this diagnostic style though and give completely useless messages like "type mismatch" without any details, but they may be improved.

@shepmaster shepmaster added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 7, 2018
@nikomatsakis
Copy link
Contributor

I have no idea what is being "stabilized" here. Surely not the text of the messages. Something about the format? If so, what? It'd be nice to see some documentation to that effect.

@nikomatsakis
Copy link
Contributor

(Personally, I feel this could have merited an RFC, also.)

@estebank
Copy link
Contributor

Do we have reports that it is satisfying their needs properly and is not just a stop gap for them?

There's a vocal small group of people that really like this feature. The increase in complexity is fairly contained.

Some errors are not adapted for this diagnostic style though and give completely useless messages like "type mismatch" without any details, but they may be improved.

I'm yet to write down a proposal pointing out (been offline for two weeks), but I want to formalize the way all errors are written to make short errors always make sense. Labels and notes should provide extra context and guidance, not be mandatory for understanding, although for the type mismatch errors we might want to wait until we have a way to use "shorter" (local namespace) type names to avoid overly long messages.

As to @nikomatsakis' comments, I feel that the improvement of the text to fit this format can be tackled progressively over time and the feature is small and obscure enough (I foresee few people to use it on a regular basis, and the ones that do are more likely to accept the growing pains until we improve the messages) to not be a problem to stabilize it as presented.

@Manishearth
Copy link
Member

Perhaps we can add a ~/.cargo/config key for this? You can already do this as a rustflags key but a dedicated key should be nice.

IMO folks should not be interacting with Rust flags directly as much as possible.

@pietroalbini
Copy link
Member

Ping from triage @rust-lang/compiler! How's the discussion going for this?

@nikomatsakis
Copy link
Contributor

@estebank

As to @nikomatsakis' comments, I feel that the improvement of the text to fit this format can be tackled progressively over time and the feature is small and obscure enough (I foresee few people to use it on a regular basis, and the ones that do are more likely to accept the growing pains until we improve the messages) to not be a problem to stabilize it as presented.

I still do not know what we are stabilizing! Can somebody please write that up? All I see:

r? @oli-obk
Added in #44636

I would like to see something like the report in this issue:

#48854 (comment)

It doesn't have to be long, just say what is being stabilized and not. =)

@nikomatsakis
Copy link
Contributor

Also, how it is tested =)

@nikomatsakis
Copy link
Contributor

Some more discussion on IRC here. Specifically, I would like to know:

  • We are stabilizing some kind of flag that selects this output. What is that flag? Is there an environment variable? How does it work?
  • Are we stabilizing anything about the output? The fact that it is one line per error? A regex that will match to extract filename/line number? Nothing at all?

Presumably we are not stabilizing the specific text of error messages, which I guess we would never do.

Is there documentation, or a plan to document? Are there tests?

@nikomatsakis
Copy link
Contributor

@rfcbot concern what-is-being-stabilized

As noted here, I don't really know what we are discussing yet and would like to see some documentation of it =)

@QuietMisdreavus
Copy link
Member

QuietMisdreavus commented Apr 26, 2018

Here's what i know about this feature. @GuillaumeGomez can fill in more specifics if i'm missing something.

  • This adds a new option to the --error-format flag: short.
  • Passing --error-format short to rustc is like --error-format human, but it will omit everything but the "first line" of an error. Help messages, span displays, supplementary info, things like that. When rustc emits an error under this format, it only prints the file, line, column, and the initial error message.
  • Right now, before this PR, if you wanted to pass --error-format short, you also needed to pass -Z unstable-options. The flag was made unstable since it was a new implementation, in case we wanted to change how it was named or something significant about its implementation or something like that. The unstable requirement was added in Add short error message-format #44636. This PR removes that.
  • When the flag was initially implemented, i opened a PR to Cargo to add this option to their --message-format flag to match, but it was closed since the option was so new and needed to be marked unstable. Presumably, if this PR is merged and the unstable requirement is lifted, that PR can be reopened.

If there are other implications to adding an error format, i haven't considered them. I personally think it's harmless to add, but i also didn't write it. 😁

EDIT: For an example of how this looks, there's a UI test that shows it off.

@nikomatsakis
Copy link
Contributor

So I would specifically like to know if we are committing to:

file.rs:LL:CC - error[E0308]: XXX

I think we should consider adopting one of the standard gnu formats, which I think we almost do, except that we should say file:LL:CC:.

@nikomatsakis
Copy link
Contributor

@rfcbot resolved what-is-being-stabilized

thanks @QuietMisdreavus !

@nikomatsakis
Copy link
Contributor

@rfcbot concern gnu-compatible

I think the output should be compatible with the standard gnu formats, which I think we almost do, except that we should say file:LL:CC: (i.e., colon after the column number). Not sure what happens for more complex line/column information.

@nikomatsakis
Copy link
Contributor

@rfcbot resolve what-is-being-stabilized

@oli-obk
Copy link
Contributor

oli-obk commented May 31, 2018

@bors r+

@bors
Copy link
Contributor

bors commented May 31, 2018

📌 Commit 0be4cb9 has been approved by oli-obk

@bors bors 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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels May 31, 2018
@bors
Copy link
Contributor

bors commented May 31, 2018

🔒 Merge conflict

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 31, 2018
@GuillaumeGomez
Copy link
Member Author

Didn't have merge conflict apparently but rebased just in case...

@GuillaumeGomez
Copy link
Member Author

@bors: r=oli-obk

@bors
Copy link
Contributor

bors commented May 31, 2018

📌 Commit 426b63f has been approved by oli-obk

@bors bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 31, 2018
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
bors added a commit that referenced this pull request May 31, 2018
Rollup of 7 pull requests

Successful merges:

 - #49546 (Stabilize short error format)
 - #51123 (Update build instructions)
 - #51146 (typeck: Do not pass the field check on field error)
 - #51193 (Fixes some style issues in rustdoc "implementations on Foreign types")
 - #51213 (fs: copy: Use File::set_permissions instead of fs::set_permissions)
 - #51227 (mod.rs isn't beautiful)
 - #51240 (Two minor parsing tweaks)

Failed merges:
@bors
Copy link
Contributor

bors commented May 31, 2018

⌛ Testing commit 426b63f with merge 1ffb321...

@bors bors merged commit 426b63f into rust-lang:master May 31, 2018
@GuillaumeGomez GuillaumeGomez deleted the stabilize-short-error-format branch May 31, 2018 23:25
@bors
Copy link
Contributor

bors commented Jun 1, 2018

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 1, 2018
@nagisa
Copy link
Member

nagisa commented Jun 1, 2018 via email

@Manishearth
Copy link
Member

Manishearth commented Jun 1, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-diagnostics Working group: Diagnostics
Projects
None yet
Development

Successfully merging this pull request may close these issues.