cargo-fix: Overlapping suggestions should be discarded and cargo-fix rerun.

[alexcrichton]

Bugs like #5797 show a case where legitimate suggestions are provided but are overlapping.

We should probably discard overlapping suggestions in cargo fix and then automatically rerun the compiler if any suggestions were discarded (up to a maximum).

[alexcrichton]

@killercup I'm curious on your thoughts on this, I'm wondering if we may want to tweak the API of rustfix to accomodate something like this? Right now we give a list of suggestions to rustfix to apply to a file, but it either fails everything or succeeds entirely. I wonder if perhaps we should allow it to "partially succeed"? (returning the failed-to-apply fixes)

I think if we did that Cargo could do something like:

• If 0 fixes were applied, print the big scary "this is a bug" warning
• If all fixes were applied, yay!
• If some fixes failed to apply..
  1. Discard fixes that failed to apply.
  2. Keep fixing other files in the project (discarding fixes that fail to apply)
  3. Recompile the project
  4. If compile fails, print scary "this is a bug" warning, rollback changes
  5. If compile succeeds, redo everything and try to fix files again
  6. Loop a fixed number of times, printing a big scary warning if we loop too much, otherwise lints should be incrementally applied.

This should help with #5797 since it needs two rustfix invocations to fix.

Thoughts though? Is this too fancy/intrusive?

[killercup]

Interesting! Having rustfix return a "partial success" (with a vec of failed-to-apply) suggestions sounds like a good way, as it also make the library a bit more useful.

An alternative might be to implement part of #5781: Match on the error returned by rustfix, and when it's "Cannot replace already replaced data" (we should make this a concrete error type), then have cargo-fix try to apply only some of the suggestions. Problem with this is that we'd need to be able to figure out conflicting suggestions in cargo-fix.

I think we need to try and find a good way to deal with "rustfix loops".

Is this too fancy/intrusive?

No, from the feedback we've gotten it seems people want us to actually do more fancy stuff!

[ehuss]

Would it be possible for rustfix to be able to apply overlapping suggestions?

[killercup]

@ehuss I'm not sure how you mean? The comments above describe a way how it can deal with overlapping suggestions :)

#5797 describes a case where the compiler tells rustfix to change ::generic_return::<::B>(); to crate::generic_return::<::B> but also to change the ::B part to crate::B. Without teaching it Rust's semantics, rustfix can't guess which fix to do first or even whether the ::B that is contained in the first replacement is the same as the ::B it should look for to do the second replacement.

[ehuss]

Oh, I meant apply them all in one step instead of iteratively testing the changes. I was just thinking it would use a heuristic like apply outer changes first, then inner ones (that at least works in my editor for that particular example). But I see what you mean, as a general solution that probably would cause problems.

[nwoeanhinnogaehr]

Here's a minimal test case I came across (pretty sure this is the same issue?):

#![feature(rust_2018_preview)]
#![warn(rust_2018_idioms)]

mod foo {
    pub use std::{io, fs};
}
fn main() { }

[alexcrichton]

@nwoeanhinnogaehr that's not quite the same issue, but it's definitely an issue! Want to open an issue on rust-lang/rust for that? That's one which should be fixed at the lint level

[alexcrichton]

@killercup oh sorry didn't see this until now, but sounds good to me! I think we'll just need to add an API to the rustfix crate to apply one suggestion at a time, right? (right now it takes a vector so we don't know which caused an error)