diff --git a/Cargo.lock b/Cargo.lock index 2366615cd27a..d6edc276410f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2943,7 +2943,7 @@ checksum = "08d43f7aa6b08d49f382cde6a7982047c3426db949b1424bc4b7ec9ae12c6ce2" [[package]] name = "rustfix" -version = "0.8.4" +version = "0.8.5" dependencies = [ "anyhow", "proptest", diff --git a/crates/rustfix/Cargo.toml b/crates/rustfix/Cargo.toml index 1f9b61cd5e29..ce455bf6f84c 100644 --- a/crates/rustfix/Cargo.toml +++ b/crates/rustfix/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "rustfix" -version = "0.8.4" +version = "0.8.5" authors = [ "Pascal Hertleif ", "Oliver Schneider ", diff --git a/crates/rustfix/src/lib.rs b/crates/rustfix/src/lib.rs index 2f6acaf30287..92059989f809 100644 --- a/crates/rustfix/src/lib.rs +++ b/crates/rustfix/src/lib.rs @@ -213,6 +213,7 @@ pub fn collect_suggestions( /// 1. Feeds the source of a file to [`CodeFix::new`]. /// 2. Calls [`CodeFix::apply`] to apply suggestions to the source code. /// 3. Calls [`CodeFix::finish`] to get the "fixed" code. +#[derive(Clone)] pub struct CodeFix { data: replace::Data, /// Whether or not the data has been modified. @@ -230,12 +231,18 @@ impl CodeFix { /// Applies a suggestion to the code. pub fn apply(&mut self, suggestion: &Suggestion) -> Result<(), Error> { - for sol in &suggestion.solutions { - for r in &sol.replacements { - self.data - .replace_range(r.snippet.range.clone(), r.replacement.as_bytes())?; - self.modified = true; - } + for solution in &suggestion.solutions { + self.apply_solution(solution)?; + } + Ok(()) + } + + /// Applies an individual solution from a [`Suggestion`] + pub fn apply_solution(&mut self, solution: &Solution) -> Result<(), Error> { + for r in &solution.replacements { + self.data + .replace_range(r.snippet.range.clone(), r.replacement.as_bytes())?; + self.modified = true; } Ok(()) } @@ -252,6 +259,10 @@ impl CodeFix { } /// Applies multiple `suggestions` to the given `code`. +/// +/// When a diagnostic has multiple `help` subdiagnostics that offer suggestions +/// they are merged together into a single output, to apply them separately see +/// [`apply_suggestions_separately`] pub fn apply_suggestions(code: &str, suggestions: &[Suggestion]) -> Result { let mut already_applied = HashSet::new(); let mut fix = CodeFix::new(code); @@ -270,3 +281,33 @@ pub fn apply_suggestions(code: &str, suggestions: &[Suggestion]) -> Result Result, Error> { + let max_solutions = suggestions + .iter() + .map(|suggestion| suggestion.solutions.len()) + .max() + .unwrap_or_default(); + let mut already_applied = HashSet::new(); + let mut fixes = vec![CodeFix::new(code); max_solutions]; + for suggestion in suggestions.iter().rev() { + for (solution, fix) in suggestion.solutions.iter().zip(&mut fixes) { + if already_applied.insert(solution) { + fix.apply_solution(solution)?; + } + } + } + fixes.iter().map(CodeFix::finish).collect() +} diff --git a/crates/rustfix/tests/parse_and_replace.rs b/crates/rustfix/tests/parse_and_replace.rs index 7bb11d33239e..4179063fa284 100644 --- a/crates/rustfix/tests/parse_and_replace.rs +++ b/crates/rustfix/tests/parse_and_replace.rs @@ -23,7 +23,7 @@ #![allow(clippy::disallowed_methods, clippy::print_stdout, clippy::print_stderr)] use anyhow::{anyhow, ensure, Context, Error}; -use rustfix::apply_suggestions; +use rustfix::{apply_suggestions, apply_suggestions_separately}; use std::collections::HashSet; use std::env; use std::ffi::OsString; @@ -33,8 +33,12 @@ use std::process::{Command, Output}; use tempfile::tempdir; use tracing::{debug, info, warn}; -mod fixmode { - pub const EVERYTHING: &str = "yolo"; +#[derive(Clone, Copy)] +enum ReplaceMode { + /// Combine suggestions into a single `.fixed` file with [`apply_suggestions`] + Combined, + /// Create a separate `.fixed` file per suggestion with [`apply_suggestions_separately`] + Separate, } mod settings { @@ -151,23 +155,16 @@ fn diff(expected: &str, actual: &str) -> String { res } -fn test_rustfix_with_file>(file: P, mode: &str) -> Result<(), Error> { +fn test_rustfix_with_file>(file: P, replace_mode: ReplaceMode) -> Result<(), Error> { let file: &Path = file.as_ref(); let json_file = file.with_extension("json"); - let fixed_file = file.with_extension("fixed.rs"); - - let filter_suggestions = if mode == fixmode::EVERYTHING { - rustfix::Filter::Everything - } else { - rustfix::Filter::MachineApplicableOnly - }; debug!("next up: {:?}", file); let code = fs::read_to_string(file)?; let errors = compile_and_get_json_errors(file).context(format!("could compile {}", file.display()))?; let suggestions = - rustfix::get_suggestions_from_json(&errors, &HashSet::new(), filter_suggestions) + rustfix::get_suggestions_from_json(&errors, &HashSet::new(), rustfix::Filter::Everything) .context("could not load suggestions")?; if std::env::var(settings::RECORD_JSON).is_ok() { @@ -179,9 +176,12 @@ fn test_rustfix_with_file>(file: P, mode: &str) -> Result<(), Err "could not load json fixtures for {}", file.display() ))?; - let expected_suggestions = - rustfix::get_suggestions_from_json(&expected_json, &HashSet::new(), filter_suggestions) - .context("could not load expected suggestions")?; + let expected_suggestions = rustfix::get_suggestions_from_json( + &expected_json, + &HashSet::new(), + rustfix::Filter::Everything, + ) + .context("could not load expected suggestions")?; ensure!( expected_suggestions == suggestions, @@ -193,28 +193,52 @@ fn test_rustfix_with_file>(file: P, mode: &str) -> Result<(), Err ); } - let fixed = apply_suggestions(&code, &suggestions) - .context(format!("could not apply suggestions to {}", file.display()))? - .replace('\r', ""); + let bless = std::env::var_os(settings::BLESS) + .is_some_and(|bless_name| bless_name == file.file_name().unwrap()); - if std::env::var(settings::RECORD_FIXED_RUST).is_ok() { - fs::write(file.with_extension("recorded.rs"), &fixed)?; + if bless { + std::fs::write(&json_file, &errors)?; } - if let Some(bless_name) = std::env::var_os(settings::BLESS) { - if bless_name == file.file_name().unwrap() { - std::fs::write(&json_file, &errors)?; - std::fs::write(&fixed_file, &fixed)?; + match replace_mode { + ReplaceMode::Combined => { + let fixed = apply_suggestions(&code, &suggestions) + .context(format!("could not apply suggestions to {}", file.display()))? + .replace('\r', ""); + check_fixed_file(fixed, file.with_extension("fixed.rs"), bless)?; } + ReplaceMode::Separate => { + let fixes = apply_suggestions_separately(&code, &suggestions) + .context(format!("could not apply suggestions to {}", file.display()))?; + for (i, fixed) in fixes.iter().enumerate() { + check_fixed_file( + fixed.replace('\r', ""), + file.with_extension(format!("fixed.{i}.rs")), + bless, + )?; + } + } + } + + Ok(()) +} + +fn check_fixed_file(fixed: String, fixed_file: PathBuf, bless: bool) -> Result<(), Error> { + if std::env::var(settings::RECORD_FIXED_RUST).is_ok() { + fs::write(fixed_file.with_extension("recorded.rs"), &fixed)?; + } + + if bless { + std::fs::write(&fixed_file, &fixed)?; } let expected_fixed = fs::read_to_string(&fixed_file) - .context(format!("could read fixed file for {}", file.display()))? + .context(format!("could read fixed file {}", fixed_file.display()))? .replace('\r', ""); ensure!( fixed.trim() == expected_fixed.trim(), - "file {} doesn't look fixed:\n{}", - file.display(), + "file {} doesn't match expected fix:\n{}", + fixed_file.display(), diff(fixed.trim(), expected_fixed.trim()) ); @@ -229,12 +253,12 @@ fn get_fixture_files(p: &str) -> Result, Error> { .filter(|p| p.is_file()) .filter(|p| { let x = p.to_string_lossy(); - x.ends_with(".rs") && !x.ends_with(".fixed.rs") && !x.ends_with(".recorded.rs") + x.ends_with(".rs") && !x.contains(".fixed.") && !x.ends_with(".recorded.rs") }) .collect()) } -fn assert_fixtures(dir: &str, mode: &str) { +fn assert_fixtures(dir: &str, replace_mode: ReplaceMode) { let files = get_fixture_files(dir) .context(format!("couldn't load dir `{}`", dir)) .unwrap(); @@ -254,7 +278,7 @@ fn assert_fixtures(dir: &str, mode: &str) { info!("skipped: {file:?}"); continue; } - if let Err(err) = test_rustfix_with_file(file, mode) { + if let Err(err) = test_rustfix_with_file(file, replace_mode) { println!("failed: {}", file.display()); warn!("{:?}", err); failures += 1; @@ -275,5 +299,6 @@ fn assert_fixtures(dir: &str, mode: &str) { #[test] fn everything() { tracing_subscriber::fmt::init(); - assert_fixtures("./tests/everything", fixmode::EVERYTHING); + assert_fixtures("./tests/everything", ReplaceMode::Combined); + assert_fixtures("./tests/separate", ReplaceMode::Separate); } diff --git a/crates/rustfix/tests/separate/multiple.fixed.0.rs b/crates/rustfix/tests/separate/multiple.fixed.0.rs new file mode 100644 index 000000000000..086838859463 --- /dev/null +++ b/crates/rustfix/tests/separate/multiple.fixed.0.rs @@ -0,0 +1,8 @@ +fn main() { + let mut a = 0; + a += 1; + a += 2; + + // Should be `_unused` in `.fixed.0.rs` but left as `unused` in `.fixed.1.rs` + let _unused = a; +} diff --git a/crates/rustfix/tests/separate/multiple.fixed.1.rs b/crates/rustfix/tests/separate/multiple.fixed.1.rs new file mode 100644 index 000000000000..fbfe34685be4 --- /dev/null +++ b/crates/rustfix/tests/separate/multiple.fixed.1.rs @@ -0,0 +1,8 @@ +fn main() { + let mut a = 0; + a = a + a + 1; + a = a + a + 2; + + // Should be `_unused` in `.fixed.0.rs` but left as `unused` in `.fixed.1.rs` + let unused = a; +} diff --git a/crates/rustfix/tests/separate/multiple.json b/crates/rustfix/tests/separate/multiple.json new file mode 100644 index 000000000000..34cd60859d1b --- /dev/null +++ b/crates/rustfix/tests/separate/multiple.json @@ -0,0 +1,280 @@ +{ + "$message_type": "diagnostic", + "message": "unused variable: `unused`", + "code": { + "code": "unused_variables", + "explanation": null + }, + "level": "warning", + "spans": [ + { + "file_name": "./tests/separate/multiple.rs", + "byte_start": 154, + "byte_end": 160, + "line_start": 7, + "line_end": 7, + "column_start": 9, + "column_end": 15, + "is_primary": true, + "text": [ + { + "text": " let unused = a;", + "highlight_start": 9, + "highlight_end": 15 + } + ], + "label": null, + "suggested_replacement": null, + "suggestion_applicability": null, + "expansion": null + } + ], + "children": [ + { + "message": "`#[warn(unused_variables)]` on by default", + "code": null, + "level": "note", + "spans": [], + "children": [], + "rendered": null + }, + { + "message": "if this is intentional, prefix it with an underscore", + "code": null, + "level": "help", + "spans": [ + { + "file_name": "./tests/separate/multiple.rs", + "byte_start": 154, + "byte_end": 160, + "line_start": 7, + "line_end": 7, + "column_start": 9, + "column_end": 15, + "is_primary": true, + "text": [ + { + "text": " let unused = a;", + "highlight_start": 9, + "highlight_end": 15 + } + ], + "label": null, + "suggested_replacement": "_unused", + "suggestion_applicability": "MaybeIncorrect", + "expansion": null + } + ], + "children": [], + "rendered": null + } + ], + "rendered": "warning: unused variable: `unused`\n --> ./tests/separate/multiple.rs:7:9\n |\n7 | let unused = a;\n | ^^^^^^ help: if this is intentional, prefix it with an underscore: `_unused`\n |\n = note: `#[warn(unused_variables)]` on by default\n\n" +} +{ + "$message_type": "diagnostic", + "message": "variable appears on both sides of an assignment operation", + "code": { + "code": "clippy::misrefactored_assign_op", + "explanation": null + }, + "level": "warning", + "spans": [ + { + "file_name": "./tests/separate/multiple.rs", + "byte_start": 35, + "byte_end": 45, + "line_start": 3, + "line_end": 3, + "column_start": 5, + "column_end": 15, + "is_primary": true, + "text": [ + { + "text": " a += a + 1;", + "highlight_start": 5, + "highlight_end": 15 + } + ], + "label": null, + "suggested_replacement": null, + "suggestion_applicability": null, + "expansion": null + } + ], + "children": [ + { + "message": "`#[warn(clippy::misrefactored_assign_op)]` on by default", + "code": null, + "level": "note", + "spans": [], + "children": [], + "rendered": null + }, + { + "message": "did you mean `a = a + 1` or `a = a + a + 1`? Consider replacing it with", + "code": null, + "level": "help", + "spans": [ + { + "file_name": "./tests/separate/multiple.rs", + "byte_start": 35, + "byte_end": 45, + "line_start": 3, + "line_end": 3, + "column_start": 5, + "column_end": 15, + "is_primary": true, + "text": [ + { + "text": " a += a + 1;", + "highlight_start": 5, + "highlight_end": 15 + } + ], + "label": null, + "suggested_replacement": "a += 1", + "suggestion_applicability": "MaybeIncorrect", + "expansion": null + } + ], + "children": [], + "rendered": null + }, + { + "message": "or", + "code": null, + "level": "help", + "spans": [ + { + "file_name": "./tests/separate/multiple.rs", + "byte_start": 35, + "byte_end": 45, + "line_start": 3, + "line_end": 3, + "column_start": 5, + "column_end": 15, + "is_primary": true, + "text": [ + { + "text": " a += a + 1;", + "highlight_start": 5, + "highlight_end": 15 + } + ], + "label": null, + "suggested_replacement": "a = a + a + 1", + "suggestion_applicability": "MaybeIncorrect", + "expansion": null + } + ], + "children": [], + "rendered": null + } + ], + "rendered": "warning: variable appears on both sides of an assignment operation\n --> ./tests/separate/multiple.rs:3:5\n |\n3 | a += a + 1;\n | ^^^^^^^^^^\n |\n = note: `#[warn(clippy::misrefactored_assign_op)]` on by default\nhelp: did you mean `a = a + 1` or `a = a + a + 1`? Consider replacing it with\n |\n3 | a += 1;\n | ~~~~~~\nhelp: or\n |\n3 | a = a + a + 1;\n | ~~~~~~~~~~~~~\n\n" +} +{ + "$message_type": "diagnostic", + "message": "variable appears on both sides of an assignment operation", + "code": { + "code": "clippy::misrefactored_assign_op", + "explanation": null + }, + "level": "warning", + "spans": [ + { + "file_name": "./tests/separate/multiple.rs", + "byte_start": 51, + "byte_end": 61, + "line_start": 4, + "line_end": 4, + "column_start": 5, + "column_end": 15, + "is_primary": true, + "text": [ + { + "text": " a += a + 2;", + "highlight_start": 5, + "highlight_end": 15 + } + ], + "label": null, + "suggested_replacement": null, + "suggestion_applicability": null, + "expansion": null + } + ], + "children": [ + { + "message": "did you mean `a = a + 2` or `a = a + a + 2`? Consider replacing it with", + "code": null, + "level": "help", + "spans": [ + { + "file_name": "./tests/separate/multiple.rs", + "byte_start": 51, + "byte_end": 61, + "line_start": 4, + "line_end": 4, + "column_start": 5, + "column_end": 15, + "is_primary": true, + "text": [ + { + "text": " a += a + 2;", + "highlight_start": 5, + "highlight_end": 15 + } + ], + "label": null, + "suggested_replacement": "a += 2", + "suggestion_applicability": "MaybeIncorrect", + "expansion": null + } + ], + "children": [], + "rendered": null + }, + { + "message": "or", + "code": null, + "level": "help", + "spans": [ + { + "file_name": "./tests/separate/multiple.rs", + "byte_start": 51, + "byte_end": 61, + "line_start": 4, + "line_end": 4, + "column_start": 5, + "column_end": 15, + "is_primary": true, + "text": [ + { + "text": " a += a + 2;", + "highlight_start": 5, + "highlight_end": 15 + } + ], + "label": null, + "suggested_replacement": "a = a + a + 2", + "suggestion_applicability": "MaybeIncorrect", + "expansion": null + } + ], + "children": [], + "rendered": null + } + ], + "rendered": "warning: variable appears on both sides of an assignment operation\n --> ./tests/separate/multiple.rs:4:5\n |\n4 | a += a + 2;\n | ^^^^^^^^^^\n |\nhelp: did you mean `a = a + 2` or `a = a + a + 2`? Consider replacing it with\n |\n4 | a += 2;\n | ~~~~~~\nhelp: or\n |\n4 | a = a + a + 2;\n | ~~~~~~~~~~~~~\n\n" +} +{ + "$message_type": "diagnostic", + "message": "3 warnings emitted", + "code": null, + "level": "warning", + "spans": [], + "children": [], + "rendered": "warning: 3 warnings emitted\n\n" +} diff --git a/crates/rustfix/tests/separate/multiple.rs b/crates/rustfix/tests/separate/multiple.rs new file mode 100644 index 000000000000..c38811b9317b --- /dev/null +++ b/crates/rustfix/tests/separate/multiple.rs @@ -0,0 +1,8 @@ +fn main() { + let mut a = 0; + a += a + 1; + a += a + 2; + + // Should be `_unused` in `.fixed.0.rs` but left as `unused` in `.fixed.1.rs` + let unused = a; +} diff --git a/crates/rustfix/tests/separate/separate.fixed.0.rs b/crates/rustfix/tests/separate/separate.fixed.0.rs new file mode 100644 index 000000000000..4233d8c72e83 --- /dev/null +++ b/crates/rustfix/tests/separate/separate.fixed.0.rs @@ -0,0 +1,3 @@ +fn main() { + let _ = "\x1b[0m"; +} diff --git a/crates/rustfix/tests/separate/separate.fixed.1.rs b/crates/rustfix/tests/separate/separate.fixed.1.rs new file mode 100644 index 000000000000..1c5d28f415f0 --- /dev/null +++ b/crates/rustfix/tests/separate/separate.fixed.1.rs @@ -0,0 +1,3 @@ +fn main() { + let _ = "\x0033[0m"; +} diff --git a/crates/rustfix/tests/separate/separate.json b/crates/rustfix/tests/separate/separate.json new file mode 100644 index 000000000000..bb9f36cffd2a --- /dev/null +++ b/crates/rustfix/tests/separate/separate.json @@ -0,0 +1,120 @@ +{ + "$message_type": "diagnostic", + "message": "octal-looking escape in string literal", + "code": { + "code": "clippy::octal_escapes", + "explanation": null + }, + "level": "warning", + "spans": [ + { + "file_name": "./tests/separate/separate.rs", + "byte_start": 24, + "byte_end": 33, + "line_start": 2, + "line_end": 2, + "column_start": 13, + "column_end": 22, + "is_primary": true, + "text": [ + { + "text": " let _ = \"\\033[0m\";", + "highlight_start": 13, + "highlight_end": 22 + } + ], + "label": null, + "suggested_replacement": null, + "suggestion_applicability": null, + "expansion": null + } + ], + "children": [ + { + "message": "octal escapes are not supported, `\\0` is always a null character", + "code": null, + "level": "help", + "spans": [], + "children": [], + "rendered": null + }, + { + "message": "`#[warn(clippy::octal_escapes)]` on by default", + "code": null, + "level": "note", + "spans": [], + "children": [], + "rendered": null + }, + { + "message": "if an octal escape was intended, use the hexadecimal representation instead", + "code": null, + "level": "help", + "spans": [ + { + "file_name": "./tests/separate/separate.rs", + "byte_start": 24, + "byte_end": 33, + "line_start": 2, + "line_end": 2, + "column_start": 13, + "column_end": 22, + "is_primary": true, + "text": [ + { + "text": " let _ = \"\\033[0m\";", + "highlight_start": 13, + "highlight_end": 22 + } + ], + "label": null, + "suggested_replacement": "\"\\x1b[0m\"", + "suggestion_applicability": "MaybeIncorrect", + "expansion": null + } + ], + "children": [], + "rendered": null + }, + { + "message": "if the null character is intended, disambiguate using", + "code": null, + "level": "help", + "spans": [ + { + "file_name": "./tests/separate/separate.rs", + "byte_start": 24, + "byte_end": 33, + "line_start": 2, + "line_end": 2, + "column_start": 13, + "column_end": 22, + "is_primary": true, + "text": [ + { + "text": " let _ = \"\\033[0m\";", + "highlight_start": 13, + "highlight_end": 22 + } + ], + "label": null, + "suggested_replacement": "\"\\x0033[0m\"", + "suggestion_applicability": "MaybeIncorrect", + "expansion": null + } + ], + "children": [], + "rendered": null + } + ], + "rendered": "warning: octal-looking escape in string literal\n --> ./tests/separate/separate.rs:2:13\n |\n2 | let _ = \"\\033[0m\";\n | ^^^^^^^^^\n |\n = help: octal escapes are not supported, `\\0` is always a null character\n = note: `#[warn(clippy::octal_escapes)]` on by default\nhelp: if an octal escape was intended, use the hexadecimal representation instead\n |\n2 | let _ = \"\\x1b[0m\";\n | ~~~~~~~~~\nhelp: if the null character is intended, disambiguate using\n |\n2 | let _ = \"\\x0033[0m\";\n | ~~~~~~~~~~~\n\n" +} +{ + "$message_type": "diagnostic", + "message": "1 warning emitted", + "code": null, + "level": "warning", + "spans": [], + "children": [], + "rendered": "warning: 1 warning emitted\n\n" +} diff --git a/crates/rustfix/tests/separate/separate.rs b/crates/rustfix/tests/separate/separate.rs new file mode 100644 index 000000000000..7e8382b406e1 --- /dev/null +++ b/crates/rustfix/tests/separate/separate.rs @@ -0,0 +1,3 @@ +fn main() { + let _ = "\033[0m"; +}