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

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

[kraktus]

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?

[Qix-]

Just chiming in, being able to say something like --try-anyway even if it results in syntax errors etc. is still better than having to refactor 10,000 instances of something tedious (e.g. adding a generic to a deeply-coupled type).

[est31]

This would also be useful for rustc tests. It's not always the case that a MaybeIncorrect suggestion is incorrect, and you might want to add rustfix based tests for the cases where it is right, to ensure that there is no regression. e.g. I've filed rust-lang/rust#105227 about an incorrect suggestion and the PR to fix it would benefit from a rustfix option to apply the suggestion.

[ehuss]

@est31 compiletest applies all suggestions regardless of applicability with the run-rustfix header. Only when using rustfix-only-machine-applicable will it limit it to machine-applicable.

[est31]

@ehuss oh it seems compiletest seems to use rustfix as a crate and there you can tell it to apply MaybeIncorrect suggestions. Thanks for the pointer!

[Manishearth]

Here's the current plan:

• add PotentialBehaviorChange and PotentialCompileError. Keep MaybeIncorrect around for now.
  • PotentialCompileError is considered more machine applicable than PotentialBehaviorChange since the latter means both
• do a campaign to migrate everyone
• remove MaybeIncorrect
• work with rustfix and RA for better UI

@Veykril does this look OK to you from a rust-analyzer perspective? are these categories useful?

[ehuss]

I'm not sure I understand "remove MachineApplicable". Isn't that the variant that is most useful? Did you maybe mean MaybeIncorrect?

Can you define exactly what these two categories mean? MaybeIncorrect is often used for suggestions that are known to be outright buggy. I'm not sure that really fits PotentialCompileError, which reads to me that rustc is unable to provide a better suggestion.

[Manishearth]

Er, sorry, meant MaybeIncorrect

Can you define exactly what these two categories mean? MaybeIncorrect is often used for suggestions that are known to be outright buggy. I'm not sure that really fits PotentialCompileError, which reads to me that rustc is unable to provide a better suggestion.

• PotentialCompileError: this may introduce a compiler error
• PotentialBehaviorChange: this may change the behavior of the code, or introduce a compiler error (i.e., needs manual review)

It's an interesting point about suggestions that are outright buggy. Perhaps we should add a Nursery/Buggy category? Not sure. I kinda feel BehaviorChange covers that okayishly. Right now, MaybeIncorrect covers a lot of different types of incorrectness with lots of different implications on how people use the tooling. I'm not sure if Buggy and PotentialBehaviorChange have different implications on tooling use

[estebank]

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

[jyn514]

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

[Veykril]

Here's the current plan:

* add PotentialBehaviorChange and PotentialCompileError. Keep MaybeIncorrect around for now.
  
  * PotentialCompileError is considered more machine applicable than PotentialBehaviorChange since the latter means both

* do a campaign to migrate everyone

* remove MaybeIncorrect

* work with rustfix and RA for better UI

@Veykril does this look OK to you from a rust-analyzer perspective? are these categories useful?

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

[estebank]

@jyn514 it could be, I'm not sure if we are using it that way though.

[ehuss]

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.

[llogiq]

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.

[Manishearth]

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)

[flip1995]

We might want to have an explicit Applicability::Buggy category.

Unspecified in Clippy is pretty much a shrug 🤷 for suggestions right now: no one spent time looking at the suggestion to categorize it.

That said, I don't think a Buggy category would add much value here. I think, that every (potentially) "buggy" suggestion could be categorized into one of PotentialCompileError, PotentialBehaviorChange, or HasPlaceholder. And if it is really impossible to tell where it should go, I think Unspecified describes that pretty well.

[Manishearth]

cc @Muscraft

[Kixunil]

Thanks a lot @jyn514 this is incredibly helpful!

In my case, I need to rename struct fields from input to inputs and output to outputs because they are both Vecs (carrying different types so mixup is impossible). If I do this as a single change - change the field names and then fun cargo fix then this is not only entirely correct but there's literally nothing that can go wrong. All the code is tested properly and there are no fields that have similar names and same types to be confused with. I've just made a demo commit changing >200 LOC in a matter of minutes.

So I think for cases like this it's not really "YOLO" and it'd be nice to expose it to users more cleanly but even having it at all is already a huge deal.