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

Highlight when errors have automatically applicable suggestions #59103

Open
davidtwco opened this issue Mar 11, 2019 · 18 comments
Open

Highlight when errors have automatically applicable suggestions #59103

davidtwco opened this issue Mar 11, 2019 · 18 comments
Labels
A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@davidtwco
Copy link
Member

With tool_only_span_diagnostics having landed, some suggestions that can be automatically applied aren't shown to the user as labels with help: ... messages, so users who look for that to know they can automatically apply the fix won't know about those suggestions.

We should add some indicator that tells the user that an error can be automatically fixed.

See this conversation.

@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. labels Mar 11, 2019
@estebank
Copy link
Contributor

I believe some iteration of the following would be appropriate:

error[E0252]*: the name `X` is defined multiple times
  --> $DIR/double-type-import.rs:3:9
   |
LL |     pub use self::bar::X;
   |             ------------ previous import of the type `X` here
LL |     use self::bar::X;
   |         ^^^^^^^^^^^^ `X` reimported here
   |
   = note: `X` must be defined only once in the type namespace of this module

error: aborting due to previous error

For more information about this error, try `rustc --explain E0252`.
* `rustfix` applicable suggestion is available for this error

With proper coloring, this should be easy to follow.

Does rustfix apply MaybeIncorrect suggestions? If not, this should be only included for MachineApplicable suggestions.

CC @killercup

@killercup
Copy link
Member

killercup commented Mar 11, 2019 via email

@estebank
Copy link
Contributor

In that case, we should only provide the asterisk (or whatever we land on) for MachineApplicable suggestions.

The position of the asterisk and the last line text is bikeshedable to death:

error[E0252]*: the name `X` is defined multiple times
*error[E0252]: the name `X` is defined multiple times
error*[E0252]: the name `X` is defined multiple times
error[E0252]: the name `X` is defined multiple times*
error[E0252]:* the name `X` is defined multiple times

@davidtwco
Copy link
Member Author

I'll kick off the bikeshed by saying that I prefer the fourth option.

@killercup
Copy link
Member

+1 this comment if you think 1st option is best option

@estebank
Copy link
Contributor

estebank commented Mar 15, 2019

I personally feel that all the presented options are suboptimal, but I don't care enough to fight too much. I think overall that either 1 or 4 lead the pack. Vote on the individual comments below for your preference.

@estebank
Copy link
Contributor

error[E0252]*: the name `X` is defined multiple times

@estebank
Copy link
Contributor

*error[E0252]: the name `X` is defined multiple times

@estebank
Copy link
Contributor

error*[E0252]: the name `X` is defined multiple times

@estebank
Copy link
Contributor

error[E0252]: the name `X` is defined multiple times*

@estebank
Copy link
Contributor

error[E0252]:* the name `X` is defined multiple times

@estebank
Copy link
Contributor

error[E0252*]: the name `X` is defined multiple times

@estebank
Copy link
Contributor

bikeshed-signal to @rust-lang/wg-diagnostics

@oli-obk
Copy link
Contributor

oli-obk commented Mar 15, 2019

Won't that asterisk (or any other symbol) just be very confusing if you don't already know what to look for? I'd prefer a nonsigil solution, so something like

autofixable error[E0252]: the name `X` is defined multiple times

@oli-obk
Copy link
Contributor

oli-obk commented Mar 15, 2019

or

error[E0252] (autofixable): the name `X` is defined multiple times

@oli-obk
Copy link
Contributor

oli-obk commented Mar 15, 2019

or

error[E0252]: the name `X` is defined multiple times (automatically fixable)

@davidtwco
Copy link
Member Author

Perhaps if --teach is provided (or something similar to that) then we could add a (* means that the error is automatically fixable) somewhere (or similar). Otherwise I'd be concerned that it will just be a lot of noise/clutter for those who do know what it means.

@tesuji
Copy link
Contributor

tesuji commented Mar 15, 2019

s/autofixable/rustfix/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants