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 https://github.com/rust-lang/cargo/issues/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: https://github.com/rust-lang/rustfix/issues/163

[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...? https://github.com/rust-lang/cargo/blob/50a0af4bfd47294f53cf3000e1a7e076162280c6/src/cargo/ops/fix.rs#L581

[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

[camsteffen]

All the kids are saying it

[jyn514]

@NikosEfthias sample usage:

I was supposed to check if I accidentally commited tax crimes today via armchair lawyering but we can just yolo that

[Manishearth]

@estebank @rust-lang/clippy it's been a while since it's been filed and there's some interest in exposing applicability levels to users. Before we do so, do y'all think it's worth splitting MaybeIncorrect into SemanticChange and MayIntroduceNewError?

Ideally the applicability exposition lets you choose how far you want to go.

[flip1995]

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.

[Manishearth]

Maybe we can do a nice audit to clippy to see which goes where. I wonder if that's something @3tilley is interested in looking in to

The way I see it is that there are three somewhat independent work items here:

• Look at migrating clippy and rustc over to a splti MaybeIncorrect level
• Propose a cargo flag that exposes rustfix's yolo-mode. Design it better (perhaps as a "minimum applicability level" setting)
• Add an interactive mode to rustfix.

[estebank]

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

[3tilley]

Hello, I've been following this conversation but haven't had a chance to dig in properly. I YOLO'd this evening to help me with some module refactoring and it certainly did what it said on the tin. Just recording four and a half potential bugs here while I've got them in front of me. This is git bash running on regular Windows 10 for reference:

1. The u from use got dropped somehow:

2. Where use was required in multiple places, it was added multiple times:

3. This could have been because of poor code quality initially with non-standard imports, but it seems like use sometimes reexports types from the module and sometimes takes them from the original crate? I'm not good enough at Rust to know exactly what's happening here, and what should happen:

4. Do macro import suggestions count as MaybeIncorrect? YOLO wouldn't implement those suggestions, which perhaps means it doesn't?

5. I don't know if this is really a bug, but the format that was used for the use lines seems a bit odd - lots of blank lines added between statements. Are fixes supposed to be rustfmt compatible?

[llogiq]

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).

[camsteffen]

How does splitting applicability improve UX? Or what problem does it solve?

[llogiq]

We may use the split values for reporting. Users would likely be interested what they should expect. Also if we had a "might not compile, but if it does, it's OK" category, rustfix could try to auto-fix and back out on errors.

[Manishearth]

@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.