From 21e51adc7e67b6a2bf99c64b8cb98d2f42e9120c 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/lib.rs | 1 - crates/ruff_cli/src/printer.rs | 125 +++++++++++++++++----- crates/ruff_cli/tests/integration_test.rs | 58 +++++++++- 3 files changed, 150 insertions(+), 34 deletions(-) diff --git a/crates/ruff_cli/src/lib.rs b/crates/ruff_cli/src/lib.rs index cfda3e73c2765d..2b9b3bac55731e 100644 --- a/crates/ruff_cli/src/lib.rs +++ b/crates/ruff_cli/src/lib.rs @@ -13,7 +13,6 @@ 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, PyFormatOptions}; use crate::args::{Args, CheckArgs, Command}; diff --git a/crates/ruff_cli/src/printer.rs b/crates/ruff_cli/src/printer.rs index c1a06088909ada..c3a1716b0d84d0 100644 --- a/crates/ruff_cli/src/printer.rs +++ b/crates/ruff_cli/src/printer.rs @@ -7,6 +7,7 @@ use anyhow::Result; use bitflags::bitflags; use colored::Colorize; use itertools::{iterate, Itertools}; +use ruff_diagnostics::Applicability; use rustc_hash::FxHashMap; use serde::Serialize; @@ -118,19 +119,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 @@ -191,9 +187,11 @@ impl Printer { } SerializationFormat::Text => { TextEmitter::default() - .with_show_fix_status(show_fix_status(self.autofix_level)) - .with_show_fix_diff(self.flags.intersects(Flags::SHOW_FIX_DIFF)) - .with_show_source(self.flags.intersects(Flags::SHOW_SOURCE)) + .with_show_fix_status( + applicable_fixes(self.autofix_level, diagnostics).is_some(), + ) + .with_show_fix_diff(self.flags.contains(Flags::SHOW_FIX_DIFF)) + .with_show_source(self.flags.contains(Flags::SHOW_SOURCE)) .emit(writer, &diagnostics.messages, &context)?; if self.flags.intersects(Flags::SHOW_FIX_SUMMARY) { @@ -208,8 +206,10 @@ impl Printer { } SerializationFormat::Grouped => { GroupedEmitter::default() - .with_show_source(self.flags.intersects(Flags::SHOW_SOURCE)) - .with_show_fix_status(show_fix_status(self.autofix_level)) + .with_show_source(self.flags.contains(Flags::SHOW_SOURCE)) + .with_show_fix_status( + applicable_fixes(self.autofix_level, diagnostics).is_some(), + ) .emit(writer, &diagnostics.messages, &context)?; if self.flags.intersects(Flags::SHOW_FIX_SUMMARY) { @@ -366,8 +366,8 @@ impl Printer { let context = EmitterContext::new(&diagnostics.source_kind); TextEmitter::default() - .with_show_fix_status(show_fix_status(self.autofix_level)) - .with_show_source(self.flags.intersects(Flags::SHOW_SOURCE)) + .with_show_fix_status(applicable_fixes(self.autofix_level, diagnostics).is_some()) + .with_show_source(self.flags.contains(Flags::SHOW_SOURCE)) .emit(writer, &diagnostics.messages, &context)?; } writer.flush()?; @@ -389,14 +389,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 +482,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 e0a1d8cf1227c1..b8212680498e6d 100644 --- a/crates/ruff_cli/tests/integration_test.rs +++ b/crates/ruff_cli/tests/integration_test.rs @@ -350,7 +350,7 @@ fn unreadable_dir() -> Result<()> { } #[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 @@ -362,23 +362,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)?;