Update CLI to respect fix applicability (#7769)

Rebase of #5119 authored by @evanrittenhouse with additional refinements. ## Changes - Adds `--unsafe-fixes` / `--no-unsafe-fixes` flags to `ruff check` - Violations with unsafe fixes are not shown as fixable unless opted-in - Fix applicability is respected now - `Applicability::Never` fixes are no longer applied - `Applicability::Sometimes` fixes require opt-in - `Applicability::Always` fixes are unchanged - Hints for availability of `--unsafe-fixes` added to `ruff check` output ## Examples Check hints at hidden unsafe fixes ``` ❯ ruff check example.py --no-cache --select F601,W292 example.py:1:14: F601 Dictionary key literal `'a'` repeated example.py:2:15: W292 [*] No newline at end of file Found 2 errors. [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). ``` We could add an indicator for which violations have hidden fixes in the future. Check treats unsafe fixes as applicable with opt-in ``` ❯ ruff check example.py --no-cache --select F601,W292 --unsafe-fixes example.py:1:14: F601 [*] Dictionary key literal `'a'` repeated example.py:2:15: W292 [*] No newline at end of file Found 2 errors. [*] 2 fixable with the --fix option. ``` Also can be enabled in the config file ``` ❯ cat ruff.toml unsafe-fixes = true ``` And opted-out per invocation ``` ❯ ruff check example.py --no-cache --select F601,W292 --no-unsafe-fixes example.py:1:14: F601 Dictionary key literal `'a'` repeated example.py:2:15: W292 [*] No newline at end of file Found 2 errors. [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). ``` Diff does not include unsafe fixes ``` ❯ ruff check example.py --no-cache --select F601,W292 --diff --- example.py +++ example.py @@ -1,2 +1,2 @@ x = {'a': 1, 'a': 1} -print(('foo')) +print(('foo')) \ No newline at end of file Would fix 1 error. ``` Unless there is opt-in ``` ❯ ruff check example.py --no-cache --select F601,W292 --diff --unsafe-fixes --- example.py +++ example.py @@ -1,2 +1,2 @@ -x = {'a': 1} -print(('foo')) +x = {'a': 1, 'a': 1} +print(('foo')) \ No newline at end of file Would fix 2 errors. ``` #7790 will improve the diff messages following this pull request Similarly, `--fix` and `--fix-only` require the `--unsafe-fixes` flag to apply unsafe fixes. ## Related Replaces #5119 Closes #4185 Closes #7214 Closes #4845 Closes #3863 Addresses #6835 Addresses #7019 Needs follow-up #6962 Needs follow-up #4845 Needs follow-up #7436 Needs follow-up #7025 Needs follow-up #6434 Follow-up #7790 Follow-up #7792 --------- Co-authored-by: Evan Rittenhouse <[email protected]>
astral-sh · Oct 6, 2023 · 22e1874 · 22e1874
1 parent e8d2cbc
commit 22e1874
Show file tree

Hide file tree

Showing 37 changed files with 704 additions and 150 deletions.
diff --git a/crates/ruff_cli/src/args.rs b/crates/ruff_cli/src/args.rs
@@ -9,6 +9,7 @@ use ruff_linter::logging::LogLevel;
 use ruff_linter::registry::Rule;
 use ruff_linter::settings::types::{
     FilePattern, PatternPrefixPair, PerFileIgnore, PreviewMode, PythonVersion, SerializationFormat,
+    UnsafeFixes,
 };
 use ruff_linter::{RuleParser, RuleSelector, RuleSelectorParser};
 use ruff_workspace::configuration::{Configuration, RuleSelection};
@@ -76,12 +77,18 @@ pub enum Command {
 pub struct CheckCommand {
     /// List of files or directories to check.
     pub files: Vec<PathBuf>,
-    /// Attempt to automatically fix lint violations.
-    /// Use `--no-fix` to disable.
+    /// Apply fixes to resolve lint violations.
+    /// Use `--no-fix` to disable or `--unsafe-fixes` to include unsafe fixes.
     #[arg(long, overrides_with("no_fix"))]
     fix: bool,
     #[clap(long, overrides_with("fix"), hide = true)]
     no_fix: bool,
+    /// Include fixes that may not retain the original intent of the code.
+    /// Use `--no-unsafe-fixes` to disable.
+    #[arg(long, overrides_with("no_unsafe_fixes"))]
+    unsafe_fixes: bool,
+    #[arg(long, overrides_with("unsafe_fixes"), hide = true)]
+    no_unsafe_fixes: bool,
     /// Show violations with source code.
     /// Use `--no-show-source` to disable.
     #[arg(long, overrides_with("no_show_source"))]
@@ -100,8 +107,8 @@ pub struct CheckCommand {
     /// Run in watch mode by re-running whenever files change.
     #[arg(short, long)]
     pub watch: bool,
-    /// Fix any fixable lint violations, but don't report on leftover violations. Implies `--fix`.
-    /// Use `--no-fix-only` to disable.
+    /// Apply fixes to resolve lint violations, but don't report on leftover violations. Implies `--fix`.
+    /// Use `--no-fix-only` to disable or `--unsafe-fixes` to include unsafe fixes.
     #[arg(long, overrides_with("no_fix_only"))]
     fix_only: bool,
     #[clap(long, overrides_with("fix_only"), hide = true)]
@@ -497,6 +504,8 @@ impl CheckCommand {
                 cache_dir: self.cache_dir,
                 fix: resolve_bool_arg(self.fix, self.no_fix),
                 fix_only: resolve_bool_arg(self.fix_only, self.no_fix_only),
+                unsafe_fixes: resolve_bool_arg(self.unsafe_fixes, self.no_unsafe_fixes)
+                    .map(UnsafeFixes::from),
                 force_exclude: resolve_bool_arg(self.force_exclude, self.no_force_exclude),
                 output_format: self.output_format.or(self.format),
                 show_fixes: resolve_bool_arg(self.show_fixes, self.no_show_fixes),
@@ -599,6 +608,7 @@ pub struct CliOverrides {
     pub cache_dir: Option<PathBuf>,
     pub fix: Option<bool>,
     pub fix_only: Option<bool>,
+    pub unsafe_fixes: Option<UnsafeFixes>,
     pub force_exclude: Option<bool>,
     pub output_format: Option<SerializationFormat>,
     pub show_fixes: Option<bool>,
@@ -624,6 +634,9 @@ impl ConfigurationTransformer for CliOverrides {
         if let Some(fix_only) = &self.fix_only {
             config.fix_only = Some(*fix_only);
         }
+        if self.unsafe_fixes.is_some() {
+            config.unsafe_fixes = self.unsafe_fixes;
+        }
         config.lint.rule_selections.push(RuleSelection {
             select: self.select.clone(),
             ignore: self

diff --git a/crates/ruff_cli/src/cache.rs b/crates/ruff_cli/src/cache.rs
@@ -338,6 +338,7 @@ pub(crate) fn init(path: &Path) -> Result<()> {
 #[cfg(test)]
 mod tests {
     use filetime::{set_file_mtime, FileTime};
+    use ruff_linter::settings::types::UnsafeFixes;
     use std::env::temp_dir;
     use std::fs;
     use std::io;
@@ -410,6 +411,7 @@ mod tests {
                     Some(&cache),
                     flags::Noqa::Enabled,
                     flags::FixMode::Generate,
+                    UnsafeFixes::Enabled,
                 )
                 .unwrap();
                 if diagnostics
@@ -455,6 +457,7 @@ mod tests {
                 Some(&cache),
                 flags::Noqa::Enabled,
                 flags::FixMode::Generate,
+                UnsafeFixes::Enabled,
             )
             .unwrap();
         }
@@ -712,6 +715,7 @@ mod tests {
                 Some(cache),
                 flags::Noqa::Enabled,
                 flags::FixMode::Generate,
+                UnsafeFixes::Enabled,
             )
         }
     }

diff --git a/crates/ruff_cli/src/commands/check.rs b/crates/ruff_cli/src/commands/check.rs
@@ -11,6 +11,7 @@ use itertools::Itertools;
 use log::{debug, error, warn};
 #[cfg(not(target_family = "wasm"))]
 use rayon::prelude::*;
+use ruff_linter::settings::types::UnsafeFixes;
 use rustc_hash::FxHashMap;
 
 use ruff_diagnostics::Diagnostic;
@@ -36,6 +37,7 @@ pub(crate) fn check(
     cache: flags::Cache,
     noqa: flags::Noqa,
     fix_mode: flags::FixMode,
+    unsafe_fixes: UnsafeFixes,
 ) -> Result<Diagnostics> {
     // Collect all the Python files to check.
     let start = Instant::now();
@@ -119,7 +121,16 @@ pub(crate) fn check(
                         }
                     });
 
-                    lint_path(path, package, &settings.linter, cache, noqa, fix_mode).map_err(|e| {
+                    lint_path(
+                        path,
+                        package,
+                        &settings.linter,
+                        cache,
+                        noqa,
+                        fix_mode,
+                        unsafe_fixes,
+                    )
+                    .map_err(|e| {
                         (Some(path.to_owned()), {
                             let mut error = e.to_string();
                             for cause in e.chain() {
@@ -199,9 +210,10 @@ fn lint_path(
     cache: Option<&Cache>,
     noqa: flags::Noqa,
     fix_mode: flags::FixMode,
+    unsafe_fixes: UnsafeFixes,
 ) -> Result<Diagnostics> {
     let result = catch_unwind(|| {
-        crate::diagnostics::lint_path(path, package, settings, cache, noqa, fix_mode)
+        crate::diagnostics::lint_path(path, package, settings, cache, noqa, fix_mode, unsafe_fixes)
     });
 
     match result {
@@ -233,6 +245,8 @@ mod test {
     use std::os::unix::fs::OpenOptionsExt;
 
     use anyhow::Result;
+
+    use ruff_linter::settings::types::UnsafeFixes;
     use rustc_hash::FxHashMap;
     use tempfile::TempDir;
 
@@ -285,6 +299,7 @@ mod test {
             flags::Cache::Disabled,
             flags::Noqa::Disabled,
             flags::FixMode::Generate,
+            UnsafeFixes::Enabled,
         )
         .unwrap();
         let mut output = Vec::new();

diff --git a/crates/ruff_cli/src/diagnostics.rs b/crates/ruff_cli/src/diagnostics.rs
@@ -11,6 +11,7 @@ use anyhow::{Context, Result};
 use colored::Colorize;
 use filetime::FileTime;
 use log::{debug, error, warn};
+use ruff_linter::settings::types::UnsafeFixes;
 use rustc_hash::FxHashMap;
 
 use ruff_diagnostics::Diagnostic;
@@ -168,6 +169,7 @@ pub(crate) fn lint_path(
     cache: Option<&Cache>,
     noqa: flags::Noqa,
     fix_mode: flags::FixMode,
+    unsafe_fixes: UnsafeFixes,
 ) -> Result<Diagnostics> {
     // Check the cache.
     // TODO(charlie): `fixer::Mode::Apply` and `fixer::Mode::Diff` both have
@@ -244,8 +246,15 @@ pub(crate) fn lint_path(
             result,
             transformed,
             fixed,
-        }) = lint_fix(path, package, noqa, settings, &source_kind, source_type)
-        {
+        }) = lint_fix(
+            path,
+            package,
+            noqa,
+            unsafe_fixes,
+            settings,
+            &source_kind,
+            source_type,
+        ) {
             if !fixed.is_empty() {
                 match fix_mode {
                     flags::FixMode::Apply => transformed.write(&mut File::create(path)?)?,
@@ -355,6 +364,7 @@ pub(crate) fn lint_stdin(
             path.unwrap_or_else(|| Path::new("-")),
             package,
             noqa,
+            settings.unsafe_fixes,
             &settings.linter,
             &source_kind,
             source_type,

diff --git a/crates/ruff_cli/src/lib.rs b/crates/ruff_cli/src/lib.rs
@@ -10,7 +10,7 @@ use log::warn;
 use notify::{recommended_watcher, RecursiveMode, Watcher};
 
 use ruff_linter::logging::{set_up_logging, LogLevel};
-use ruff_linter::settings::flags;
+use ruff_linter::settings::flags::FixMode;
 use ruff_linter::settings::types::SerializationFormat;
 use ruff_linter::{fs, warn_user, warn_user_once};
 use ruff_workspace::Settings;
@@ -228,6 +228,7 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
     let Settings {
         fix,
         fix_only,
+        unsafe_fixes,
         output_format,
         show_fixes,
         show_source,
@@ -236,17 +237,20 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
 
     // Fix rules are as follows:
     // - By default, generate all fixes, but don't apply them to the filesystem.
-    // - If `--fix` or `--fix-only` is set, always apply fixes to the filesystem (or
+    // - If `--fix` or `--fix-only` is set, apply applicable fixes to the filesystem (or
     //   print them to stdout, if we're reading from stdin).
-    // - If `--diff` or `--fix-only` are set, don't print any violations (only
-    //   fixes).
+    // - If `--diff` or `--fix-only` are set, don't print any violations (only applicable fixes)
+    // - By default, applicable fixes only include [`Applicablility::Automatic`], but if
+    //   `--unsafe-fixes` is set, then [`Applicablility::Suggested`] fixes are included.
+
     let fix_mode = if cli.diff {
-        flags::FixMode::Diff
+        FixMode::Diff
     } else if fix || fix_only {
-        flags::FixMode::Apply
+        FixMode::Apply
     } else {
-        flags::FixMode::Generate
+        FixMode::Generate
     };
+
     let cache = !cli.no_cache;
     let noqa = !cli.ignore_noqa;
     let mut printer_flags = PrinterFlags::empty();
@@ -290,7 +294,13 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
         return Ok(ExitStatus::Success);
     }
 
-    let printer = Printer::new(output_format, log_level, fix_mode, printer_flags);
+    let printer = Printer::new(
+        output_format,
+        log_level,
+        fix_mode,
+        unsafe_fixes,
+        printer_flags,
+    );
 
     if cli.watch {
         if output_format != SerializationFormat::Text {
@@ -318,6 +328,7 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
             cache.into(),
             noqa.into(),
             fix_mode,
+            unsafe_fixes,
         )?;
         printer.write_continuously(&mut writer, &messages)?;
 
@@ -350,6 +361,7 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
                         cache.into(),
                         noqa.into(),
                         fix_mode,
+                        unsafe_fixes,
                     )?;
                     printer.write_continuously(&mut writer, &messages)?;
                 }
@@ -376,13 +388,14 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result<ExitStatus> {
                 cache.into(),
                 noqa.into(),
                 fix_mode,
+                unsafe_fixes,
             )?
         };
 
         // 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!(fix_mode, flags::FixMode::Apply | flags::FixMode::Diff)) {
+        if !(is_stdin && matches!(fix_mode, FixMode::Apply | FixMode::Diff)) {
             if cli.statistics {
                 printer.write_statistics(&diagnostics, &mut writer)?;
             } else {