cargo fix shouldn't skip suggestions with multiple snippets but only one replacement

[killercup]

@pietroalbini pointed out an example where cargo-fix was too conservative and skipped a valid replacement:

The problem is that this code

cargo/src/cargo/ops/fix.rs

Lines 412 to 419 in 41a7e15

	if !suggestion
	.snippets
	.iter()
	.all(|s| s.file_name == file_name && s.line_range == range)
	{
	trace!("rejecting as it spans multiple files {:?}", suggestion);
	continue;
	}

checks that the suggestion.snippets point to the same file and range. This was a conservative approach as the edition lints are not doing anything fancy here:

cargo/src/cargo/ops/fix.rs

Lines 402 to 404 in 41a7e15

	// Make sure we've got a file associated with this suggestion and all
	// snippets point to the same location. Right now it's not clear what
	// we would do with multiple locations.

The unused_imports lints however goes out of its way to only yield one replacements for a suggestion with two snippets.

It seems to me that changing the check to not be based on snippets that don't have replacements, but to look at the solutions would fix this.

cc @alexcrichton who I believe wrote the original code

trace log for the run that produced the above screenshot

    Checking foo v0.1.0 (/home/pietro/tmp/foo)
[2018-12-08T18:37:47Z TRACE cargo::ops::fix] cargo-fix as rustc got file Some("src/main.rs")
[2018-12-08T18:37:47Z TRACE cargo::ops::fix] start rustfixing "src/main.rs"
[2018-12-08T18:37:47Z TRACE cargo::ops::fix] line: {"message":"unused imports: `HashMap`, `HashSet`","code":{"code":"unused_imports","explanation":null},"level":"warning","spans":[{"file_name":"src/main.rs","byte_start":23,"byte_end":30,"line_start":1,"line_end":1,"column_start":24,"column_end":31,"is_primary":true,"text":[{"text":"use std::collections::{HashMap, HashSet};","highlight_start":24,"highlight_end":31}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null},{"file_name":"src/main.rs","byte_start":32,"byte_end":39,"line_start":1,"line_end":1,"column_start":33,"column_end":40,"is_primary":true,"text":[{"text":"use std::collections::{HashMap, HashSet};","highlight_start":33,"highlight_end":40}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"#[warn(unused_imports)] on by default","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"remove this","code":null,"level":"help","spans":[{"file_name":"src/main.rs","byte_start":0,"byte_end":41,"line_start":1,"line_end":1,"column_start":1,"column_end":42,"is_primary":true,"text":[{"text":"use std::collections::{HashMap, HashSet};","highlight_start":1,"highlight_end":42}],"label":null,"suggested_replacement":"","suggestion_applicability":"MachineApplicable","expansion":null}],"children":[],"rendered":null}],"rendered":"warning: unused imports: `HashMap`, `HashSet`\n --> src/main.rs:1:24\n  |\n1 | use std::collections::{HashMap, HashSet};\n  | -----------------------^^^^^^^--^^^^^^^-- help: remove this\n  |\n  = note: #[warn(unused_imports)] on by default\n\n"}
[2018-12-08T18:37:47Z TRACE cargo::ops::fix] suggestion
[2018-12-08T18:37:47Z TRACE cargo::ops::fix] rejecting as it spans multiple files Suggestion { message: "unused imports: `HashMap`, `HashSet`", snippets: [Snippet { file_name: "src/main.rs", line_range: LineRange { start: LinePosition { line: 1, column: 24 }, end: LinePosition { line: 1, column: 31 } }, range: 23..30, text: ("use std::collections::{", "HashMap", ", HashSet};") }, Snippet { file_name: "src/main.rs", line_range: LineRange { start: LinePosition { line: 1, column: 33 }, end: LinePosition { line: 1, column: 40 } }, range: 32..39, text: ("use std::collections::{HashMap, ", "HashSet", "};") }], solutions: [Solution { message: "remove this", replacements: [Replacement { snippet: Snippet { file_name: "src/main.rs", line_range: LineRange { start: LinePosition { line: 1, column: 1 }, end: LinePosition { line: 1, column: 42 } }, range: 0..41, text: ("", "use std::collections::{HashMap, HashSet};", "") }, replacement: "" }] }] }
[2018-12-08T18:37:47Z DEBUG cargo::ops::fix] collected 0 suggestions for `src/main.rs`
warning: unused imports: `HashMap`, `HashSet`
 --> src/main.rs:1:24
  |
1 | use std::collections::{HashMap, HashSet};
  | -----------------------^^^^^^^--^^^^^^^-- help: remove this
  |
  = note: #[warn(unused_imports)] on by default

    Finished dev [unoptimized + debuginfo] target(s) in 0.12s

Formatted version of suggestion we parse

Suggestion {
    message: "unused imports: `HashMap`, `HashSet`",
    snippets: [
        Snippet {
            file_name: "src/main.rs",
            line_range: LineRange {
                start: LinePosition { line: 1, column: 24, },
                end: LinePosition { line: 1, column: 31, },
            },
            range: 23..30,
            text: ("use std::collections::{", "HashMap", ", HashSet};"),
        },
        Snippet {
            file_name: "src/main.rs",
            line_range: LineRange {
                start: LinePosition { line: 1, column: 33, },
                end: LinePosition { line: 1, column: 40, },
            },
            range: 32..39,
            text: ("use std::collections::{HashMap, ", "HashSet", "};"),
        },
    ],
    solutions: [Solution {
        message: "remove this",
        replacements: [Replacement {
            snippet: Snippet {
                file_name: "src/main.rs",
                line_range: LineRange {
                    start: LinePosition { line: 1, column: 1 },
                    end: LinePosition { line: 1, column: 42, },
                },
                range: 0..41,
                text: ("", "use std::collections::{HashMap, HashSet};", ""),
            },
            replacement: "",
        }],
    }],
}

[pietroalbini]

Looking at how I could change the check, I have a doubt about the overlapping requirement for solutions (s.line_range == range). While having all the solutions on the same file is fine, for the unused_imports lint I need to have multiple solutions changing different parts of the line:

// Only HashSet is used here
use std::collections::{HashMap, HashSet, VecDeque};
                       ^^^^^^^^^       ^^^^^^^^^^
                            \-- removed --/

Is making sure all solutions are on the same file enough?

[killercup]

Probably.

I'm actually not sure what the range requirement was supposed to solve. Checking that the suggestions are in the same file important as we apply the changes file by file. Checking that the ranges are all the same range is saying "skip all suggestions with multiple replacements", if I read the code correctly. This is a case that might previously have helped but is now actively filtering out the suggestions we care about, it seems.