Auto merge of #13728 - weihanglo:dedup-suggestion, r=epage

fix(cargo-fix): dont apply same suggestion twice

crates/rustfix/src/lib.rs

@@ -253,8 +253,19 @@ impl CodeFix {
 
 /// Applies multiple `suggestions` to the given `code`.
 pub fn apply_suggestions(code: &str, suggestions: &[Suggestion]) -> Result<String, Error> {
+    let mut already_applied = HashSet::new();
     let mut fix = CodeFix::new(code);
     for suggestion in suggestions.iter().rev() {
+        // This assumes that if any of the machine applicable fixes in
+        // a diagnostic suggestion is a duplicate, we should see the
+        // entire suggestion as a duplicate.
+        if suggestion
+            .solutions
+            .iter()
+            .any(|sol| !already_applied.insert(sol))
+        {
+            continue;
+        }
         fix.apply(suggestion)?;
     }
     fix.finish()

crates/rustfix/tests/everything/dedup-suggestions.fixed.rs

@@ -0,0 +1,17 @@
+// This fixes rust-lang/rust#123304.
+// If that lint stops emitting duplicate suggestions,
+// we might need to find a substitution.
+#![warn(unsafe_op_in_unsafe_fn)]
+
+macro_rules! foo {
+    ($x:ident) => {
+        pub unsafe fn $x() { unsafe {
+            let _ = String::new().as_mut_vec();
+        }}
+    };
+}
+
+fn main() {
+    foo!(a);
+    foo!(b);
+}

crates/rustfix/tests/everything/dedup-suggestions.json

@@ -0,0 +1,5 @@
+{"$message_type":"diagnostic","message":"call to unsafe function `std::string::String::as_mut_vec` is unsafe and requires unsafe block (error E0133)","code":{"code":"unsafe_op_in_unsafe_fn","explanation":null},"level":"warning","spans":[{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":254,"byte_end":280,"line_start":9,"line_end":9,"column_start":21,"column_end":47,"is_primary":true,"text":[{"text":"            let _ = String::new().as_mut_vec();","highlight_start":21,"highlight_end":47}],"label":"call to unsafe function","suggested_replacement":null,"suggestion_applicability":null,"expansion":{"span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":318,"byte_end":325,"line_start":15,"line_end":15,"column_start":5,"column_end":12,"is_primary":false,"text":[{"text":"    foo!(a);","highlight_start":5,"highlight_end":12}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null},"macro_decl_name":"foo!","def_site_span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":166,"byte_end":182,"line_start":6,"line_end":6,"column_start":1,"column_end":17,"is_primary":false,"text":[{"text":"macro_rules! foo {","highlight_start":1,"highlight_end":17}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}}}],"children":[{"message":"for more information, see issue #71668 <https://github.com/rust-lang/rust/issues/71668>","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"consult the function's documentation for information on how to avoid undefined behavior","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"an unsafe function restricts its caller, but its body is safe by default","code":null,"level":"note","spans":[{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":213,"byte_end":231,"line_start":8,"line_end":8,"column_start":9,"column_end":27,"is_primary":true,"text":[{"text":"        pub unsafe fn $x() {","highlight_start":9,"highlight_end":27}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":{"span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":318,"byte_end":325,"line_start":15,"line_end":15,"column_start":5,"column_end":12,"is_primary":false,"text":[{"text":"    foo!(a);","highlight_start":5,"highlight_end":12}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null},"macro_decl_name":"foo!","def_site_span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":166,"byte_end":182,"line_start":6,"line_end":6,"column_start":1,"column_end":17,"is_primary":false,"text":[{"text":"macro_rules! foo {","highlight_start":1,"highlight_end":17}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}}}],"children":[],"rendered":null},{"message":"the lint level is defined here","code":null,"level":"note","spans":[{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":140,"byte_end":162,"line_start":4,"line_end":4,"column_start":9,"column_end":31,"is_primary":true,"text":[{"text":"#![warn(unsafe_op_in_unsafe_fn)]","highlight_start":9,"highlight_end":31}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":null},{"message":"consider wrapping the function body in an unsafe block","code":null,"level":"help","spans":[{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":232,"byte_end":232,"line_start":8,"line_end":8,"column_start":28,"column_end":28,"is_primary":true,"text":[{"text":"        pub unsafe fn $x() {","highlight_start":28,"highlight_end":28}],"label":null,"suggested_replacement":"{ unsafe ","suggestion_applicability":"MachineApplicable","expansion":{"span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":318,"byte_end":325,"line_start":15,"line_end":15,"column_start":5,"column_end":12,"is_primary":false,"text":[{"text":"    foo!(a);","highlight_start":5,"highlight_end":12}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null},"macro_decl_name":"foo!","def_site_span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":166,"byte_end":182,"line_start":6,"line_end":6,"column_start":1,"column_end":17,"is_primary":false,"text":[{"text":"macro_rules! foo {","highlight_start":1,"highlight_end":17}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}}},{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":291,"byte_end":291,"line_start":10,"line_end":10,"column_start":10,"column_end":10,"is_primary":true,"text":[{"text":"        }","highlight_start":10,"highlight_end":10}],"label":null,"suggested_replacement":"}","suggestion_applicability":"MachineApplicable","expansion":{"span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":318,"byte_end":325,"line_start":15,"line_end":15,"column_start":5,"column_end":12,"is_primary":false,"text":[{"text":"    foo!(a);","highlight_start":5,"highlight_end":12}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null},"macro_decl_name":"foo!","def_site_span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":166,"byte_end":182,"line_start":6,"line_end":6,"column_start":1,"column_end":17,"is_primary":false,"text":[{"text":"macro_rules! foo {","highlight_start":1,"highlight_end":17}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}}}],"children":[],"rendered":null}],"rendered":"warning: call to unsafe function `std::string::String::as_mut_vec` is unsafe and requires unsafe block (error E0133)\n  --> ./tests/everything/dedup-suggestions.rs:9:21\n   |\n9  |             let _ = String::new().as_mut_vec();\n   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^ call to unsafe function\n...\n15 |     foo!(a);\n   |     ------- in this macro invocation\n   |\n   = note: for more information, see issue #71668 <https://github.com/rust-lang/rust/issues/71668>\n   = note: consult the function's documentation for information on how to avoid undefined behavior\nnote: an unsafe function restricts its caller, but its body is safe by default\n  --> ./tests/everything/dedup-suggestions.rs:8:9\n   |\n8  |         pub unsafe fn $x() {\n   |         ^^^^^^^^^^^^^^^^^^\n...\n15 |     foo!(a);\n   |     ------- in this macro invocation\nnote: the lint level is defined here\n  --> ./tests/everything/dedup-suggestions.rs:4:9\n   |\n4  | #![warn(unsafe_op_in_unsafe_fn)]\n   |         ^^^^^^^^^^^^^^^^^^^^^^\n   = note: this warning originates in the macro `foo` (in Nightly builds, run with -Z macro-backtrace for more info)\n\n"}
+{"$message_type":"diagnostic","message":"call to unsafe function `std::string::String::as_mut_vec` is unsafe and requires unsafe block (error E0133)","code":{"code":"unsafe_op_in_unsafe_fn","explanation":null},"level":"warning","spans":[{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":254,"byte_end":280,"line_start":9,"line_end":9,"column_start":21,"column_end":47,"is_primary":true,"text":[{"text":"            let _ = String::new().as_mut_vec();","highlight_start":21,"highlight_end":47}],"label":"call to unsafe function","suggested_replacement":null,"suggestion_applicability":null,"expansion":{"span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":331,"byte_end":338,"line_start":16,"line_end":16,"column_start":5,"column_end":12,"is_primary":false,"text":[{"text":"    foo!(b);","highlight_start":5,"highlight_end":12}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null},"macro_decl_name":"foo!","def_site_span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":166,"byte_end":182,"line_start":6,"line_end":6,"column_start":1,"column_end":17,"is_primary":false,"text":[{"text":"macro_rules! foo {","highlight_start":1,"highlight_end":17}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}}}],"children":[{"message":"for more information, see issue #71668 <https://github.com/rust-lang/rust/issues/71668>","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"consult the function's documentation for information on how to avoid undefined behavior","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"an unsafe function restricts its caller, but its body is safe by default","code":null,"level":"note","spans":[{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":213,"byte_end":231,"line_start":8,"line_end":8,"column_start":9,"column_end":27,"is_primary":true,"text":[{"text":"        pub unsafe fn $x() {","highlight_start":9,"highlight_end":27}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":{"span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":331,"byte_end":338,"line_start":16,"line_end":16,"column_start":5,"column_end":12,"is_primary":false,"text":[{"text":"    foo!(b);","highlight_start":5,"highlight_end":12}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null},"macro_decl_name":"foo!","def_site_span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":166,"byte_end":182,"line_start":6,"line_end":6,"column_start":1,"column_end":17,"is_primary":false,"text":[{"text":"macro_rules! foo {","highlight_start":1,"highlight_end":17}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}}}],"children":[],"rendered":null},{"message":"consider wrapping the function body in an unsafe block","code":null,"level":"help","spans":[{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":232,"byte_end":232,"line_start":8,"line_end":8,"column_start":28,"column_end":28,"is_primary":true,"text":[{"text":"        pub unsafe fn $x() {","highlight_start":28,"highlight_end":28}],"label":null,"suggested_replacement":"{ unsafe ","suggestion_applicability":"MachineApplicable","expansion":{"span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":331,"byte_end":338,"line_start":16,"line_end":16,"column_start":5,"column_end":12,"is_primary":false,"text":[{"text":"    foo!(b);","highlight_start":5,"highlight_end":12}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null},"macro_decl_name":"foo!","def_site_span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":166,"byte_end":182,"line_start":6,"line_end":6,"column_start":1,"column_end":17,"is_primary":false,"text":[{"text":"macro_rules! foo {","highlight_start":1,"highlight_end":17}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}}},{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":291,"byte_end":291,"line_start":10,"line_end":10,"column_start":10,"column_end":10,"is_primary":true,"text":[{"text":"        }","highlight_start":10,"highlight_end":10}],"label":null,"suggested_replacement":"}","suggestion_applicability":"MachineApplicable","expansion":{"span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":331,"byte_end":338,"line_start":16,"line_end":16,"column_start":5,"column_end":12,"is_primary":false,"text":[{"text":"    foo!(b);","highlight_start":5,"highlight_end":12}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null},"macro_decl_name":"foo!","def_site_span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":166,"byte_end":182,"line_start":6,"line_end":6,"column_start":1,"column_end":17,"is_primary":false,"text":[{"text":"macro_rules! foo {","highlight_start":1,"highlight_end":17}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}}}],"children":[],"rendered":null}],"rendered":"warning: call to unsafe function `std::string::String::as_mut_vec` is unsafe and requires unsafe block (error E0133)\n  --> ./tests/everything/dedup-suggestions.rs:9:21\n   |\n9  |             let _ = String::new().as_mut_vec();\n   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^ call to unsafe function\n...\n16 |     foo!(b);\n   |     ------- in this macro invocation\n   |\n   = note: for more information, see issue #71668 <https://github.com/rust-lang/rust/issues/71668>\n   = note: consult the function's documentation for information on how to avoid undefined behavior\nnote: an unsafe function restricts its caller, but its body is safe by default\n  --> ./tests/everything/dedup-suggestions.rs:8:9\n   |\n8  |         pub unsafe fn $x() {\n   |         ^^^^^^^^^^^^^^^^^^\n...\n16 |     foo!(b);\n   |     ------- in this macro invocation\n   = note: this warning originates in the macro `foo` (in Nightly builds, run with -Z macro-backtrace for more info)\n\n"}
+{"$message_type":"diagnostic","message":"function `a` is never used","code":{"code":"dead_code","explanation":null},"level":"warning","spans":[{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":323,"byte_end":324,"line_start":15,"line_end":15,"column_start":10,"column_end":11,"is_primary":true,"text":[{"text":"    foo!(a);","highlight_start":10,"highlight_end":11}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":{"span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":318,"byte_end":325,"line_start":15,"line_end":15,"column_start":5,"column_end":12,"is_primary":false,"text":[{"text":"    foo!(a);","highlight_start":5,"highlight_end":12}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null},"macro_decl_name":"foo!","def_site_span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":166,"byte_end":182,"line_start":6,"line_end":6,"column_start":1,"column_end":17,"is_primary":false,"text":[{"text":"macro_rules! foo {","highlight_start":1,"highlight_end":17}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}}}],"children":[{"message":"`#[warn(dead_code)]` on by default","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"warning: function `a` is never used\n  --> ./tests/everything/dedup-suggestions.rs:15:10\n   |\n15 |     foo!(a);\n   |          ^\n   |\n   = note: `#[warn(dead_code)]` on by default\n   = note: this warning originates in the macro `foo` (in Nightly builds, run with -Z macro-backtrace for more info)\n\n"}
+{"$message_type":"diagnostic","message":"function `b` is never used","code":{"code":"dead_code","explanation":null},"level":"warning","spans":[{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":336,"byte_end":337,"line_start":16,"line_end":16,"column_start":10,"column_end":11,"is_primary":true,"text":[{"text":"    foo!(b);","highlight_start":10,"highlight_end":11}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":{"span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":331,"byte_end":338,"line_start":16,"line_end":16,"column_start":5,"column_end":12,"is_primary":false,"text":[{"text":"    foo!(b);","highlight_start":5,"highlight_end":12}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null},"macro_decl_name":"foo!","def_site_span":{"file_name":"./tests/everything/dedup-suggestions.rs","byte_start":166,"byte_end":182,"line_start":6,"line_end":6,"column_start":1,"column_end":17,"is_primary":false,"text":[{"text":"macro_rules! foo {","highlight_start":1,"highlight_end":17}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}}}],"children":[],"rendered":"warning: function `b` is never used\n  --> ./tests/everything/dedup-suggestions.rs:16:10\n   |\n16 |     foo!(b);\n   |          ^\n   |\n   = note: this warning originates in the macro `foo` (in Nightly builds, run with -Z macro-backtrace for more info)\n\n"}
+{"$message_type":"diagnostic","message":"4 warnings emitted","code":null,"level":"warning","spans":[],"children":[],"rendered":"warning: 4 warnings emitted\n\n"}

crates/rustfix/tests/everything/dedup-suggestions.rs

@@ -0,0 +1,17 @@
+// This fixes rust-lang/rust#123304.
+// If that lint stops emitting duplicate suggestions,
+// we might need to find a substitution.
+#![warn(unsafe_op_in_unsafe_fn)]
+
+macro_rules! foo {
+    ($x:ident) => {
+        pub unsafe fn $x() {
+            let _ = String::new().as_mut_vec();
+        }
+    };
+}
+
+fn main() {
+    foo!(a);
+    foo!(b);
+}

src/cargo/ops/fix.rs

@@ -735,12 +735,23 @@ fn rustfix_and_fix(
         });
         let mut fixed = CodeFix::new(&code);
 
-        // As mentioned above in `rustfix_crate`, we don't immediately warn
-        // about suggestions that fail to apply here, and instead we save them
-        // off for later processing.
+        let mut already_applied = HashSet::new();
         for suggestion in suggestions.iter().rev() {
+            // This assumes that if any of the machine applicable fixes in
+            // a diagnostic suggestion is a duplicate, we should see the
+            // entire suggestion as a duplicate.
+            if suggestion
+                .solutions
+                .iter()
+                .any(|sol| !already_applied.insert(sol))
+            {
+                continue;
+            }
             match fixed.apply(suggestion) {
                 Ok(()) => fixed_file.fixes_applied += 1,
+                // As mentioned above in `rustfix_crate`, we don't immediately
+                // warn about suggestions that fail to apply here, and instead
+                // we save them off for later processing.
                 Err(e) => fixed_file.errors_applying_fixes.push(e.to_string()),
             }
         }

tests/testsuite/fix.rs

@@ -1897,3 +1897,48 @@ warning: `foo` (lib) generated 1 warning (run `cargo fix --lib -p foo` to apply
 ")
         .run();
 }
+
+// This fixes rust-lang/rust#123304.
+// If that lint stops emitting duplicate suggestions,
+// we might need to find a substitution.
+#[cargo_test]
+fn fix_only_once_for_duplicates() {
+    let p = project()
+        .file(
+            "src/lib.rs",
+            r#"
+                #![warn(unsafe_op_in_unsafe_fn)]
+
+                macro_rules! foo {
+                    ($x:ident) => {
+                        pub unsafe fn $x() {
+                            let _ = String::new().as_mut_vec();
+                        }
+                    };
+                }
+
+                foo!(a);
+                foo!(b);
+            "#,
+        )
+        .build();
+
+    p.cargo("fix --allow-no-vcs")
+        .with_stderr(
+            "\
+[CHECKING] foo v0.0.1 ([CWD])
+[FIXED] src/lib.rs (1 fix)
+[FINISHED] `dev` profile [..]
+",
+        )
+        .run();
+
+    assert_eq!(
+        p.read_file("src/lib.rs").matches("unsafe").count(),
+        4,
+        "unsafe keyword in src/lib.rs:\n\
+            2 in lint name;\n\
+            1 from original unsafe fn;\n\
+            1 from newly-applied unsafe blocks"
+    );
+}