From d01969c974a29372f5d4269a17ea2b6ea00d8ef2 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Mon, 2 Oct 2023 17:05:01 -0500 Subject: [PATCH] Update CLI to respect fix applicability --- crates/ruff_cli/src/args.rs | 11 +- crates/ruff_cli/src/cache.rs | 6 +- crates/ruff_cli/src/commands/check.rs | 2 +- crates/ruff_cli/src/diagnostics.rs | 292 +++++++++++--------- crates/ruff_cli/src/lib.rs | 31 ++- crates/ruff_cli/src/printer.rs | 107 +++++-- crates/ruff_cli/tests/integration_test.rs | 307 ++++++++++++++++----- crates/ruff_diagnostics/src/fix.rs | 32 ++- crates/ruff_linter/src/fix/mod.rs | 22 +- crates/ruff_linter/src/linter.rs | 4 +- crates/ruff_linter/src/settings/flags.rs | 20 +- crates/ruff_linter/src/test.rs | 7 +- crates/ruff_workspace/src/configuration.rs | 4 + crates/ruff_workspace/src/options.rs | 9 + crates/ruff_workspace/src/settings.rs | 3 + docs/configuration.md | 2 + ruff.schema.json | 9 +- 17 files changed, 599 insertions(+), 269 deletions(-) diff --git a/crates/ruff_cli/src/args.rs b/crates/ruff_cli/src/args.rs index 28581fb920..831419b864 100644 --- a/crates/ruff_cli/src/args.rs +++ b/crates/ruff_cli/src/args.rs @@ -72,7 +72,6 @@ pub enum Command { // The `Parser` derive is for ruff_dev, for ruff_cli `Args` would be sufficient #[derive(Clone, Debug, clap::Parser)] -#[allow(clippy::struct_excessive_bools)] pub struct CheckCommand { /// List of files or directories to check. pub files: Vec, @@ -80,7 +79,10 @@ pub struct CheckCommand { /// Use `--no-fix` to disable. #[arg(long, overrides_with("no_fix"))] fix: bool, - #[clap(long, overrides_with("fix"), hide = true)] + /// Attempt to automatically fix both automatic and suggested lint violations. + #[arg(long, overrides_with_all(["fix", "no_fix"]))] + fix_suggested: bool, + #[clap(long, overrides_with_all(["fix", "fix_suggested"]), hide = true)] no_fix: bool, /// Show violations with source code. /// Use `--no-show-source` to disable. @@ -497,6 +499,7 @@ 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), + fix_suggested: Some(self.fix_suggested), 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 +602,7 @@ pub struct CliOverrides { pub cache_dir: Option, pub fix: Option, pub fix_only: Option, + pub fix_suggested: Option, pub force_exclude: Option, pub output_format: Option, pub show_fixes: Option, @@ -624,6 +628,9 @@ impl ConfigurationTransformer for CliOverrides { if let Some(fix_only) = &self.fix_only { config.fix_only = Some(*fix_only); } + if self.fix_suggested.is_some() { + config.fix_suggested = self.fix_suggested; + } 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 index 3a2b851451..b48593e527 100644 --- a/crates/ruff_cli/src/cache.rs +++ b/crates/ruff_cli/src/cache.rs @@ -409,7 +409,7 @@ mod tests { &settings.linter, Some(&cache), flags::Noqa::Enabled, - flags::FixMode::Generate, + flags::FixMode::Generate(flags::SuggestedFixes::Apply), ) .unwrap(); if diagnostics @@ -454,7 +454,7 @@ mod tests { &settings.linter, Some(&cache), flags::Noqa::Enabled, - flags::FixMode::Generate, + flags::FixMode::Generate(flags::SuggestedFixes::Apply), ) .unwrap(); } @@ -711,7 +711,7 @@ mod tests { &self.settings.linter, Some(cache), flags::Noqa::Enabled, - flags::FixMode::Generate, + flags::FixMode::Generate(flags::SuggestedFixes::Apply), ) } } diff --git a/crates/ruff_cli/src/commands/check.rs b/crates/ruff_cli/src/commands/check.rs index f19c8d80d4..28012af5ff 100644 --- a/crates/ruff_cli/src/commands/check.rs +++ b/crates/ruff_cli/src/commands/check.rs @@ -284,7 +284,7 @@ mod test { &CliOverrides::default(), flags::Cache::Disabled, flags::Noqa::Disabled, - flags::FixMode::Generate, + flags::FixMode::Generate(flags::SuggestedFixes::Apply), ) .unwrap(); let mut output = Vec::new(); diff --git a/crates/ruff_cli/src/diagnostics.rs b/crates/ruff_cli/src/diagnostics.rs index d3dea9f56a..b0c336b7c6 100644 --- a/crates/ruff_cli/src/diagnostics.rs +++ b/crates/ruff_cli/src/diagnostics.rs @@ -241,96 +241,110 @@ pub(crate) fn lint_path( error: parse_error, }, fixed, - ) = if matches!(fix_mode, flags::FixMode::Apply | flags::FixMode::Diff) { - if let Ok(FixerResult { - result, - transformed, - fixed, - }) = lint_fix(path, package, noqa, settings, &source_kind, source_type) - { - if !fixed.is_empty() { - match fix_mode { - flags::FixMode::Apply => match transformed.as_ref() { - SourceKind::Python(transformed) => { - write(path, transformed.as_bytes())?; - } - SourceKind::IpyNotebook(notebook) => { - let mut writer = BufWriter::new(File::create(path)?); - notebook.write(&mut writer)?; - } - }, - flags::FixMode::Diff => { - match transformed.as_ref() { + ) = match fix_mode { + flags::FixMode::Apply(fix_suggested) | flags::FixMode::Diff(fix_suggested) => { + if let Ok(FixerResult { + result, + transformed, + fixed, + }) = lint_fix( + path, + package, + noqa, + settings, + &source_kind, + source_type, + fix_suggested, + ) { + if !fixed.is_empty() { + match fix_mode { + flags::FixMode::Apply(_) => match transformed.as_ref() { SourceKind::Python(transformed) => { - let mut stdout = io::stdout().lock(); - TextDiff::from_lines(source_kind.source_code(), transformed) - .unified_diff() - .header(&fs::relativize_path(path), &fs::relativize_path(path)) - .to_writer(&mut stdout)?; - stdout.write_all(b"\n")?; - stdout.flush()?; + write(path, transformed.as_bytes())?; } - SourceKind::IpyNotebook(dest_notebook) => { - // We need to load the notebook again, since we might've - // mutated it. - let src_notebook = source_kind.as_ipy_notebook().unwrap(); - let mut stdout = io::stdout().lock(); - // Cell indices are 1-based. - for ((idx, src_cell), dest_cell) in (1u32..) - .zip(src_notebook.cells().iter()) - .zip(dest_notebook.cells().iter()) - { - let (Cell::Code(src_code_cell), Cell::Code(dest_code_cell)) = - (src_cell, dest_cell) - else { - continue; - }; - TextDiff::from_lines( - &src_code_cell.source.to_string(), - &dest_code_cell.source.to_string(), - ) - .unified_diff() - // Jupyter notebook cells don't necessarily have a newline - // at the end. For example, - // - // ```python - // print("hello") - // ``` - // - // For a cell containing the above code, there'll only be one line, - // and it won't have a newline at the end. If it did, there'd be - // two lines, and the second line would be empty: - // - // ```python - // print("hello") - // - // ``` - .missing_newline_hint(false) - .header( - &format!("{}:cell {}", &fs::relativize_path(path), idx), - &format!("{}:cell {}", &fs::relativize_path(path), idx), - ) - .to_writer(&mut stdout)?; + SourceKind::IpyNotebook(notebook) => { + let mut writer = BufWriter::new(File::create(path)?); + notebook.write(&mut writer)?; + } + }, + flags::FixMode::Diff(_) => { + match transformed.as_ref() { + SourceKind::Python(transformed) => { + let mut stdout = io::stdout().lock(); + TextDiff::from_lines(source_kind.source_code(), transformed) + .unified_diff() + .header( + &fs::relativize_path(path), + &fs::relativize_path(path), + ) + .to_writer(&mut stdout)?; + stdout.write_all(b"\n")?; + stdout.flush()?; + } + SourceKind::IpyNotebook(dest_notebook) => { + // We need to load the notebook again, since we might've + // mutated it. + let src_notebook = source_kind.as_ipy_notebook().unwrap(); + let mut stdout = io::stdout().lock(); + for ((idx, src_cell), dest_cell) in src_notebook + .cells() + .iter() + .enumerate() + .zip(dest_notebook.cells().iter()) + { + let (Cell::Code(src_code_cell), Cell::Code(dest_code_cell)) = + (src_cell, dest_cell) + else { + continue; + }; + TextDiff::from_lines( + &src_code_cell.source.to_string(), + &dest_code_cell.source.to_string(), + ) + .unified_diff() + // Jupyter notebook cells don't necessarily have a newline + // at the end. For example, + // + // ```python + // print("hello") + // ``` + // + // For a cell containing the above code, there'll only be one line, + // and it won't have a newline at the end. If it did, there'd be + // two lines, and the second line would be empty: + // + // ```python + // print("hello") + // + // ``` + .missing_newline_hint(false) + .header( + &format!("{}:cell {}", &fs::relativize_path(path), idx), + &format!("{}:cell {}", &fs::relativize_path(path), idx), + ) + .to_writer(&mut stdout)?; + } + stdout.write_all(b"\n")?; + stdout.flush()?; } - stdout.write_all(b"\n")?; - stdout.flush()?; } } + flags::FixMode::Generate(_) => {} } - flags::FixMode::Generate => {} } + (result, fixed) + } else { + // If we fail to fix, lint the original source code. + let result = lint_only(path, package, settings, noqa, &source_kind, source_type); + let fixed = FxHashMap::default(); + (result, fixed) } - (result, fixed) - } else { - // If we fail to fix, lint the original source code. + } + flags::FixMode::Generate(_) => { let result = lint_only(path, package, settings, noqa, &source_kind, source_type); let fixed = FxHashMap::default(); (result, fixed) } - } else { - let result = lint_only(path, package, settings, noqa, &source_kind, source_type); - let fixed = FxHashMap::default(); - (result, fixed) }; let imports = imports.unwrap_or_default(); @@ -392,7 +406,7 @@ pub(crate) fn lint_stdin( }; // Extract the sources from the file. - let source_kind = match SourceKind::from_source_code(contents, source_type) { + let source_kind = match SourceKind::from_source_code(contents.clone(), source_type) { Ok(Some(source_kind)) => source_kind, Ok(None) => return Ok(Diagnostics::default()), Err(err) => { @@ -407,49 +421,70 @@ pub(crate) fn lint_stdin( error: parse_error, }, fixed, - ) = if matches!(fix_mode, flags::FixMode::Apply | flags::FixMode::Diff) { - if let Ok(FixerResult { - result, - transformed, - fixed, - }) = lint_fix( - path.unwrap_or_else(|| Path::new("-")), - package, - noqa, - &settings.linter, - &source_kind, - source_type, - ) { - match fix_mode { - flags::FixMode::Apply => { - // Write the contents to stdout, regardless of whether any errors were fixed. - transformed.write(&mut io::stdout().lock())?; - } - flags::FixMode::Diff => { - // But only write a diff if it's non-empty. - if !fixed.is_empty() { - let text_diff = TextDiff::from_lines( - source_kind.source_code(), - transformed.source_code(), - ); - 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()?; + ) = match fix_mode { + flags::FixMode::Apply(fix_suggested) | flags::FixMode::Diff(fix_suggested) => { + if let Ok(FixerResult { + result, + transformed, + fixed, + }) = lint_fix( + path.unwrap_or_else(|| Path::new("-")), + package, + noqa, + &settings.linter, + &source_kind, + source_type, + fix_suggested, + ) { + match fix_mode { + flags::FixMode::Apply(_) => { + // Write the contents to stdout, regardless of whether any errors were fixed. + io::stdout().write_all(transformed.source_code().as_bytes())?; } - } - flags::FixMode::Generate => {} - } + flags::FixMode::Diff(_) => { + // But only write a diff if it's non-empty. + if !fixed.is_empty() { + let text_diff = TextDiff::from_lines( + source_kind.source_code(), + transformed.source_code(), + ); + let mut unified_diff = text_diff.unified_diff(); + if let Some(path) = path { + unified_diff + .header(&fs::relativize_path(path), &fs::relativize_path(path)); + } - (result, fixed) - } else { - // If we fail to fix, lint the original source code. + let mut stdout = io::stdout().lock(); + unified_diff.to_writer(&mut stdout)?; + stdout.write_all(b"\n")?; + stdout.flush()?; + } + } + flags::FixMode::Generate(_) => {} + } + + (result, fixed) + } else { + // If we fail to fix, lint the original source code. + let result = lint_only( + path.unwrap_or_else(|| Path::new("-")), + package, + &settings.linter, + noqa, + &source_kind, + source_type, + ); + let fixed = FxHashMap::default(); + + // Write the contents to stdout anyway. + if fix_mode.is_apply() { + io::stdout().write_all(contents.as_bytes())?; + } + + (result, fixed) + } + } + flags::FixMode::Generate(_) => { let result = lint_only( path.unwrap_or_else(|| Path::new("-")), package, @@ -459,25 +494,8 @@ pub(crate) fn lint_stdin( source_type, ); let fixed = FxHashMap::default(); - - // Write the contents to stdout anyway. - if fix_mode.is_apply() { - source_kind.write(&mut io::stdout().lock())?; - } - (result, fixed) } - } else { - let result = lint_only( - path.unwrap_or_else(|| Path::new("-")), - package, - &settings.linter, - noqa, - &source_kind, - source_type, - ); - 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 f9dbb164df..2397aa8ee8 100644 --- 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, SuggestedFixes}; 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 { let Settings { fix, fix_only, + fix_suggested, output_format, show_fixes, show_source, @@ -237,16 +238,28 @@ pub fn check(args: CheckCommand, log_level: LogLevel) -> Result { // 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 - // print them to stdout, if we're reading from stdin). + // print them to stdout, if we're reading from stdin) for [`Applicability::Automatic`] rules + // only. // - If `--diff` or `--fix-only` are set, don't print any violations (only - // fixes). - let fix_mode = if cli.diff { - flags::FixMode::Diff - } else if fix || fix_only { - flags::FixMode::Apply + // fixes) for [`Applicability::Automatic`] rules only. + // - If `--fix--suggested` is set, the above rules will apply to both [`Applicability::Suggested`] and + // [`Applicability::Automatic`] fixes. + // TODO: can't fix this until @zanieb changes the CLI + let fix_suggested = if fix_suggested { + SuggestedFixes::Apply } else { - flags::FixMode::Generate + SuggestedFixes::Disable }; + + let fix_mode = if cli.diff { + FixMode::Diff(fix_suggested) + } else if fix || fix_only { + FixMode::Apply(fix_suggested) + } else { + // We'll always generate all fixes, regardless of [`Applicability`], in `generate` mode + FixMode::Generate(SuggestedFixes::Apply) + }; + let cache = !cli.no_cache; let noqa = !cli.ignore_noqa; let mut printer_flags = PrinterFlags::empty(); @@ -382,7 +395,7 @@ pub fn check(args: CheckCommand, 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!(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 { diff --git a/crates/ruff_cli/src/printer.rs b/crates/ruff_cli/src/printer.rs index 7700df9749..a56d1b2bcf 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; @@ -19,7 +20,7 @@ use ruff_linter::message::{ }; use ruff_linter::notify_user; use ruff_linter::registry::{AsRule, Rule}; -use ruff_linter::settings::flags; +use ruff_linter::settings::flags::{self, SuggestedFixes}; use ruff_linter::settings::types::SerializationFormat; use crate::diagnostics::Diagnostics; @@ -118,19 +119,10 @@ impl Printer { writeln!(writer, "Found {remaining} error{s}.")?; } - if show_fix_status(self.fix_mode) { - 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(), - )?; - } + let fixables = FixableStatistics::new(diagnostics, self.fix_mode.suggested_fixes()); + + if !fixables.is_empty() { + writeln!(writer, "{}", fixables.violation_string())?; } } else { let fixed = diagnostics @@ -178,6 +170,7 @@ impl Printer { } let context = EmitterContext::new(&diagnostics.notebook_indexes); + let fixables = FixableStatistics::new(diagnostics, self.fix_mode.suggested_fixes()); match self.format { SerializationFormat::Json => { @@ -191,9 +184,9 @@ impl Printer { } SerializationFormat::Text => { TextEmitter::default() - .with_show_fix_status(show_fix_status(self.fix_mode)) - .with_show_fix_diff(self.flags.intersects(Flags::SHOW_FIX_DIFF)) - .with_show_source(self.flags.intersects(Flags::SHOW_SOURCE)) + .with_show_fix_status(show_fix_status(self.fix_mode, fixables)) + .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) { @@ -209,7 +202,7 @@ impl Printer { SerializationFormat::Grouped => { GroupedEmitter::default() .with_show_source(self.flags.intersects(Flags::SHOW_SOURCE)) - .with_show_fix_status(show_fix_status(self.fix_mode)) + .with_show_fix_status(show_fix_status(self.fix_mode, fixables)) .emit(writer, &diagnostics.messages, &context)?; if self.flags.intersects(Flags::SHOW_FIX_SUMMARY) { @@ -359,6 +352,8 @@ impl Printer { ); } + let fixables = FixableStatistics::new(diagnostics, self.fix_mode.suggested_fixes()); + if !diagnostics.messages.is_empty() { if self.log_level >= LogLevel::Default { writeln!(writer)?; @@ -366,7 +361,7 @@ impl Printer { let context = EmitterContext::new(&diagnostics.notebook_indexes); TextEmitter::default() - .with_show_fix_status(show_fix_status(self.fix_mode)) + .with_show_fix_status(show_fix_status(self.fix_mode, fixables)) .with_show_source(self.flags.intersects(Flags::SHOW_SOURCE)) .emit(writer, &diagnostics.messages, &context)?; } @@ -390,13 +385,13 @@ fn num_digits(n: usize) -> usize { } /// Return `true` if the [`Printer`] should indicate that a rule is fixable. -const fn show_fix_status(fix_mode: flags::FixMode) -> bool { +fn show_fix_status(fix_mode: flags::FixMode, fixables: FixableStatistics) -> 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 fix is not // enabled, we may inadvertently indicate that a rule is fixable.) - !fix_mode.is_apply() + (!fix_mode.is_apply()) && fixables.fixes_are_applicable() } fn print_fix_summary(writer: &mut dyn Write, fixed: &FxHashMap) -> Result<()> { @@ -439,3 +434,73 @@ fn print_fix_summary(writer: &mut dyn Write, fixed: &FxHashMap } Ok(()) } + +/// Contains the number of [`Applicability::Automatic`] and [`Applicability::Suggested`] fixes +struct FixableStatistics<'a> { + automatic: u32, + suggested: u32, + apply_suggested: &'a SuggestedFixes, +} + +impl<'a> FixableStatistics<'a> { + fn new(diagnostics: &Diagnostics, apply_suggested: &'a SuggestedFixes) -> Self { + let mut automatic = 0; + let mut suggested = 0; + + for message in &diagnostics.messages { + if let Some(fix) = &message.fix { + if fix.applicability() == Applicability::Suggested { + suggested += 1; + } else if fix.applicability() == Applicability::Automatic { + automatic += 1; + } + } + } + + Self { + automatic, + suggested, + apply_suggested, + } + } + + fn fixes_are_applicable(&self) -> bool { + match self.apply_suggested { + SuggestedFixes::Apply => self.automatic > 0 || self.suggested > 0, + SuggestedFixes::Disable => self.automatic > 0, + } + } + + /// Returns [`true`] if there aren't any fixes to be displayed + fn is_empty(&self) -> bool { + self.automatic == 0 && self.suggested == 0 + } + + /// Build the displayed fix status message depending on the types of the remaining fixes. + fn violation_string(&self) -> String { + let prefix = format!("[{}]", "*".cyan()); + let mut fix_status = prefix; + + if self.automatic > 0 { + fix_status = format!( + "{fix_status} {} potentially fixable with the --fix option.", + self.automatic + ); + } + + if self.suggested > 0 { + let (line_break, extra_prefix) = if self.automatic > 0 { + ("\n", format!("[{}]", "*".cyan())) + } else { + ("", String::new()) + }; + + let total = self.automatic + self.suggested; + fix_status = format!( + "{fix_status}{line_break}{extra_prefix} {total} potentially fixable with the --fix-suggested option." + ); + } + + fix_status + } +} diff --git a/crates/ruff_cli/tests/integration_test.rs b/crates/ruff_cli/tests/integration_test.rs index a47a974d81..dbf8e82939 100644 --- a/crates/ruff_cli/tests/integration_test.rs +++ b/crates/ruff_cli/tests/integration_test.rs @@ -241,81 +241,9 @@ fn stdin_fix_jupyter() { success: true exit_code: 0 ----- stdout ----- - { - "cells": [ - { - "cell_type": "code", - "execution_count": 1, - "id": "dccc687c-96e2-4604-b957-a8a89b5bec06", - "metadata": {}, - "outputs": [], - "source": [] - }, - { - "cell_type": "markdown", - "id": "19e1b029-f516-4662-a9b9-623b93edac1a", - "metadata": {}, - "source": [ - "Foo" - ] - }, - { - "cell_type": "code", - "execution_count": 2, - "id": "cdce7b92-b0fb-4c02-86f6-e233b26fa84f", - "metadata": {}, - "outputs": [], - "source": [] - }, - { - "cell_type": "code", - "execution_count": 3, - "id": "e40b33d2-7fe4-46c5-bdf0-8802f3052565", - "metadata": {}, - "outputs": [ - { - "name": "stdout", - "output_type": "stream", - "text": [ - "1\n" - ] - } - ], - "source": [ - "print(1)" - ] - }, - { - "cell_type": "code", - "execution_count": null, - "id": "a1899bc8-d46f-4ec0-b1d1-e1ca0f04bf60", - "metadata": {}, - "outputs": [], - "source": [] - } - ], - "metadata": { - "kernelspec": { - "display_name": "Python 3 (ipykernel)", - "language": "python", - "name": "python3" - }, - "language_info": { - "codemirror_mode": { - "name": "ipython", - "version": 3 - }, - "file_extension": ".py", - "mimetype": "text/x-python", - "name": "python", - "nbconvert_exporter": "python", - "pygments_lexer": "ipython3", - "version": "3.11.2" - } - }, - "nbformat": 4, - "nbformat_minor": 5 - } + print(1) + + ----- stderr ----- "###); } @@ -869,3 +797,232 @@ fn check_input_from_argfile() -> Result<()> { Ok(()) } + +#[test] +fn displays_fix_applicability_levels() -> Result<()> { + // `--fix` should only apply automatic fixes, but should tell the user about `--fix --unautomatic` if + // there are remaining unautomatic fixes. + // TODO: this should be a failure but we don't have a way to track that + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args([ + "-", + "--output-format=text", + "--isolated", + "--select", + "F601,UP034", + "--no-cache", + ]) + .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + @r###" + success: false + exit_code: 1 + ----- stdout ----- + -: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-suggested option. + + ----- stderr ----- + "###); + + Ok(()) +} + +#[test] +fn displays_remaining_suggested_fixes() -> Result<()> { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(["-", "--output-format", "text", "--no-cache", "--isolated", "--select", "F601"]) + .pass_stdin("x = {'a': 1, 'a': 1}\n"), + @r###" + success: false + exit_code: 1 + ----- stdout ----- + -:1:14: F601 [*] Dictionary key literal `'a'` repeated + Found 1 error. + [*] 1 potentially fixable with the --fix-suggested option. + + ----- stderr ----- + "###); + + Ok(()) +} + +#[test] +fn fix_applies_automatic_fixes_only() -> Result<()> { + // `--fix` should only apply automatic fixes. Since we're runnnig in `stdin` mode, output shouldn't + // be printed. + assert_cmd_snapshot!( + Command::new(get_cargo_bin(BIN_NAME)) + .args([ + "-", + "--output-format", + "text", + "--isolated","--no-cache", + "--select", + "F601,UP034", + "--fix", + ]) + .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + @r###" + success: false + exit_code: 1 + ----- stdout ----- + x = {'a': 1, 'a': 1} + print('foo') + + ----- stderr ----- + "###); + + Ok(()) +} + +#[test] +fn fix_applies_automatic_and_suggested_fixes_with_fix_suggested() -> Result<()> { + assert_cmd_snapshot!( + Command::new(get_cargo_bin(BIN_NAME)) + .args([ + "-", + "--output-format", + "text", + "--isolated","--no-cache", + "--select", + "F601,UP034", + "--fix", + "--fix-suggested", + ]) + .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + @r###" + success: false + exit_code: 1 + ----- stdout ----- + -: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-suggested option. + + ----- stderr ----- + "###); + + Ok(()) +} + +#[test] +fn diff_shows_automatic_and_suggested_fixes_with_fix_suggested() -> Result<()> { + assert_cmd_snapshot!( + Command::new(get_cargo_bin(BIN_NAME)) + .args([ + "-", + "--output-format", + "text", + "--isolated","--no-cache", + "--select", + "F601,UP034", + "--diff", + "--fix-suggested", + ]) + .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + @r###" + success: false + exit_code: 1 + ----- stdout ----- + @@ -1,2 +1,2 @@ + -x = {'a': 1, 'a': 1} + -print(('foo')) + +x = {'a': 1} + +print('foo') + + + ----- stderr ----- + "### + ); + + Ok(()) +} + +#[test] +fn diff_shows_automatic_fixes_only() -> Result<()> { + assert_cmd_snapshot!( + Command::new(get_cargo_bin(BIN_NAME)) + .args([ + "-", + "--output-format", + "text", + "--isolated","--no-cache", + "--select", + "F601,UP034", + "--diff", + ]) + .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + @r###" + success: false + exit_code: 1 + ----- stdout ----- + @@ -1,2 +1,2 @@ + x = {'a': 1, 'a': 1} + -print(('foo')) + +print('foo') + + + ----- stderr ----- + "### + ); + + Ok(()) +} + +#[test] +fn fix_only_applies_automatic_and_suggested_fixes_with_fix_suggested() -> Result<()> { + assert_cmd_snapshot!( + Command::new(get_cargo_bin(BIN_NAME)) + .args([ + "-", + "--output-format", + "text", + "--isolated","--no-cache", + "--select", + "F601,UP034", + "--fix-only", + "--fix-suggested", + ]) + .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + @r###" + success: true + exit_code: 0 + ----- stdout ----- + x = {'a': 1} + print('foo') + + ----- stderr ----- + "###); + + Ok(()) +} + +#[test] +fn fix_only_applies_automatic_fixes_only() -> Result<()> { + assert_cmd_snapshot!( + Command::new(get_cargo_bin(BIN_NAME)) + .args([ + "-", + "--output-format", + "text", + "--isolated","--no-cache", + "--select", + "F601,UP034", + "--fix-only", + ]) + .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\n"), + @r###" + success: true + exit_code: 0 + ----- stdout ----- + x = {'a': 1, 'a': 1} + print('foo') + + ----- stderr ----- + "###); + + Ok(()) +} diff --git a/crates/ruff_diagnostics/src/fix.rs b/crates/ruff_diagnostics/src/fix.rs index 7456e82b7e..8db635392f 100644 --- a/crates/ruff_diagnostics/src/fix.rs +++ b/crates/ruff_diagnostics/src/fix.rs @@ -5,27 +5,30 @@ use ruff_text_size::{Ranged, TextSize}; use crate::edit::Edit; -/// Indicates confidence in the correctness of a suggested fix. +/// Indicates confidence in the correctness of a suggested fix. Rust internally allows comparison +/// of enum values based on their order (see Rust's [enum +/// documentation](https://doc.rust-lang.org/reference/items/enumerations.html)). This allows us to +/// apply [`Fix`]es based on their [`Applicability`]. #[derive(Default, Copy, Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] 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, + /// 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, + + /// The applicability of the fix is unknown or not relevant. + #[default] + Unspecified, /// 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, - /// 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, - - /// The applicability of the fix is unknown. - #[default] - Unspecified, + /// The fix is definitely what the user intended, or maintains the exact meaning of the code. + /// This fix should be automatically applied. + Automatic, } /// Indicates the level of isolation required to apply a fix. @@ -162,4 +165,9 @@ impl Fix { self.isolation_level = isolation; self } + + /// Return [`true`] if this [`Fix`] should be applied with a given [`Applicability`]. + pub fn is_applied(&self, applicability: Applicability) -> bool { + self.applicability >= applicability + } } diff --git a/crates/ruff_linter/src/fix/mod.rs b/crates/ruff_linter/src/fix/mod.rs index 563fac9e83..aa626d8992 100644 --- a/crates/ruff_linter/src/fix/mod.rs +++ b/crates/ruff_linter/src/fix/mod.rs @@ -4,11 +4,12 @@ use std::collections::BTreeSet; use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; use rustc_hash::{FxHashMap, FxHashSet}; -use ruff_diagnostics::{Diagnostic, Edit, Fix, IsolationLevel, SourceMap}; +use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, IsolationLevel, SourceMap}; use ruff_source_file::Locator; use crate::linter::FixTable; use crate::registry::{AsRule, Rule}; +use crate::settings::flags::SuggestedFixes; pub(crate) mod codemods; pub(crate) mod edits; @@ -24,10 +25,25 @@ 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, + fix_suggested: SuggestedFixes, +) -> Option { let mut with_fixes = diagnostics .iter() - .filter(|diag| diag.fix.is_some()) + .filter(|diag| { + let Some(ref fix) = diag.fix else { + return false; + }; + + // change this + if matches!(fix_suggested, SuggestedFixes::Apply) { + fix.applicability() >= Applicability::Suggested + } else { + fix.applicability() == Applicability::Automatic + } + }) .peekable(); if with_fixes.peek().is_none() { diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index 69b466cbb3..66ad1fb0bd 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -415,6 +415,7 @@ fn diagnostics_to_messages( /// Generate `Diagnostic`s from source code content, iteratively fixing /// until stable. +#[allow(clippy::too_many_arguments)] pub fn lint_fix<'a>( path: &Path, package: Option<&Path>, @@ -422,6 +423,7 @@ pub fn lint_fix<'a>( settings: &LinterSettings, source_kind: &'a SourceKind, source_type: PySourceType, + fix_suggested: flags::SuggestedFixes, ) -> Result> { let mut transformed = Cow::Borrowed(source_kind); @@ -494,7 +496,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, fix_suggested) { if iterations < MAX_ITERATIONS { // Count the number of fixed errors. diff --git a/crates/ruff_linter/src/settings/flags.rs b/crates/ruff_linter/src/settings/flags.rs index a1ce403194..244db27d9d 100644 --- a/crates/ruff_linter/src/settings/flags.rs +++ b/crates/ruff_linter/src/settings/flags.rs @@ -1,8 +1,24 @@ #[derive(Debug, Copy, Clone, Hash, is_macro::Is)] pub enum FixMode { - Generate, + Generate(SuggestedFixes), + Apply(SuggestedFixes), + Diff(SuggestedFixes), +} + +impl FixMode { + pub fn suggested_fixes(&self) -> &SuggestedFixes { + match self { + FixMode::Generate(suggested) => suggested, + FixMode::Apply(suggested) => suggested, + FixMode::Diff(suggested) => suggested, + } + } +} + +#[derive(Debug, Copy, Clone, Hash, is_macro::Is)] +pub enum SuggestedFixes { Apply, - Diff, + Disable, } #[derive(Debug, Copy, Clone, Hash, result_like::BoolLike)] diff --git a/crates/ruff_linter/src/test.rs b/crates/ruff_linter/src/test.rs index 75de40cd2e..208c0ec018 100644 --- a/crates/ruff_linter/src/test.rs +++ b/crates/ruff_linter/src/test.rs @@ -155,8 +155,11 @@ pub(crate) fn test_contents<'a>( code: fixed_contents, source_map, .. - }) = fix_file(&diagnostics, &Locator::new(transformed.source_code())) - { + }) = fix_file( + &diagnostics, + &Locator::new(transformed.source_code()), + flags::SuggestedFixes::Apply, + ) { if iterations < max_iterations() { iterations += 1; } else { diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index ad350a3346..ac3971c00c 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -63,6 +63,7 @@ pub struct Configuration { pub cache_dir: Option, pub extend: Option, pub fix: Option, + pub fix_suggested: Option, pub fix_only: Option, pub output_format: Option, pub preview: Option, @@ -136,6 +137,7 @@ impl Configuration { .clone() .unwrap_or_else(|| cache_dir(project_root)), fix: self.fix.unwrap_or(false), + fix_suggested: self.fix_suggested.unwrap_or(false), fix_only: self.fix_only.unwrap_or(false), output_format: self.output_format.unwrap_or_default(), show_fixes: self.show_fixes.unwrap_or(false), @@ -365,6 +367,7 @@ impl Configuration { }), fix: options.fix, fix_only: options.fix_only, + fix_suggested: options.fix_suggested, output_format: options.output_format.or_else(|| { options .format @@ -418,6 +421,7 @@ impl Configuration { include: self.include.or(config.include), fix: self.fix.or(config.fix), fix_only: self.fix_only.or(config.fix_only), + fix_suggested: self.fix_suggested.or(config.fix_suggested), output_format: self.output_format.or(config.output_format), force_exclude: self.force_exclude.or(config.force_exclude), line_length: self.line_length.or(config.line_length), diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index a376aea45f..23ff6b466c 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -89,9 +89,18 @@ pub struct Options { /// Enable fix behavior by-default when running `ruff` (overridden /// by the `--fix` and `--no-fix` command-line flags). + /// Only includes automatic fixes unless `--fix-suggested` is provided. #[option(default = "false", value_type = "bool", example = "fix = true")] pub fix: Option, + /// Enable application of suggested fixes. + #[option( + default = "false", + value_type = "bool", + example = "fix-suggested = true" + )] + pub fix_suggested: Option, + /// Like `fix`, but disables reporting on leftover violation. Implies `fix`. #[option(default = "false", value_type = "bool", example = "fix-only = true")] pub fix_only: Option, diff --git a/crates/ruff_workspace/src/settings.rs b/crates/ruff_workspace/src/settings.rs index 0e069a58c0..80a7538b00 100644 --- a/crates/ruff_workspace/src/settings.rs +++ b/crates/ruff_workspace/src/settings.rs @@ -17,6 +17,8 @@ pub struct Settings { #[cache_key(ignore)] pub fix: bool, #[cache_key(ignore)] + pub fix_suggested: bool, + #[cache_key(ignore)] pub fix_only: bool, #[cache_key(ignore)] pub output_format: SerializationFormat, @@ -36,6 +38,7 @@ impl Default for Settings { Self { cache_dir: cache_dir(project_root), fix: false, + fix_suggested: false, fix_only: false, output_format: SerializationFormat::default(), show_fixes: false, diff --git a/docs/configuration.md b/docs/configuration.md index e97374b620..dbe568dcc2 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -194,6 +194,8 @@ Arguments: Options: --fix Attempt to automatically fix lint violations. Use `--no-fix` to disable + --fix-suggested + Attempt to automatically fix both automatic and suggested lint violations --show-source Show violations with source code. Use `--no-show-source` to disable --show-fixes diff --git a/ruff.schema.json b/ruff.schema.json index 20199a2bd9..c657da23af 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -128,7 +128,7 @@ } }, "fix": { - "description": "Enable fix behavior by-default when running `ruff` (overridden by the `--fix` and `--no-fix` command-line flags).", + "description": "Enable fix behavior by-default when running `ruff` (overridden by the `--fix` and `--no-fix` command-line flags). Only includes automatic fixes unless `--fix-suggested` is provided.", "type": [ "boolean", "null" @@ -141,6 +141,13 @@ "null" ] }, + "fix-suggested": { + "description": "Enable application of suggested fixes.", + "type": [ + "boolean", + "null" + ] + }, "fixable": { "description": "A list of rule codes or prefixes to consider fixable. By default, all rules are considered fixable.", "type": [