rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.9k stars 12.68k forks source link

Highlight when errors have automatically applicable suggestions #59103

Open davidtwco opened 5 years ago

davidtwco commented 5 years ago

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.

estebank commented 5 years ago

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 commented 5 years ago

rustfix only applies MachineApplicable suggestions, because… it's a machine. Yeah, that's why, totally not to masquerade its AI superpowers or anything.

On Mon, 11 Mar 2019, 17:59 Esteban Kuber, notifications@github.com wrote:

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 https://github.com/killercup

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/59103#issuecomment-471627850, or mute the thread https://github.com/notifications/unsubscribe-auth/AABOX35DahZBIZOanQ65ILnYVd1Xa36eks5vVouOgaJpZM4bo0p8 .

estebank commented 5 years ago

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 commented 5 years ago

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

killercup commented 5 years ago

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

estebank commented 5 years ago

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 commented 5 years ago
error[E0252]*: the name `X` is defined multiple times
estebank commented 5 years ago
*error[E0252]: the name `X` is defined multiple times
estebank commented 5 years ago
error*[E0252]: the name `X` is defined multiple times
estebank commented 5 years ago
error[E0252]: the name `X` is defined multiple times*
estebank commented 5 years ago
error[E0252]:* the name `X` is defined multiple times
estebank commented 5 years ago
error[E0252*]: the name `X` is defined multiple times
estebank commented 5 years ago

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

oli-obk commented 5 years ago

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 commented 5 years ago

or

error[E0252] (autofixable): the name `X` is defined multiple times
oli-obk commented 5 years ago

or

error[E0252]: the name `X` is defined multiple times (automatically fixable)
davidtwco commented 5 years ago

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 commented 5 years ago

s/autofixable/rustfix/