cargo fix: tell user why some fixes aren't applied (such as maybe-incorrect)

[tklebanoff]

Problem

when I run "cargo fix", It completes with no errors, but nothing changes in the source code.

$cargo fix --allow-dirty
Checking my_program v0.1.0 (/Users/user/home/work/repo/my_program)
warning: variant ct_none should have an upper camel case name
--> src/prog/parameter.rs:14:5
|
14 | ct_none,
| ^^^^^^^ help: convert the identifier to upper camel case: CtNone
|
= note: #[warn(non_camel_case_types)] on by default

warning: variant ct_percent should have an upper camel case name
--> src/prog/parameter.rs:15:5
|
15 | ct_percent,
| ^^^^^^^^^^ help: convert the identifier to upper camel case: CtPercent

warning: variant ct_percent_bidi should have an upper camel case name
--> src/prog/parameter.rs:16:5
|
16 | ct_percent_bidi,
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: convert the identifier to upper camel case: CtPercentBidi

...

warning: union is never used: PData
--> src/prog/parameter.rs:2:7
|
2 | union PData {
| ^^^^^
|
= note: #[warn(dead_code)] on by default

warning: enum is never used: ValTypes
--> src/prog/parameter.rs:8:6
|
8 | enum ValTypes {
| ^^^^^^^^

warning: enum is never used: CtrlTypes
--> src/prog/parameter.rs:13:6
|
13 | enum CtrlTypes {
| ^^^^^^^^^

warning: enum is never used: ControlGroup
--> src/prog/parameter.rs:99:6
|
99 | enum ControlGroup
| ^^^^^^^^^^^^

warning: struct is never constructed: XIDPromise
--> src/prog/parameter.rs:125:8
|
125 | struct XIDPromise {
| ^^^^^^^^^^^^^^^^^^

warning: struct is never constructed: XIDCounter
--> src/prog/parameter.rs:130:8
|
130 | struct XIDCounter {
| ^^^^^^^^^^^^^^^^^^

warning: 94 warnings emitted

Finished dev [unoptimized + debuginfo] target(s) in 0.26s

$ git status
On branch master
Your branch is ahead of 'origin/master' by 3 commits.
(use "git push" to publish your local commits)

nothing to commit, working tree clean

^^here, I expected some sort of lint fixing to be done automatically

Steps

run "cargo fix"in a project with some non camel case values within an enum.

I was expecting this command to automatically fix the suggested lints. Instead, git status indicates no change to the project directory.
Perhaps I have the command wrong. I read the manpage for cargo fix and it seemed to indicate I had the right command.

Notes

Output of cargo version:

$ cargo version
cargo 1.48.0-nightly (126907a 2020-08-31)

$ rustc --version
rustc 1.48.0-nightly (5099914a1 2020-09-08)

OS Platform : OSX 10.15.6

Additionally, I tried cargo clippy. This command gave me the expected behavior, but not on the camelcase lints I wanted it to fix! It found a set of hygiene issues I did not previously know about! How can I get this behavior for the camelcase lints! I grepped rustc-clippy and didn't find anything geared towards these particular camelcase lints. The enums in question are ported from C and sometimes have hundreds of non camelcase elements. It would be super useful to me if there was a command which could apply the suggested lints automatically!

Any help is much appreciated! thank you!

[ehuss]

Thanks for the report!

In this particular case, the lint is marked as MaybeIncorrect, and cargo fix does not apply those automatically. I think in this case that is intentional, though I am uncertain of the exact reason. I'm guessing that it is because applying that fix would end up breaking all uses of that variant, and rustc is not built for doing refactoring of entire codebases. There are other tools that are better suited for that (I think rust-analyzer can maybe do that).

There is an issue for forcing cargo fix to apply non-MachineApplicable lints at #13023, although in this case this would result in a lot of broken code, and probably not what you want.

Perhaps cargo fix should include a message explaining why certain fixes were not applied?

[Manishearth]

@ehuss filed #13028

Would love to help folks implement this. We'd probably need to figure out a UI for it in cargo as well.

[estebank]

@ehuss part of the solution here would be to make rustc collect all the usages of that identifier and emit a tool_only suggestion for them to also change and make all of them MachineApplicable, but it might be quite hard to accomplish, depending on what stage this is in (rustc isn't well suited for "find all usages" at the moment, I think that rustdoc does its own thing that could be repurposed).

[ehuss]

I'm a little confused, as there are multiple things being discussed here, and I don't understand the tool_only comment.

For this issue, Cargo could write a note at the end that some suggestions weren't applied. It'll be a little tricky. The final messages are displayed here by running rustc. However, this "inner" cargo wrapper doesn't capture the output during this final run, so it doesn't know which suggestions were remaining. There are a few different approaches to that. One is to intercept all the output, reparse it, and catch all the suggestions, and if there are any, displaying a note that it wasn't applied. Another option is to send some signal to the "outer" cargo to do that same work. Either way probably won't be pretty.

If you are talking about adding some kind of flag to do the equivalent of __CARGO_FIX_YOLO, that's something that could be discussed, but I think would result in errors most of the time.

Or maybe you are talking about #13023 where __CARGO_FIX_YOLO would be automated in some way? That's certainly possible, but I think would be quite difficult to do in a way that would result in a good user experience. Either it'll take an extremely long time to run, or will just fail in most cases.

Or maybe you are talking about having an interactive UI to choose applying lints? Rustfix used to have that, but it was removed. We could consider that, too, but I'm wondering why someone would prefer to use that over an IDE? I would presume the majority of users are using some kind of smart editor where they can just make that choice there. I'm not saying the command-line cargo fix isn't completely unneeded, but I don't expect users to usually use it.

It would be helpful to better understand the use cases you want to address, and how common or useful we would expect those to be.

[Manishearth]

@ehuss I think the solution proposed in #13028 is quite doable:

• rustfix prints out "hey some of these could not be applied for , use --flag to force apply them"
• cargo fix has a flag that passes through. nothing fancy for cargo

The tool_lint comment is specific to this lint, whose suggestion is not machine applicable but could be made machine applicable if it were not shown to the user.

[estebank]

I don't understand the tool_only comment

I was bringing it up in the context of

collect all the usages of that identifier

to make the suggestion MachineApplicable (because it would cover everything we would need to change), but hide all of the uses to keep the output to a reasonable lenght.

[ehuss]

rustfix prints out "hey some of these could not be applied for , use --flag to force apply them"

rustfix-the-library doesn't (and can't) print anything. It would need to be done in cargo::ops::fix, and it is doable, but not easy. (Another option I didn't mention would be to have the cargo wrapper buffer all the diagnostics, and then instead of running rustc one last time, to just replay its cache. Though that seems quite intrusive for adding a suggestion.)

[Manishearth]

Ah, I see. Would be nice to have that, we could potentially signal this state through the return code idk