Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Apr 10, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b05a3b
test(rustfix): demonstrate duplicate suggestions
weihanglo Apr 9, 2024
95edc06
fix(rustfix): dont apply same suggestion twice
weihanglo Apr 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions crates/rustfix/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to duplicate the work in also rustfix. I didn't do it in CodeFix as it might require a longer lifetime bound than it was.

// 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()
Expand Down
17 changes: 17 additions & 0 deletions crates/rustfix/tests/everything/dedup-suggestions.fixed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
}
5 changes: 5 additions & 0 deletions crates/rustfix/tests/everything/dedup-suggestions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"}
17 changes: 17 additions & 0 deletions crates/rustfix/tests/everything/dedup-suggestions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
}
17 changes: 14 additions & 3 deletions src/cargo/ops/fix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
}
}
Expand Down
45 changes: 45 additions & 0 deletions tests/testsuite/fix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
epage marked this conversation as resolved.
Show resolved Hide resolved
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"
);
}