`cargo fix`: Improve handling of MaybeIncorrect suggestions

[Manishearth]

When the compiler or clippy produce suggestions, some of them are marked as MaybeIncorrect; i.e. the suggestion should probably not be automatically applied. rustfix currently doesn't touch these.

This leads to confusion like in #8806, and is somewhat frustrating. We should at the very least be clear that such suggestions were not applied.

Ideally, we should provide a mode where you can apply them anyway (cargo clippy --fix --apply-all-suggestions?), or apply them using a "pick and choose" mode. We can then mention it when people have suggestions that were not applied.

See also: #13023

[Manishearth]

cc @estebank

[Manishearth]

also cc @ehuss for thinking about what the UX would be like from cargo's perspective

[estebank]

Is there something on the rustc side that needs to be done? We could add a new variant with a degree of confidence between MachineApplicable and MaybeIncorrect. We could have LikelyIncorrect for things we have little confidence in and start applying MaybeIncorrect on rustfix's side.

[Manishearth]

@estebank no, I don't think that'll help. MaybeIncorrect already is that variant: there's another one for when you're absolutely sure it's incorrect. MaybeIncorrect is just when it's a bad idea to auto apply the fix without checking; because the fix may introduce semantic change (etc)

At least the way it's used in clippy, most MaybeIncorrects are not supposed to be blindly applied.

cc @rust-lang/clippy in case folks have opinions on whether we need additional suggestion applicability types

This should be entirely on the rustfix/cargo side I think.

[matthiaskrgr]

I think to some extent rustfix already tries to skip applying lint results where the resulting code does not compile.
I would love to be able to just throw everything at it and let rustc and rustfix figure it out by themselves. :D

[Manishearth]

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

[flip1995]

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.

[camsteffen]

Both reasons might apply. That may be reason enough not to split it. Unless we say, "if it is maybe incorrect for multiple reasons, prefer X." Is the motivation for splitting to offer an explanation to the user? Maybe instead we could add a "incorrect reason" field. In fact, in reviews I've picked up the habit of making sure there is a code comment explaining MaybeIncorrect.

[flip1995]

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)

[camsteffen]

For what it's worth, I think "may introduce error" can also be described as "maybe incomplete" - additional changes are necessary to "complete" the intended suggestion, like adding an import.

[jyn514]

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.

[ta3pks]

@jyn514 what does the YOLO stand for ?

[matthiaskrgr]

I think it tries to apply suggestions no matter what...?

cargo/src/cargo/ops/fix.rs

Line 581 in 50a0af4

let fix_mode = env::var_os("__CARGO_FIX_YOLO")

[steffahn]

@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