From 460a7e4a166b822386fcd45c2eeb45b454dd4fe6 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 23 Mar 2023 23:19:43 -0400 Subject: [PATCH] Allow diagnostics to generate multi-edit fixes --- Cargo.lock | 1 + crates/ruff/src/autofix/mod.rs | 58 +++++---- crates/ruff/src/checkers/ast/mod.rs | 2 +- crates/ruff/src/checkers/logical_lines.rs | 18 +-- crates/ruff/src/message.rs | 4 +- .../pyupgrade/rules/replace_stdout_stderr.rs | 118 ++++++------------ crates/ruff/src/test.rs | 2 +- crates/ruff_cli/src/printer.rs | 32 +++-- crates/ruff_diagnostics/src/diagnostic.rs | 20 +-- crates/ruff_diagnostics/src/edit.rs | 2 + crates/ruff_diagnostics/src/fix.rs | 45 +++++++ crates/ruff_diagnostics/src/lib.rs | 2 + crates/ruff_wasm/Cargo.toml | 8 +- crates/ruff_wasm/src/lib.rs | 29 ++--- crates/ruff_wasm/tests/api.rs | 3 +- 15 files changed, 190 insertions(+), 154 deletions(-) create mode 100644 crates/ruff_diagnostics/src/fix.rs diff --git a/Cargo.lock b/Cargo.lock index 36287f950793ed..31acae55cba0af 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2259,6 +2259,7 @@ dependencies = [ "js-sys", "log", "ruff", + "ruff_diagnostics", "ruff_python_ast", "ruff_rustpython", "rustpython-parser", diff --git a/crates/ruff/src/autofix/mod.rs b/crates/ruff/src/autofix/mod.rs index 0a0f3c44672fe5..308b819a9f2897 100644 --- a/crates/ruff/src/autofix/mod.rs +++ b/crates/ruff/src/autofix/mod.rs @@ -4,7 +4,7 @@ use itertools::Itertools; use rustc_hash::FxHashMap; use rustpython_parser::ast::Location; -use ruff_diagnostics::{Diagnostic, Edit}; +use ruff_diagnostics::{Diagnostic, Edit, Fix}; use ruff_python_ast::source_code::Locator; use ruff_python_ast::types::Range; @@ -15,7 +15,7 @@ pub mod helpers; /// Auto-fix errors in a file, and write the fixed source code to disk. pub fn fix_file(diagnostics: &[Diagnostic], locator: &Locator) -> Option<(String, FixTable)> { - if diagnostics.iter().all(|check| check.fix.is_none()) { + if diagnostics.iter().all(|check| check.fix.is_empty()) { None } else { Some(apply_fixes(diagnostics.iter(), locator)) @@ -34,36 +34,43 @@ fn apply_fixes<'a>( for (rule, fix) in diagnostics .filter_map(|diagnostic| { - diagnostic - .fix - .as_ref() - .map(|fix| (diagnostic.kind.rule(), fix)) + if diagnostic.fix.is_empty() { + None + } else { + Some((diagnostic.kind.rule(), &diagnostic.fix)) + } }) .sorted_by(|(rule1, fix1), (rule2, fix2)| cmp_fix(*rule1, *rule2, fix1, fix2)) { // If we already applied an identical fix as part of another correction, skip // any re-application. - if applied.contains(&fix) { + if fix.edits().iter().all(|edit| applied.contains(edit)) { *fixed.entry(rule).or_default() += 1; continue; } // Best-effort approach: if this fix overlaps with a fix we've already applied, // skip it. - if last_pos.map_or(false, |last_pos| last_pos >= fix.location) { + if last_pos.map_or(false, |last_pos| { + fix.location() + .map_or(false, |fix_location| last_pos >= fix_location) + }) { continue; } - // Add all contents from `last_pos` to `fix.location`. - let slice = locator.slice(Range::new(last_pos.unwrap_or_default(), fix.location)); - output.push_str(slice); + for edit in fix.edits() { + // Add all contents from `last_pos` to `fix.location`. + let slice = locator.slice(Range::new(last_pos.unwrap_or_default(), edit.location)); + output.push_str(slice); - // Add the patch itself. - output.push_str(&fix.content); + // Add the patch itself. + output.push_str(&edit.content); + + // Track that the edit was applied. + last_pos = Some(edit.end_location); + applied.insert(edit); + } - // Track that the fix was applied. - last_pos = Some(fix.end_location); - applied.insert(fix); *fixed.entry(rule).or_default() += 1; } @@ -93,9 +100,9 @@ pub(crate) fn apply_fix(fix: &Edit, locator: &Locator) -> String { } /// Compare two fixes. -fn cmp_fix(rule1: Rule, rule2: Rule, fix1: &Edit, fix2: &Edit) -> std::cmp::Ordering { - fix1.location - .cmp(&fix2.location) +fn cmp_fix(rule1: Rule, rule2: Rule, fix1: &Fix, fix2: &Fix) -> std::cmp::Ordering { + fix1.location() + .cmp(&fix2.location()) .then_with(|| match (&rule1, &rule2) { // Apply `EndsInPeriod` fixes before `NewLineAfterLastParagraph` fixes. (Rule::EndsInPeriod, Rule::NewLineAfterLastParagraph) => std::cmp::Ordering::Less, @@ -115,15 +122,14 @@ mod tests { use crate::autofix::{apply_fix, apply_fixes}; use crate::rules::pycodestyle::rules::MissingNewlineAtEndOfFile; - fn create_diagnostics(fixes: impl IntoIterator) -> Vec { - fixes - .into_iter() - .map(|fix| Diagnostic { + fn create_diagnostics(edit: impl IntoIterator) -> Vec { + edit.into_iter() + .map(|edit| Diagnostic { // The choice of rule here is arbitrary. kind: MissingNewlineAtEndOfFile.into(), - location: fix.location, - end_location: fix.end_location, - fix: Some(fix), + location: edit.location, + end_location: edit.end_location, + fix: edit.into(), parent: None, }) .collect() diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 1b855384cbe7e7..25530eb40cf972 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2455,7 +2455,7 @@ where pyupgrade::rules::replace_universal_newlines(self, func, keywords); } if self.settings.rules.enabled(Rule::ReplaceStdoutStderr) { - pyupgrade::rules::replace_stdout_stderr(self, expr, func, keywords); + pyupgrade::rules::replace_stdout_stderr(self, expr, func, args, keywords); } if self.settings.rules.enabled(Rule::OSErrorAlias) { pyupgrade::rules::os_error_alias_call(self, func); diff --git a/crates/ruff/src/checkers/logical_lines.rs b/crates/ruff/src/checkers/logical_lines.rs index 01c33efd0c800a..584e3d9b2a5ae8 100644 --- a/crates/ruff/src/checkers/logical_lines.rs +++ b/crates/ruff/src/checkers/logical_lines.rs @@ -5,7 +5,7 @@ use itertools::Itertools; use rustpython_parser::ast::Location; use rustpython_parser::lexer::LexResult; -use ruff_diagnostics::Diagnostic; +use ruff_diagnostics::{Diagnostic, Fix}; use ruff_python_ast::source_code::{Locator, Stylist}; use ruff_python_ast::types::Range; @@ -75,7 +75,7 @@ pub fn check_logical_lines( kind, location, end_location: location, - fix: None, + fix: Fix::empty(), parent: None, }); } @@ -93,7 +93,7 @@ pub fn check_logical_lines( kind, location, end_location: location, - fix: None, + fix: Fix::empty(), parent: None, }); } @@ -108,7 +108,7 @@ pub fn check_logical_lines( kind, location, end_location: location, - fix: None, + fix: Fix::empty(), parent: None, }); } @@ -120,7 +120,7 @@ pub fn check_logical_lines( kind, location, end_location: location, - fix: None, + fix: Fix::empty(), parent: None, }); } @@ -133,7 +133,7 @@ pub fn check_logical_lines( kind, location: range.location, end_location: range.end_location, - fix: None, + fix: Fix::empty(), parent: None, }); } @@ -148,7 +148,7 @@ pub fn check_logical_lines( kind, location, end_location: location, - fix: None, + fix: Fix::empty(), parent: None, }); } @@ -159,7 +159,7 @@ pub fn check_logical_lines( kind, location, end_location: location, - fix: None, + fix: Fix::empty(), parent: None, }); } @@ -210,7 +210,7 @@ pub fn check_logical_lines( kind, location, end_location: location, - fix: None, + fix: Fix::empty(), parent: None, }); } diff --git a/crates/ruff/src/message.rs b/crates/ruff/src/message.rs index 50765218ec656d..f837311fb050b1 100644 --- a/crates/ruff/src/message.rs +++ b/crates/ruff/src/message.rs @@ -3,7 +3,7 @@ use std::cmp::Ordering; pub use rustpython_parser::ast::Location; use serde::{Deserialize, Serialize}; -use ruff_diagnostics::{Diagnostic, DiagnosticKind, Edit}; +use ruff_diagnostics::{Diagnostic, DiagnosticKind, Fix}; use ruff_python_ast::source_code::Locator; use ruff_python_ast::types::Range; @@ -12,7 +12,7 @@ pub struct Message { pub kind: DiagnosticKind, pub location: Location, pub end_location: Location, - pub fix: Option, + pub fix: Fix, pub filename: String, pub source: Option, pub noqa_row: usize, diff --git a/crates/ruff/src/rules/pyupgrade/rules/replace_stdout_stderr.rs b/crates/ruff/src/rules/pyupgrade/rules/replace_stdout_stderr.rs index 6cb31281ee6e82..cc7dbec4a38b08 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/replace_stdout_stderr.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/replace_stdout_stderr.rs @@ -1,12 +1,13 @@ +use anyhow::Result; use rustpython_parser::ast::{Expr, Keyword}; -use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit}; +use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::find_keyword; -use ruff_python_ast::source_code::{Locator, Stylist}; +use ruff_python_ast::source_code::Locator; use ruff_python_ast::types::Range; -use ruff_python_ast::whitespace::indentation; +use crate::autofix::helpers::remove_argument; use crate::checkers::ast::Checker; use crate::registry::AsRule; @@ -24,87 +25,46 @@ impl AlwaysAutofixableViolation for ReplaceStdoutStderr { } } -#[derive(Debug)] -struct MiddleContent<'a> { - contents: &'a str, - multi_line: bool, -} - -/// Return the number of "dirty" characters. -fn dirty_count(iter: impl Iterator) -> usize { - let mut the_count = 0; - for current_char in iter { - if current_char == ' ' - || current_char == ',' - || current_char == '\n' - || current_char == '\r' - { - the_count += 1; - } else { - break; - } - } - the_count -} - -/// Extract the `Middle` content between two arguments. -fn extract_middle(contents: &str) -> Option { - let multi_line = contents.contains('\n'); - let start_gap = dirty_count(contents.chars()); - if contents.len() == start_gap { - return None; - } - let end_gap = dirty_count(contents.chars().rev()); - Some(MiddleContent { - contents: &contents[start_gap..contents.len() - end_gap], - multi_line, - }) -} - /// Generate a [`Edit`] for a `stdout` and `stderr` [`Keyword`] pair. fn generate_fix( - stylist: &Stylist, locator: &Locator, + func: &Expr, + args: &[Expr], + keywords: &[Keyword], stdout: &Keyword, stderr: &Keyword, -) -> Option { - let line_end = stylist.line_ending().as_str(); - let first = if stdout.location < stderr.location { - stdout - } else { - stderr - }; - let last = if stdout.location > stderr.location { - stdout +) -> Result { + let (first, second) = if stdout.location < stderr.location { + (stdout, stderr) } else { - stderr + (stderr, stdout) }; - let mut contents = String::from("capture_output=True"); - if let Some(middle) = - extract_middle(locator.slice(Range::new(first.end_location.unwrap(), last.location))) - { - if middle.multi_line { - let Some(indent) = indentation(locator, first) else { - return None; - }; - contents.push(','); - contents.push_str(line_end); - contents.push_str(indent); - } else { - contents.push(','); - contents.push(' '); - } - contents.push_str(middle.contents); - } - Some(Edit::replacement( - contents, - first.location, - last.end_location.unwrap(), - )) + Ok(Fix::new(vec![ + Edit::replacement( + "capture_output=True".to_string(), + first.location, + first.end_location.unwrap(), + ), + remove_argument( + locator, + func.location, + second.location, + second.end_location.unwrap(), + args, + keywords, + false, + )?, + ])) } /// UP022 -pub fn replace_stdout_stderr(checker: &mut Checker, expr: &Expr, func: &Expr, kwargs: &[Keyword]) { +pub fn replace_stdout_stderr( + checker: &mut Checker, + expr: &Expr, + func: &Expr, + args: &[Expr], + keywords: &[Keyword], +) { if checker .ctx .resolve_call_path(func) @@ -113,10 +73,10 @@ pub fn replace_stdout_stderr(checker: &mut Checker, expr: &Expr, func: &Expr, kw }) { // Find `stdout` and `stderr` kwargs. - let Some(stdout) = find_keyword(kwargs, "stdout") else { + let Some(stdout) = find_keyword(keywords, "stdout") else { return; }; - let Some(stderr) = find_keyword(kwargs, "stderr") else { + let Some(stderr) = find_keyword(keywords, "stderr") else { return; }; @@ -139,9 +99,9 @@ pub fn replace_stdout_stderr(checker: &mut Checker, expr: &Expr, func: &Expr, kw let mut diagnostic = Diagnostic::new(ReplaceStdoutStderr, Range::from(expr)); if checker.patch(diagnostic.kind.rule()) { - if let Some(fix) = generate_fix(checker.stylist, checker.locator, stdout, stderr) { - diagnostic.set_fix(fix); - }; + diagnostic.try_set_fix(|| { + generate_fix(checker.locator, func, args, keywords, stdout, stderr) + }); } checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff/src/test.rs b/crates/ruff/src/test.rs index b6758e9d5c79d4..43d7519df981c5 100644 --- a/crates/ruff/src/test.rs +++ b/crates/ruff/src/test.rs @@ -51,7 +51,7 @@ pub fn test_path(path: impl AsRef, settings: &Settings) -> Result { +struct ExpandedEdit<'a> { content: &'a str, - message: Option<&'a str>, location: &'a Location, end_location: &'a Location, } +#[derive(Serialize)] +struct ExpandedFix<'a> { + message: Option<&'a str>, + edits: Vec>, +} + #[derive(Serialize)] struct ExpandedMessage<'a> { code: SerializeRuleAsCode, @@ -197,12 +202,23 @@ impl Printer { .map(|message| ExpandedMessage { code: message.kind.rule().into(), message: message.kind.body.clone(), - fix: message.fix.as_ref().map(|fix| ExpandedFix { - content: &fix.content, - location: &fix.location, - end_location: &fix.end_location, - message: message.kind.suggestion.as_deref(), - }), + fix: if message.fix.is_empty() { + None + } else { + Some(ExpandedFix { + message: message.kind.suggestion.as_deref(), + edits: message + .fix + .edits() + .iter() + .map(|edit| ExpandedEdit { + content: &edit.content, + location: &edit.location, + end_location: &edit.end_location, + }) + .collect(), + }) + }, location: message.location, end_location: message.end_location, filename: &message.filename, diff --git a/crates/ruff_diagnostics/src/diagnostic.rs b/crates/ruff_diagnostics/src/diagnostic.rs index 3344b246623a5c..61856cdb8e5119 100644 --- a/crates/ruff_diagnostics/src/diagnostic.rs +++ b/crates/ruff_diagnostics/src/diagnostic.rs @@ -6,7 +6,7 @@ use serde::{Deserialize, Serialize}; use ruff_python_ast::types::Range; -use crate::edit::Edit; +use crate::Fix; #[derive(Debug, PartialEq, Eq)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] @@ -27,31 +27,31 @@ pub struct Diagnostic { pub kind: DiagnosticKind, pub location: Location, pub end_location: Location, - pub fix: Option, + pub fix: Fix, pub parent: Option, } impl Diagnostic { - pub fn new>(kind: K, range: Range) -> Self { + pub fn new>(kind: T, range: Range) -> Self { Self { kind: kind.into(), location: range.location, end_location: range.end_location, - fix: None, + fix: Fix::empty(), parent: None, } } - /// Set the [`Edit`] used to fix the diagnostic. - pub fn set_fix(&mut self, fix: Edit) { - self.fix = Some(fix); + /// Set the [`Fix`] used to fix the diagnostic. + pub fn set_fix>(&mut self, fix: T) { + self.fix = fix.into(); } - /// Set the [`Edit`] used to fix the diagnostic, if the provided function returns `Ok`. + /// Set the [`Fix`] used to fix the diagnostic, if the provided function returns `Ok`. /// Otherwise, log the error. - pub fn try_set_fix(&mut self, func: impl FnOnce() -> Result) { + pub fn try_set_fix>(&mut self, func: impl FnOnce() -> Result) { match func() { - Ok(fix) => self.fix = Some(fix), + Ok(fix) => self.fix = fix.into(), Err(err) => error!("Failed to create fix: {}", err), } } diff --git a/crates/ruff_diagnostics/src/edit.rs b/crates/ruff_diagnostics/src/edit.rs index e2f4d30b8f00d6..8c0ae0e0bb81fb 100644 --- a/crates/ruff_diagnostics/src/edit.rs +++ b/crates/ruff_diagnostics/src/edit.rs @@ -2,6 +2,8 @@ use rustpython_parser::ast::Location; #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; +/// A text edit to be applied to a source file. Inserts, deletes, or replaces +/// content at a given location. #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] pub struct Edit { diff --git a/crates/ruff_diagnostics/src/fix.rs b/crates/ruff_diagnostics/src/fix.rs new file mode 100644 index 00000000000000..15cac436e2d3b4 --- /dev/null +++ b/crates/ruff_diagnostics/src/fix.rs @@ -0,0 +1,45 @@ +use rustpython_parser::ast::Location; +#[cfg(feature = "serde")] +use serde::{Deserialize, Serialize}; + +use crate::edit::Edit; + +/// A collection of [`Edit`] elements to be applied to a source file. +#[derive(Default, Debug, PartialEq, Eq)] +#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] +pub struct Fix { + edits: Vec, +} + +impl Fix { + /// Create a new [`Fix`] from a vector of [`Edit`] elements. + pub fn new(edits: Vec) -> Self { + Self { edits } + } + + /// Create an empty [`Fix`]. + pub const fn empty() -> Self { + Self { edits: Vec::new() } + } + + /// Return `true` if the [`Fix`] contains no [`Edit`] elements. + pub fn is_empty(&self) -> bool { + self.edits.is_empty() + } + + /// Return the [`Location`] of the first [`Edit`] in the [`Fix`]. + pub fn location(&self) -> Option { + self.edits.iter().map(|edit| edit.location).min() + } + + /// Return a slice of the [`Edit`] elements in the [`Fix`]. + pub fn edits(&self) -> &[Edit] { + &self.edits + } +} + +impl From for Fix { + fn from(edit: Edit) -> Self { + Self { edits: vec![edit] } + } +} diff --git a/crates/ruff_diagnostics/src/lib.rs b/crates/ruff_diagnostics/src/lib.rs index 4c5f32bd144c9c..b3b1ee2567a510 100644 --- a/crates/ruff_diagnostics/src/lib.rs +++ b/crates/ruff_diagnostics/src/lib.rs @@ -1,7 +1,9 @@ pub use diagnostic::{Diagnostic, DiagnosticKind}; pub use edit::Edit; +pub use fix::Fix; pub use violation::{AlwaysAutofixableViolation, AutofixKind, Violation}; mod diagnostic; mod edit; +mod fix; mod violation; diff --git a/crates/ruff_wasm/Cargo.toml b/crates/ruff_wasm/Cargo.toml index a6fcd0b916bacf..dcb3b7bc6efe5e 100644 --- a/crates/ruff_wasm/Cargo.toml +++ b/crates/ruff_wasm/Cargo.toml @@ -13,17 +13,19 @@ crate-type = ["cdylib", "rlib"] default = ["console_error_panic_hook"] [dependencies] +ruff_diagnostics = { path = "../ruff_diagnostics" } +ruff_python_ast = { path = "../ruff_python_ast" } +ruff_rustpython = { path = "../ruff_rustpython" } + +console_error_panic_hook = { version = "0.1.7", optional = true } console_log = { version = "0.2.1" } getrandom = { version = "0.2.8", features = ["js"] } log = { workspace = true } ruff = { path = "../ruff" } -ruff_python_ast = { path = "../ruff_python_ast" } -ruff_rustpython = { path = "../ruff_rustpython" } rustpython-parser = { workspace = true } serde = { workspace = true } serde-wasm-bindgen = { version = "0.5.0" } wasm-bindgen = { version = "0.2.84" } -console_error_panic_hook = { version = "0.1.7", optional = true } [dev-dependencies] js-sys = { version = "0.3.61" } diff --git a/crates/ruff_wasm/src/lib.rs b/crates/ruff_wasm/src/lib.rs index 77eaec6e4118e1..1a188fb57d86b6 100644 --- a/crates/ruff_wasm/src/lib.rs +++ b/crates/ruff_wasm/src/lib.rs @@ -17,6 +17,7 @@ use ruff::rules::{ use ruff::settings::configuration::Configuration; use ruff::settings::options::Options; use ruff::settings::{defaults, flags, Settings}; +use ruff_diagnostics::Edit; use ruff_python_ast::source_code::{Indexer, Locator, Stylist}; const VERSION: &str = env!("CARGO_PKG_VERSION"); @@ -49,6 +50,12 @@ export interface Diagnostic { }; "#; +#[derive(Serialize, Deserialize, Eq, PartialEq, Debug)] +pub struct ExpandedFix { + message: Option, + edits: Vec, +} + #[derive(Serialize, Deserialize, Eq, PartialEq, Debug)] pub struct ExpandedMessage { pub code: String, @@ -58,14 +65,6 @@ pub struct ExpandedMessage { pub fix: Option, } -#[derive(Serialize, Deserialize, Eq, PartialEq, Debug)] -pub struct ExpandedFix { - content: String, - message: Option, - location: Location, - end_location: Location, -} - #[wasm_bindgen(start)] pub fn run() { use log::Level; @@ -204,12 +203,14 @@ pub fn check(contents: &str, options: JsValue) -> Result { message: message.kind.body, location: message.location, end_location: message.end_location, - fix: message.fix.map(|fix| ExpandedFix { - content: fix.content, - message: message.kind.suggestion, - location: fix.location, - end_location: fix.end_location, - }), + fix: if message.fix.is_empty() { + None + } else { + Some(ExpandedFix { + message: message.kind.suggestion, + edits: message.fix.edits().to_vec(), + }) + }, }) .collect(); diff --git a/crates/ruff_wasm/tests/api.rs b/crates/ruff_wasm/tests/api.rs index 432a86f3ea2e8f..b2bdf04dae825e 100644 --- a/crates/ruff_wasm/tests/api.rs +++ b/crates/ruff_wasm/tests/api.rs @@ -5,6 +5,7 @@ use rustpython_parser::ast::Location; use wasm_bindgen_test::*; use ruff::registry::Rule; +use ruff_diagnostics::Fix; use ruff_wasm::*; macro_rules! check { @@ -30,7 +31,7 @@ fn empty_config() { message: "If test is a tuple, which is always `True`".to_string(), location: Location::new(1, 0), end_location: Location::new(2, 8), - fix: None, + fix: Fix::empty(), }] ); }