cargo: `cargo fix`: Improve handling of MaybeIncorrect suggestions

Ideally, we should provide a mode where you can apply them anyway (cargo clippy --fix --apply-all-suggestions?),

This exists today as __CARGO_FIX_YOLO=1. It would be great to make that more discoverable with a CLI option.

All the kids are saying it

@camsteffen It lets users set some minimum applicability level with rustfix, instead of going with the default.

The way I see it, the applicabilities go in order of least to most applicable:

• HasPlaceholders
• MaybeIncorrect (may change behavior)
• MaybeIncorrect (may introduce error)
• MachineApplicable

For example, quite often with the latter two I’m actually comfortable running rustfix on my codebase, with the former two I kinda want to review them case by case.

Maybe instead we could add a “incorrect reason” field.

That also sounds reasonable to me. (Edit: no pun intended)

or apply them using a “pick and choose” mode

Especially this reason could be used in such a pick and choose prompt.

This suggestion was not automatically applied, because <reason>
Do you want to apply this suggestion anyway? (y/N)

The problem is that MaybeIncorrect is often not stuff that is incorrect because it won’t compile, it’s incorrect because it changed the semantics of the program in a way clippy thinks is according to intention but might not be

In Clippy MaybeIncorrect is used interchangeable for both cases. Sometimes we use it, if the suggestion might result in new errors (e.g. the borrow checker might complain or an additional (trait) import is needed). And sometimes, because it’s a semantic change (e.g. removes a possibly panicking expression). At least, that’s how I use it and recommend it in reviews.

So maybe splitting it up in MayIntroduceNewError and SemanticChange (with possibly better names) would make sense.

The idea behind splitting is that MayNotCompile suggestions may compile, and guarantee not to change behavior if they do. So a tool like rust fix could apply them, check compiling the result and roll them back on failure. On the other hand MayChangeBehavior have no such guarantee.

Changing the names will help us keep track of which suggestions have been vetted.

I don’t think rust-analyzer cares too much about this, I can’t think of a direct use case for us here aside from maybe grouping quickfixes of these two kinds differently (r-a is doing a terrible job at categorizing quick fixes currently), as for both we’d want to show the user a quickfix in the UI (as we do with MaybeIncorrect in general right now).

Yeah I guess that r-a isn’t batching things the way rustfix does so it’s all user-driven anyway.

The idea behind splitting is that MayNotCompile suggestions may compile, and guarantee not to change behavior if they do. So a tool like rust fix could apply them, check compiling the result and roll them back on failure. On the other hand MayChangeBehavior have no such guarantee.

It also means that the user workflow can be “run rustfix with PotentialCompileError, and then manually fix any errors they see”.

Whereas for PotentialBehaviorChange the workflow will probably be to review them one by one, and ideally put them in a separate commit for cleaner review (as a reviewer I’d want the direct rustfix fixes in one commit, PotentialCompileError fixups in another, and PotentialBehaviorChange ones in a third)

Yeah, having that split makes sense to me. Migration is going to be annoying though.

I think it does make sense to split that. In Clippy, we use this level for both of those use cases interchangeably.

EDIT: Wait, I suggested that above. I couldn’t remember that I did that 😅

In Clippy MaybeIncorrect is used interchangeable for both cases. Sometimes we use it, if the suggestion might result in new errors (e.g. the borrow checker might complain or an additional (trait) import is needed). And sometimes, because it’s a semantic change (e.g. removes a possibly panicking expression). At least, that’s how I use it and recommend it in reviews.

So maybe splitting it up in MayIntroduceNewError and SemanticChange (with possibly better names) would make sense.

That’s Unspecified, no?

I think Unspecified was for diagnostics that have not been vetted as to whether or not they work correctly. I believe that was used when the applicability system was put in, and there was a huge list of diagnostics to update, so Unspecified was the default until a more appropriate category could be determined.

We might want to have an explicit Applicability::Buggy category. That would help rustfix to ignore them and is to audit them.

That’s Unspecified, no?

https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint_defs/enum.Applicability.html

As a clippy user I would also be interested in a better handling of MaybeIncorrect options. clippy::semicolon_if_nothing_returned is the canonical example of the lint that if failing sometimes automatically is not an issue, and on a large codebase it’s much easier to dealt with only the corner cases manually than all the occurrences.

The split of MaybeIncorrect is probably worth being nominated in a clippy meeting?

I also agree with splitting MaybeIncorrect. Perhaps MayFailToCompile, MayChangeBehavior and MayBeCounterproductive (for things that might compile and keep behavior, but not help readability or perf).

@jyn514 what does the YOLO stand for ?

Proabably “you only live once”, which is

a call to live life to its fullest extent, even embracing behavior which carries inherent risk

The problem is that MaybeIncorrect is often not stuff that is incorrect because it won’t compile, it’s incorrect because it changed the semantics of the program in a way clippy thinks is according to intention but might not be