Support --unsafe option with --fix

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};

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,7 +187,9 @@ impl Printer {
             }
             SerializationFormat::Text => {
                 TextEmitter::default()
-                    .with_show_fix_status(show_fix_status(self.autofix_level))
+                    .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)?;
@@ -209,7 +207,9 @@ impl Printer {
             SerializationFormat::Grouped => {
                 GroupedEmitter::default()
                     .with_show_source(self.flags.contains(Flags::SHOW_SOURCE))
-                    .with_show_fix_status(show_fix_status(self.autofix_level))
+                    .with_show_fix_status(
+                        applicable_fixes(self.autofix_level, diagnostics).is_some(),
+                    )
                     .emit(writer, &diagnostics.messages, &context)?;
 
                 if self.flags.contains(Flags::SHOW_FIX_SUMMARY) {
@@ -366,7 +366,7 @@ impl Printer {
 
             let context = EmitterContext::new(&diagnostics.source_kind);
             TextEmitter::default()
-                .with_show_fix_status(show_fix_status(self.autofix_level))
+                .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)?;
         }
@@ -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<String, FixTable>) -> Result<()> {
@@ -439,3 +482,29 @@ fn print_fix_summary(writer: &mut dyn Write, fixed: &FxHashMap<String, FixTable>
     }
     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()
+}

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)?;