Output which errors are automatically correctable, to improve discoverability of cargo fix and cargo clippy --fix

[jgpaiva]

Problem

cargo fix and cargo clippy --fix can be great tools for sparing developer cycles, especially when a new version of cargo is released. However, the discoverability of these tools is a problem: not everyone knows they exist.

Proposed Solution

To improve discoverability, we could do something similar to what rubocop does: each error could include info about whether it's automatically correctable or not. This may prompt users who don't know about this feature to go find it. It also helps users who do know about the feature: they can avoid running the fix command in situations where it'll be useless because the tool actually can't automatically fix those particular errors.

For reference, here's a screenshot of rubocop's output:

Notes

No response

[jyn514]

This information is already available in the JSON output:

// foo.rs
#[derive(Debug(x))]
struct Foo;

; rustc foo.rs --crate-type lib
error: traits in `#[derive(...)]` don't accept arguments
 --> foo.rs:1:15
  |
1 | #[derive(Debug(x))]
  |               ^^^ help: remove the arguments
; rustc foo.rs --crate-type lib --error-format json 2>&1 | jq .children[].spans[].suggestion_applicability
"MachineApplicable"

[jyn514]

@epage this isn't unique to clippy, it's useful for normal check lints and errors too.

[epage]

@jyn514 yes, rather than tagging every command that can output warnings, I just tagged the ones that the suggestions would be for.

[epage]

To improve discoverability, we could do something similar to what rubocop does: each error could include info about whether it's automatically correctable or not.

Another approach is the one I took in cargo upgrade. I report the status of every dependency but if a dependency is held back and needs a flag to be upgraded, I track that and report it at the end.

In this case, it'd be a note like

note: Run `cargo --fix` to automatically address some of the above.

EDIT: The challenge with this is it violates layering / separation of concerns. cargo doesn't know about these messages to put the suggestion in and the commands don't know about cargo to do so.

[jyn514]

EDIT: The challenge with this is it violates layering / separation of concerns. cargo doesn't know about these messages to put the suggestion in and the commands don't know about cargo to do so.

I don't understand the problem? Cargo already knows about these suggestions because they're present in the JSON output from rustc. Cargo already parses those messages to know how many errors were present.

[jyn514]

Here's an example of a similar change in the past: #9655

[epage]

Oh, great! I had missed the intent of your previous message

[Muscraft]

This seems interesting, I'd love to work on it if we can figure out what the message should be. I lean towards an explicit callout to cargo --fix but I could see just having how many are fixable.

[jyn514]

maybe something like this?

; cargo check
warning: unused import: `::r#unsafe`
 --> src/lib.rs:1:5
  |
1 | use ::r#unsafe;
  |     ^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

warning: `test-cargo` (lib) generated 1 warning (1 automatically fixable)
help: use `cargo fix` to automatically fix warnings

and only display the help message if there's at least one automatically fixable warning.

[epage]

I was thinking to keep things more targeted

; cargo check
warning: unused import: `::r#unsafe`
 --> src/lib.rs:1:5
  |
1 | use ::r#unsafe;
  |     ^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

warning: `test-cargo` (lib) generated 1 warning
note: to fix 1 warning automatically, run `cargo fix`

• atm cargo doesn't have help though it could be added, so I've been using note: for similar.
• by building the number into the note it makes the message more direct / succinct which makes it easier to scan.

On a related note, I've been meaning to bring up if cargo should align with rustc in its error style guide

[jyn514]

@epage what do you think should happen for errors that can be fixed automatically? maybe something like this?

note: to fix 1 warning and 1 error automatically, run `cargo fix`

[epage]

Seems reasonable. I can't remember which order the compiler does it but I'm assuming errors would go before warnings.

[epage]

Some things for us to keep in mind, depending on what layer this is put in at

• If its in code shared with cargo install, then we need to not show this message for ephemeral workspaces
• If its in code shared between cargo check and cargo clippy, then we'll need to customize the message for the given command.

[epage]

Oh right, clippy is external. Instead, we'll want to create a separate issue in its repo: https://github.com/rust-lang/rust-clippy/issues

[jyn514]

@epage hmm, I've forgotten how the cargo-clippy wrapper works under the hood - does it call cargo? I wonder if making this work for check would automatically get it working for clippy

I think it uses RUSTC_WORKSPACE_WRAPPER?

[Muscraft]

How I have it currently working doesn't work for clippy as its a subcommand

[epage]

from @ehuss in #10989

I'm reluctant to add something like this for a few reasons:

• I never run cargo fix. Something like this will just be constant noise for me. I think it is good to be very cautious about being too verbose or too prodding.

• cargo fix often won't work correctly if there are any errors in the code.

• cargo fix won't work with changes in the user's repo. They have to run with --allow-dirty and/or --allow-staged, which can be awkward. Also, there is a moderate risk that cargo fix will damage or otherwise mess up the user's code with no way to revert (the reason --allow-dirty is there). If there is no git repo, then it is doubly more dangerous. Or if the files haven't been added yet, cargo fix may overwrite them without confirmation.

• Even filtering on machine-fixable errors, it is moderately common that the fix fails for one reason or another. I'm not particularly confident in pushing large numbers of people to use it more often. (Minor concern.)

• This doesn't really make it clear to the user which issues will be fixed.

I appreciate it can be difficult to educate people on the existence of tools or options for performing certain tasks. I'm wondering, though, does rust-analyzer not show a quick-fix for fixable suggestions? Perhaps that would be a better route to make it easier for people to apply suggestions, or otherwise have easy access to it?

[epage]

I never run cargo fix. Something like this will just be constant noise for me. I think it is good to be very cautious about being too verbose or too prodding.

@ehuss I want to contrast this with me who nevers use cargo fix because it never crosses my mind. Even if I did, I probably wouldn't want to do the mental calculus to see if its worth running. Adding this would be a big help to me and, I suspect, to a lot of new users.

As for balancing noise vs help, we intentionally made it so we only emit 1 brief line for the entire run because of this. I personally suspect this one line won't be an issue

cargo fix often won't work correctly if there are any errors in the code.

Maybe we should limit the message to only when there are fixable errors warnings?

cargo fix won't work with changes in the user's repo

To me, this just means we evaluate the behavior between cargo fix and cargo fix --allow-dirty to see which holistic solution is better for being the default recommended command.

Also, there is a moderate risk that cargo fix will damage or otherwise mess up the user's code with no way to revert (the reason --allow-dirty is there). I

I think a --allow-dirty flag, whether we suggest it or not, will give users enough of a hint about this. Also, by having increased visibility of the feature, we'll likely get more bug reports so we'll improve the behavior.

Even filtering on machine-fixable errors, it is moderately common that the fix fails for one reason or another. I'm not particularly confident in pushing large numbers of people to use it more often. (Minor concern.)

ditto about this just being an opportunity to improve it.

This doesn't really make it clear to the user which issues will be fixed.

As a user, I don't care. I'll run the command and get things fixed and then address the rest. I'll be elated to not have to manually update 200+ warnings that I sometimes get.

I appreciate it can be difficult to educate people on the existence of tools or options for performing certain tasks. I'm wondering, though, does rust-analyzer not show a quick-fix for fixable suggestions? Perhaps that would be a better route to make it easier for people to apply suggestions, or otherwise have easy access to it?

I at least do not use rust-analyzer and punting to rust-analyzer is a slipper slope (we don't need cargo add because rust-analyzer should do it, etc).

[ehuss]

As for balancing noise vs help, we intentionally made it so we only emit 1 brief line for the entire run because of this. I personally suspect this one line won't be an issue

For a single error message, an additional line can increase the amount of output by 10%. An individual addition like this isn't likely to be "too verbose" on its own, but the aggregate of all notes, hints, descriptions can eventually get to the point where the user is exposed to too much information to be overwhelming.

Just as a thought, would it work to extend the existing summary line, maybe something like this?

warning: `foo` (lib) generated 1 warning (run `cargo fix` to apply suggestions)

Maybe we should limit the message to only when there are fixable errors?

I think the only safe thing that can be done right now is to only suggest when there are 0 total errors. cargo fix would need some work to make it suitable for running with errors.

I think it would also need to be restricted to is_local() crates.

I'm also concerned that the suggestion of running cargo fix can be misguiding if the diagnostics show up in a dependency in a workspace. Running cargo fix won't fix those issues (needing the user to run with --workspace or -p). I'm not suggesting that the note should provide details on how to deal with all of this; but I am pointing out that there are many ways where the suggestion won't actually work without the user needing to figure things out on their own.

I think a --allow-dirty flag, whether we suggest it or not, will give users enough of a hint about this. Also, by having increased visibility of the feature, we'll likely get more bug reports so we'll improve the behavior.

My concern is about the overall user experience as it would exist today. For example:

1. User gets note to run cargo fix
2. User runs cargo fix, gets the big scary error that they have uncommitted changes and "destructive changes". No guidance is given as to how to decide to proceed.
3. User regardless proceeds, it updates the code (hopefully successfully).
4. Hopefully the user's editor has change notifications, which triggers a reload. Hopefully they didn't have any unsaved changes, otherwise they may be lost.

There are other concerns, like cargo fix will apply fixes to all targets, which might make modifications to files (like examples) that the user is not even working on.

I think encouraging people to use something that does not have a good user experience should be done with caution. I don't feel like there is a need for more bug reports; there are plenty of known deficiencies with cargo fix.

[jyn514]

I'm also concerned that the suggestion of running cargo fix can be misguiding if the diagnostics show up in a dependency in a workspace. Running cargo fix won't fix those issues (needing the user to run with --workspace or -p). I'm not suggesting that the note should provide details on how to deal with all of this; but I am pointing out that there are many ways where the suggestion won't actually work without the user needing to figure things out on their own.

hmm, having the note suggest -p seems fine to me? It should be unambiguous which crate to suggest, since cargo reports that summary line one crate at a time (cc #8749).

There are other concerns, like cargo fix will apply fixes to all targets, which might make modifications to files (like examples) that the user is not even working on.

That would be fixed by making the note slightly smarter :) suggesting -p like you mentioned above, and adding --lib or --tests as necessary to make it more precise. This is all info cargo needs to have anyway to check the crate.

I don't feel like there is a need for more bug reports; there are plenty of known deficiencies with cargo fix.

Hmm, I think this is talking about slightly different bugs from what you mentioned originally:

Even filtering on machine-fixable errors, it is moderately common that the fix fails for one reason or another. I'm not particularly confident in pushing large numbers of people to use it more often. (Minor concern.)

Those are usually bugs in the compiler emitting the lint, not rustfix itself, and my impression is that both T-clippy and T-compiler are good about fixing broken suggestions pretty quickly :)

There are certainly bugs in rustfix itself, but they're predictable (not fixing rustdoc lints, multi-span suggestions, path dependencies) and we can avoid suggesting cargo fix for those. If you think that's too much special casing, I'm happy to consider this blocked on those bugs :) rust-lang/rustfix#141, #13022, #13025.

I agree that having rustfix immediately say "refusing to update files because you have changes" is not a great UX, but I do want to mention there are cases you'd want to use rustfix without making changes, for example when updating your toolchain and fixing clippy lints (which was the original motivation for @jgpaiva opening this issue).

[ehuss]

Those are usually bugs in the compiler emitting the lint, not rustfix itself, and my impression is that both T-clippy and T-compiler are good about fixing broken suggestions pretty quickly :)

My impression is not the same. There are plenty of issues about bad MachineApplicable suggestions that have been open for many years. I don't consider that by itself a blocker, just that it is a contribution to the overall possibly poor user experience.

[epage]

Hopefully the user's editor has change notifications, which triggers a reload. Hopefully they didn't have any unsaved changes, otherwise they may be lost.

They ran a command with the intention of fixing their source. I don't see users would then be surprised that it modified their code.

User runs cargo fix, gets the big scary error that they have uncommitted changes and "destructive changes". No guidance is given as to how to decide to proceed.

Personally, I wonder if we should make --allow-dirty the default and deprecate it.

People had made the opposite suggestion in the thread for merging cargo upgrade into cargo and my reasons for not doing that I think equally apply. This is a command to be run in an imperfect state, typically from there being edits made. This just adds an extra speed bump to that which can be frustrating as a user. The edition upgrade workflow had so many speedbumps and confusing points that I just gave up on it after a couple crates in 2018 and haven't used it since, and this is one of them.

I don't consider that by itself a blocker, just that it is a contribution to the overall possibly poor user experience.

People only rarely running cargo fix is a sure way of it never getting fixed.

[Muscraft]

It was discussed in a recent cargo team meeting to move forward with showing a message that cargo fix can fix some warnings but only on nightly.

Current format:

warning: `foo` (lib) generated 1 warning (run `cargo fix --lib -p foo` to apply 1 suggestion)

Current implementation from #10989:

• Shown on nightly only to allow iterative (potentially breaking) updates driven by get user feedback
• Shown only on warnings as there are problems with cargo fix and errors
• Shown in the summary line to not add extra output lines
• The command is shown on a per target basis
• The exact command to run is shown (minus version control specific flags)

Open questions:

• Should the message stay in the summary line or be on its own line?
  • The summary line can be very long with the current implementation
• Should the command be specific or genera?
• Should the command to run be per target or shown once at the end?
• Should version control specific flags be added to the command to run?

[Alexendoo]

Running cargo clippy will also emit this message, but as it's suggesting a plain cargo fix it won't apply the suggestions

It would be great it if could suggest running cargo clippy --fix, but if a way to do that hasn't been decided yet it's probably best to omit the message when either RUSTC_WRAPPER or RUSTC_WORKSPACE_WRAPPER are set