From 29d243ee1dea6efd7e1fe808cf99d5a9f01f446b Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Wed, 12 Jul 2023 20:45:53 -0500 Subject: [PATCH] Support --unsafe option with --fix --- crates/ruff_cli/src/printer.rs | 106 +++++++++++++++++----- crates/ruff_cli/tests/integration_test.rs | 58 +++++++++++- 2 files changed, 138 insertions(+), 26 deletions(-) diff --git a/crates/ruff_cli/src/printer.rs b/crates/ruff_cli/src/printer.rs index 0c8070e6e455cc..e0c00d2224a41f 100644 --- a/crates/ruff_cli/src/printer.rs +++ b/crates/ruff_cli/src/printer.rs @@ -118,19 +118,14 @@ impl Printer { writeln!(writer, "Found {remaining} error{s}.")?; } - if show_fix_status(self.autofix_level) { - let num_fixable = diagnostics - .messages - .iter() - .filter(|message| message.fix.is_some()) - .count(); - if num_fixable > 0 { - writeln!( - writer, - "[{}] {num_fixable} potentially fixable with the --fix option.", - "*".cyan(), - )?; - } + if let Some((safe_remaining, unsafe_remaining)) = + applicable_fixes(self.autofix_level, diagnostics) + { + writeln!( + writer, + "{}", + display_violations(safe_remaining, unsafe_remaining) + )?; } } else { let fixed = diagnostics @@ -389,14 +384,57 @@ fn num_digits(n: usize) -> usize { .max(1) } -/// Return `true` if the [`Printer`] should indicate that a rule is fixable. -const fn show_fix_status(autofix_level: flags::FixMode) -> bool { - // If we're in application mode, avoid indicating that a rule is fixable. - // If the specific violation were truly fixable, it would've been fixed in - // this pass! (We're occasionally unable to determine whether a specific - // violation is fixable without trying to fix it, so if autofix is not - // enabled, we may inadvertently indicate that a rule is fixable.) - !autofix_level.is_apply() +/// Return the number of safe ([`Applicability::Automatic`]) and unsafe ([`Applicability::Suggested`]) +/// fixes generated from a given [`Diagnostics`], contingent upon the mode/applicability levels +/// passed by the user. Returns [`None`] if there are no applicable fixes. +fn applicable_fixes( + autofix_level: flags::FixMode, + diagnostics: &Diagnostics, +) -> Option<(i32, i32)> { + let mut safe_remaining = 0; + let mut unsafe_remaining = 0; + diagnostics.messages.iter().for_each(|msg| { + if let Some(fix) = &msg.fix { + if fix.applicability() == Applicability::Suggested { + unsafe_remaining += 1; + } else if fix.applicability() == Applicability::Automatic { + safe_remaining += 1; + } + } + }); + + // If we're in apply mode, calculate the status based on the applicability level of the + // fixes in question. If the user passed `--fix --unsafe`, we can safely say that all fixes + // should've been applied in this pass, so we do not want to show the status (We're occasionally + // unable to determine whether a specific violation is fixable without trying to fix it, so + // if autofix is not enabled, we may inadvertently indicate that a rule is fixable.). + // + // If the user passed `--fix`, unsafe fixes (if any were generated) would not have been + // applied. If this is the case, the fix status message will depend on the types of fixes that + // remain, so we'll have to return both. + // + // If we're NOT in apply mode, we want to only show a status if any fixes, regardless of + // safety, exist. + match autofix_level { + flags::FixMode::Apply(should_fix_unsafe) => { + if should_fix_unsafe { + None + } else { + if safe_remaining > 0 || unsafe_remaining > 0 { + Some((safe_remaining, unsafe_remaining)) + } else { + None + } + } + } + _ => { + if safe_remaining > 0 || unsafe_remaining > 0 { + Some((safe_remaining, unsafe_remaining)) + } else { + None + } + } + } } fn print_fix_summary(writer: &mut dyn Write, fixed: &FxHashMap) -> Result<()> { @@ -439,3 +477,29 @@ fn print_fix_summary(writer: &mut dyn Write, fixed: &FxHashMap } Ok(()) } + +/// Build the displayed fix status message depending on the types of the remaining fixes. +fn display_violations<'a>(safe_remaining: i32, unsafe_remaining: i32) -> String { + let prefix = format!("[{}]", "*".cyan()); + let mut fix_status = prefix; + + if safe_remaining > 0 { + fix_status = + format!("{fix_status} {safe_remaining} potentially fixable with the --fix option."); + } + if unsafe_remaining > 0 { + let (line_break, extra_prefix) = if safe_remaining > 0 { + // Avoid re-allocating prefix. + ("\n", format!("[{}]", "*".cyan())) + } else { + ("", String::new()) + }; + + let total = safe_remaining + unsafe_remaining; + fix_status = format!( + "{fix_status}{line_break}{extra_prefix} {total} potentially fixable with the --fix --unsafe options." + ); + } + + fix_status.to_owned() +} diff --git a/crates/ruff_cli/tests/integration_test.rs b/crates/ruff_cli/tests/integration_test.rs index 7ff5b7f7533b97..cf7e3e61759a42 100644 --- a/crates/ruff_cli/tests/integration_test.rs +++ b/crates/ruff_cli/tests/integration_test.rs @@ -280,7 +280,7 @@ Found 1 error. } #[test] -fn applies_safe_fixes_only() -> Result<()> { +fn different_safety_levels() -> Result<()> { let mut cmd = Command::cargo_bin(BIN_NAME)?; // `--fix` should only apply safe fixes, but should tell the user about `--fix --unsafe` if @@ -292,23 +292,71 @@ fn applies_safe_fixes_only() -> Result<()> { "text", "--isolated", "--select", - "F601", - "--fix", + "F601,UP034", ]) .write_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n") .assert() .failure(); assert_eq!( str::from_utf8(&output.get_output().stdout)?, - r#"-:1:11: F601 Dictionary key `'a'` repeated + r#"-:1:14: F601 [*] Dictionary key literal `'a'` repeated +-:2:7: UP034 [*] Avoid extraneous parentheses +Found 2 errors. +[*] 1 potentially fixable with the --fix option. +[*] 2 potentially fixable with the --fix --unsafe options. +"# + ); + + Ok(()) +} + +#[test] +fn only_unsafe_fixes_remain() -> Result<()> { + let mut cmd = Command::cargo_bin(BIN_NAME)?; + + let output = cmd + .args(["-", "--format", "text", "--isolated", "--select", "F601"]) + .write_stdin("x = {'a': 1, 'a': 1}\n") + .assert() + .failure(); + assert_eq!( + str::from_utf8(&output.get_output().stdout)?, + r#"-:1:14: F601 [*] Dictionary key literal `'a'` repeated Found 1 error. -1 potentially fixable with the --fix --unsafe options. +[*] 1 potentially fixable with the --fix --unsafe options. "# ); Ok(()) } +#[test] +fn applies_safe_fixes_only() -> Result<()> { + let mut cmd = Command::cargo_bin(BIN_NAME)?; + + // `--fix` should only apply safe fixes. Since we're runnnig in `stdin` mode, output shouldn't + // be printed. + let output = cmd + .args([ + "-", + "--format", + "text", + "--isolated", + "--select", + "F601,UP034", + "--fix", + ]) + .write_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n") + .assert() + .failure(); + assert_eq!( + str::from_utf8(&output.get_output().stdout)?, + "x = {'a': 1, 'a': 1}\nprint('foo')\n" + ); + + Ok(()) +} + #[test] fn applies_all_fixes() -> Result<()> { let mut cmd = Command::cargo_bin(BIN_NAME)?;