From 1f707fff9dc41108fe0c7d88065dda809f048886 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Wed, 14 Jun 2023 22:40:55 -0500 Subject: [PATCH] Autofix Automatic/Suggested fixes with --fix --- crates/ruff/src/autofix/mod.rs | 16 ++- crates/ruff/src/linter.rs | 5 +- crates/ruff/src/settings/flags.rs | 6 +- crates/ruff/src/test.rs | 8 +- crates/ruff_cli/src/diagnostics.rs | 189 +++++++++++++++-------------- crates/ruff_cli/src/lib.rs | 7 +- crates/ruff_diagnostics/src/fix.rs | 8 +- 7 files changed, 133 insertions(+), 106 deletions(-) diff --git a/crates/ruff/src/autofix/mod.rs b/crates/ruff/src/autofix/mod.rs index a666171b54bfc5..0c9687ff6be035 100644 --- a/crates/ruff/src/autofix/mod.rs +++ b/crates/ruff/src/autofix/mod.rs @@ -5,7 +5,7 @@ use nohash_hasher::IntSet; use ruff_text_size::{TextLen, TextRange, TextSize}; use rustc_hash::FxHashMap; -use ruff_diagnostics::{Diagnostic, Edit, Fix, IsolationLevel}; +use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, IsolationLevel}; use ruff_python_ast::source_code::Locator; use crate::autofix::source_map::SourceMap; @@ -26,10 +26,20 @@ pub(crate) struct FixResult { } /// Auto-fix errors in a file, and write the fixed source code to disk. -pub(crate) fn fix_file(diagnostics: &[Diagnostic], locator: &Locator) -> Option { +pub(crate) fn fix_file( + diagnostics: &[Diagnostic], + locator: &Locator, + base_applicability: &Applicability, +) -> Option { let mut with_fixes = diagnostics .iter() - .filter(|diag| diag.fix.is_some()) + .filter(|diag| { + if let Some(fix) = &diag.fix { + &fix.applicability() >= base_applicability + } else { + false + } + }) .peekable(); if with_fixes.peek().is_none() { diff --git a/crates/ruff/src/linter.rs b/crates/ruff/src/linter.rs index ebdfded0813276..b1ec5ec62e42cf 100644 --- a/crates/ruff/src/linter.rs +++ b/crates/ruff/src/linter.rs @@ -10,7 +10,7 @@ use rustc_hash::FxHashMap; use rustpython_parser::lexer::LexResult; use rustpython_parser::ParseError; -use ruff_diagnostics::Diagnostic; +use ruff_diagnostics::{Applicability, Diagnostic}; use ruff_python_ast::imports::ImportMap; use ruff_python_ast::source_code::{Indexer, Locator, SourceFileBuilder, Stylist}; use ruff_python_stdlib::path::is_python_stub_file; @@ -399,6 +399,7 @@ pub fn lint_fix<'a>( noqa: flags::Noqa, settings: &Settings, source_kind: &mut SourceKind, + base_applicability: &Applicability, ) -> Result> { let mut transformed = Cow::Borrowed(contents); @@ -469,7 +470,7 @@ pub fn lint_fix<'a>( code: fixed_contents, fixes: applied, source_map, - }) = fix_file(&result.data.0, &locator) + }) = fix_file(&result.data.0, &locator, base_applicability) { if iterations < MAX_ITERATIONS { // Count the number of fixed errors. diff --git a/crates/ruff/src/settings/flags.rs b/crates/ruff/src/settings/flags.rs index a1ce403194c83d..aad86fa3d3f066 100644 --- a/crates/ruff/src/settings/flags.rs +++ b/crates/ruff/src/settings/flags.rs @@ -1,8 +1,10 @@ +use ruff_diagnostics::Applicability; + #[derive(Debug, Copy, Clone, Hash, is_macro::Is)] pub enum FixMode { Generate, - Apply, - Diff, + Apply(Applicability), + Diff(Applicability), } #[derive(Debug, Copy, Clone, Hash, result_like::BoolLike)] diff --git a/crates/ruff/src/test.rs b/crates/ruff/src/test.rs index 1e46037db71153..c42409df96d70a 100644 --- a/crates/ruff/src/test.rs +++ b/crates/ruff/src/test.rs @@ -141,8 +141,12 @@ fn test_contents(source_kind: &mut SourceKind, path: &Path, settings: &Settings) code: fixed_contents, source_map, .. - }) = fix_file(&diagnostics, &Locator::new(&contents)) - { + }) = fix_file( + &diagnostics, + &Locator::new(&contents), + // We treat Suggested/Automatic applicabilities as fixable, so take the superset. + &ruff_diagnostics::Applicability::Suggested, + ) { if iterations < max_iterations() { iterations += 1; } else { diff --git a/crates/ruff_cli/src/diagnostics.rs b/crates/ruff_cli/src/diagnostics.rs index 68c110851e4a97..d26596c3838b43 100644 --- a/crates/ruff_cli/src/diagnostics.rs +++ b/crates/ruff_cli/src/diagnostics.rs @@ -157,52 +157,56 @@ pub(crate) fn lint_path( error: parse_error, }, fixed, - ) = if matches!(autofix, flags::FixMode::Apply | flags::FixMode::Diff) { - if let Ok(FixerResult { - result, - transformed, - fixed, - }) = lint_fix( - &contents, - path, - package, - noqa, - &settings.lib, - &mut source_kind, - ) { - if !fixed.is_empty() { - match autofix { - flags::FixMode::Apply => match &source_kind { - SourceKind::Python(_) => { - write(path, transformed.as_bytes())?; - } - SourceKind::Jupyter(notebook) => { - notebook.write(path)?; + ) = match autofix { + flags::FixMode::Apply(base_applicability) | flags::FixMode::Diff(base_applicability) => { + if let Ok(FixerResult { + result, + transformed, + fixed, + }) = lint_fix( + &contents, + path, + package, + noqa, + &settings.lib, + &mut source_kind, + &base_applicability, + ) { + if !fixed.is_empty() { + match autofix { + flags::FixMode::Apply(_) => match &source_kind { + SourceKind::Python(_) => { + write(path, transformed.as_bytes())?; + } + SourceKind::Jupyter(notebook) => { + notebook.write(path)?; + } + }, + flags::FixMode::Diff(_) => { + let mut stdout = io::stdout().lock(); + TextDiff::from_lines(contents.as_str(), &transformed) + .unified_diff() + .header(&fs::relativize_path(path), &fs::relativize_path(path)) + .to_writer(&mut stdout)?; + stdout.write_all(b"\n")?; + stdout.flush()?; } - }, - flags::FixMode::Diff => { - let mut stdout = io::stdout().lock(); - TextDiff::from_lines(contents.as_str(), &transformed) - .unified_diff() - .header(&fs::relativize_path(path), &fs::relativize_path(path)) - .to_writer(&mut stdout)?; - stdout.write_all(b"\n")?; - stdout.flush()?; + flags::FixMode::Generate => {} } - flags::FixMode::Generate => {} } + (result, fixed) + } else { + // If we fail to autofix, lint the original source code. + let result = lint_only(&contents, path, package, &settings.lib, noqa); + let fixed = FxHashMap::default(); + (result, fixed) } - (result, fixed) - } else { - // If we fail to autofix, lint the original source code. + } + flags::FixMode::Generate => { let result = lint_only(&contents, path, package, &settings.lib, noqa); let fixed = FxHashMap::default(); (result, fixed) } - } else { - let result = lint_only(&contents, path, package, &settings.lib, noqa); - let fixed = FxHashMap::default(); - (result, fixed) }; let imports = imports.unwrap_or_default(); @@ -253,6 +257,7 @@ pub(crate) fn lint_stdin( autofix: flags::FixMode, ) -> Result { let mut source_kind = SourceKind::Python(contents.to_string()); + // Lint the inputs. let ( LinterResult { @@ -260,46 +265,66 @@ pub(crate) fn lint_stdin( error: parse_error, }, fixed, - ) = if matches!(autofix, flags::FixMode::Apply | flags::FixMode::Diff) { - if let Ok(FixerResult { - result, - transformed, - fixed, - }) = lint_fix( - contents, - path.unwrap_or_else(|| Path::new("-")), - package, - noqa, - settings, - &mut source_kind, - ) { - match autofix { - flags::FixMode::Apply => { - // Write the contents to stdout, regardless of whether any errors were fixed. - io::stdout().write_all(transformed.as_bytes())?; - } - flags::FixMode::Diff => { - // But only write a diff if it's non-empty. - if !fixed.is_empty() { - let text_diff = TextDiff::from_lines(contents, &transformed); - let mut unified_diff = text_diff.unified_diff(); - if let Some(path) = path { - unified_diff - .header(&fs::relativize_path(path), &fs::relativize_path(path)); - } + ) = match autofix { + flags::FixMode::Apply(base_applicability) | flags::FixMode::Diff(base_applicability) => { + if let Ok(FixerResult { + result, + transformed, + fixed, + }) = lint_fix( + contents, + path.unwrap_or_else(|| Path::new("-")), + package, + noqa, + settings, + &mut source_kind, + &base_applicability, + ) { + match autofix { + flags::FixMode::Apply(_) => { + // Write the contents to stdout, regardless of whether any errors were fixed. + io::stdout().write_all(transformed.as_bytes())?; + } + flags::FixMode::Diff(_) => { + // But only write a diff if it's non-empty. + if !fixed.is_empty() { + let text_diff = TextDiff::from_lines(contents, &transformed); + let mut unified_diff = text_diff.unified_diff(); + if let Some(path) = path { + unified_diff + .header(&fs::relativize_path(path), &fs::relativize_path(path)); + } - let mut stdout = io::stdout().lock(); - unified_diff.to_writer(&mut stdout)?; - stdout.write_all(b"\n")?; - stdout.flush()?; + let mut stdout = io::stdout().lock(); + unified_diff.to_writer(&mut stdout)?; + stdout.write_all(b"\n")?; + stdout.flush()?; + } } + flags::FixMode::Generate => {} } - flags::FixMode::Generate => {} - } - (result, fixed) - } else { - // If we fail to autofix, lint the original source code. + (result, fixed) + } else { + // If we fail to autofix, lint the original source code. + let result = lint_only( + contents, + path.unwrap_or_else(|| Path::new("-")), + package, + settings, + noqa, + ); + let fixed = FxHashMap::default(); + + // Write the contents to stdout anyway. + if autofix.is_apply() { + io::stdout().write_all(contents.as_bytes())?; + } + + (result, fixed) + } + } + flags::FixMode::Generate => { let result = lint_only( contents, path.unwrap_or_else(|| Path::new("-")), @@ -308,24 +333,8 @@ pub(crate) fn lint_stdin( noqa, ); let fixed = FxHashMap::default(); - - // Write the contents to stdout anyway. - if autofix.is_apply() { - io::stdout().write_all(contents.as_bytes())?; - } - (result, fixed) } - } else { - let result = lint_only( - contents, - path.unwrap_or_else(|| Path::new("-")), - package, - settings, - noqa, - ); - let fixed = FxHashMap::default(); - (result, fixed) }; let imports = imports.unwrap_or_default(); diff --git a/crates/ruff_cli/src/lib.rs b/crates/ruff_cli/src/lib.rs index 01b3e229e172ca..780f125d73635b 100644 --- a/crates/ruff_cli/src/lib.rs +++ b/crates/ruff_cli/src/lib.rs @@ -12,6 +12,7 @@ use ruff::logging::{set_up_logging, LogLevel}; use ruff::settings::types::SerializationFormat; use ruff::settings::{flags, CliSettings}; use ruff::{fs, warn_user_once}; +use ruff_diagnostics::Applicability; use ruff_python_formatter::format_module; use crate::args::{Args, CheckArgs, Command}; @@ -198,9 +199,9 @@ pub fn check(args: CheckArgs, log_level: LogLevel) -> Result { // - If `--diff` or `--fix-only` are set, don't print any violations (only // fixes). let autofix = if cli.diff { - flags::FixMode::Diff + flags::FixMode::Diff(Applicability::Automatic) } else if fix || fix_only { - flags::FixMode::Apply + flags::FixMode::Apply(Applicability::Automatic) } else { flags::FixMode::Generate }; @@ -339,7 +340,7 @@ pub fn check(args: CheckArgs, log_level: LogLevel) -> Result { // Always try to print violations (the printer itself may suppress output), // unless we're writing fixes via stdin (in which case, the transformed // source code goes to stdout). - if !(is_stdin && matches!(autofix, flags::FixMode::Apply | flags::FixMode::Diff)) { + if !(is_stdin && matches!(autofix, flags::FixMode::Apply(_) | flags::FixMode::Diff(_))) { if cli.statistics { printer.write_statistics(&diagnostics)?; } else { diff --git a/crates/ruff_diagnostics/src/fix.rs b/crates/ruff_diagnostics/src/fix.rs index ae54282d041db3..2192470f59154c 100644 --- a/crates/ruff_diagnostics/src/fix.rs +++ b/crates/ruff_diagnostics/src/fix.rs @@ -10,21 +10,21 @@ use crate::edit::Edit; pub enum Applicability { /// The fix is definitely what the user intended, or maintains the exact meaning of the code. /// This fix should be automatically applied. - Automatic, + Automatic = 2, /// The fix may be what the user intended, but it is uncertain. /// The fix should result in valid code if it is applied. /// The fix can be applied with user opt-in. - Suggested, + Suggested = 1, /// The fix has a good chance of being incorrect or the code be incomplete. /// The fix may result in invalid code if it is applied. /// The fix should only be manually applied by the user. - Manual, + Manual = -1, /// The applicability of the fix is unknown. #[default] - Unspecified, + Unspecified = 0, } /// Indicates the level of isolation required to apply a fix.