diff --git a/crates/ruff_cli/tests/integration_test.rs b/crates/ruff_cli/tests/integration_test.rs index 27a1336c3ff63..9153f3d0d5b0d 100644 --- a/crates/ruff_cli/tests/integration_test.rs +++ b/crates/ruff_cli/tests/integration_test.rs @@ -1407,3 +1407,47 @@ extend-safe-fixes = ["UP034"] Ok(()) } + +#[test] +fn check_extend_unsafe_fixes_conflict_with_extend_safe_fixes_by_specificity() -> Result<()> { + // Adding a rule to one option with a more specific selector should override the other option + let tempdir = TempDir::new()?; + let ruff_toml = tempdir.path().join("ruff.toml"); + fs::write( + &ruff_toml, + r#" +target-version = "py310" +[lint] +extend-unsafe-fixes = ["UP", "UP034"] +extend-safe-fixes = ["UP03"] +"#, + )?; + + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(["check", "--config"]) + .arg(&ruff_toml) + .arg("-") + .args([ + "--output-format", + "text", + "--no-cache", + "--select", + "F601,UP018,UP034,UP038", + ]) + .pass_stdin("x = {'a': 1, 'a': 1}\nprint(('foo'))\nprint(str('foo'))\nisinstance(x, (int, str))\n"), + @r###" + success: false + exit_code: 1 + ----- stdout ----- + -:1:14: F601 Dictionary key literal `'a'` repeated + -:2:7: UP034 Avoid extraneous parentheses + -:3:7: UP018 Unnecessary `str` call (rewrite as a literal) + -:4:1: UP038 [*] Use `X | Y` in `isinstance` call instead of `(X, Y)` + Found 4 errors. + [*] 1 fixable with the `--fix` option (3 hidden fixes can be enabled with the `--unsafe-fixes` option). + + ----- stderr ----- + "###); + + Ok(()) +} diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index f12690345ce8f..0f09b8b03623e 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -8,7 +8,7 @@ use itertools::Itertools; use log::error; use rustc_hash::FxHashMap; -use ruff_diagnostics::{Applicability, Diagnostic}; +use ruff_diagnostics::Diagnostic; use ruff_python_ast::imports::ImportMap; use ruff_python_ast::PySourceType; use ruff_python_codegen::Stylist; @@ -268,24 +268,13 @@ pub fn check_path( } // Update fix applicability to account for overrides - if !settings.extend_safe_fixes.is_empty() || !settings.extend_unsafe_fixes.is_empty() { + if !settings.fix_safety.is_empty() { for diagnostic in &mut diagnostics { if let Some(fix) = diagnostic.fix.take() { - // Enforce demotions over promotions so if someone puts a rule in both we are conservative - if fix.applicability().is_safe() - && settings - .extend_unsafe_fixes - .contains(diagnostic.kind.rule()) - { - diagnostic.set_fix(fix.with_applicability(Applicability::Unsafe)); - } else if fix.applicability().is_unsafe() - && settings.extend_safe_fixes.contains(diagnostic.kind.rule()) - { - diagnostic.set_fix(fix.with_applicability(Applicability::Safe)); - } else { - // Retain the existing fix (will be dropped from `.take()` otherwise) - diagnostic.set_fix(fix); - } + let fixed_applicability = settings + .fix_safety + .resolve_applicability(diagnostic.kind.rule(), fix.applicability()); + diagnostic.set_fix(fix.with_applicability(fixed_applicability)); } } } diff --git a/crates/ruff_linter/src/settings/fix_safety_table.rs b/crates/ruff_linter/src/settings/fix_safety_table.rs new file mode 100644 index 0000000000000..061b3fe0568e5 --- /dev/null +++ b/crates/ruff_linter/src/settings/fix_safety_table.rs @@ -0,0 +1,188 @@ +use std::fmt::Debug; + +use ruff_diagnostics::Applicability; +use ruff_macros::CacheKey; +use rustc_hash::FxHashMap; +use strum::IntoEnumIterator; + +use crate::{ + registry::{Rule, RuleSet}, + rule_selector::{PreviewOptions, Specificity}, + RuleSelector, +}; + +/// A table to keep track of which rules fixes should have +/// their safety overridden. +#[derive(Debug, CacheKey, Default)] +pub struct FixSafetyTable { + forced_safe: RuleSet, + forced_unsafe: RuleSet, +} + +impl FixSafetyTable { + pub const fn resolve_applicability( + &self, + rule: Rule, + applicability: Applicability, + ) -> Applicability { + match applicability { + // If applicability is display-only we don't change it + Applicability::DisplayOnly => applicability, + Applicability::Safe | Applicability::Unsafe => { + if self.forced_unsafe.contains(rule) { + Applicability::Unsafe + } else if self.forced_safe.contains(rule) { + Applicability::Safe + } else { + applicability + } + } + } + } + + pub const fn is_empty(&self) -> bool { + self.forced_safe.is_empty() && self.forced_unsafe.is_empty() + } + + pub fn from_rule_selectors( + extend_safe_fixes: &[RuleSelector], + extend_unsafe_fixes: &[RuleSelector], + preview_options: &PreviewOptions, + ) -> Self { + enum Override { + Safe, + Unsafe, + } + + let safety_override_map: FxHashMap = { + Specificity::iter() + .flat_map(|spec| { + let safe_overrides = extend_safe_fixes + .iter() + .filter(|selector| selector.specificity() == spec) + .flat_map(|selector| selector.rules(preview_options)) + .map(|rule| (rule, Override::Safe)); + + let unsafe_overrides = extend_unsafe_fixes + .iter() + .filter(|selector| selector.specificity() == spec) + .flat_map(|selector| selector.rules(preview_options)) + .map(|rule| (rule, Override::Unsafe)); + + // Unsafe overrides take precedence over safe overrides + safe_overrides.chain(unsafe_overrides).collect::>() + }) + // More specified selectors take precedence over less specified selectors + .collect() + }; + + FixSafetyTable { + forced_safe: safety_override_map + .iter() + .filter_map(|(rule, o)| match o { + Override::Safe => Some(*rule), + Override::Unsafe => None, + }) + .collect(), + forced_unsafe: safety_override_map + .iter() + .filter_map(|(rule, o)| match o { + Override::Unsafe => Some(*rule), + Override::Safe => None, + }) + .collect(), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_resolve_applicability() { + let table = FixSafetyTable { + forced_safe: RuleSet::from_iter([Rule::RedefinedWhileUnused]), + forced_unsafe: RuleSet::from_iter([Rule::UnusedImport]), + }; + + for applicability in &[Applicability::Safe, Applicability::Unsafe] { + assert_eq!( + table.resolve_applicability(Rule::RedefinedWhileUnused, *applicability), + Applicability::Safe // It is forced to Safe + ); + } + for applicability in &[Applicability::Safe, Applicability::Unsafe] { + assert_eq!( + table.resolve_applicability(Rule::UnusedImport, *applicability), + Applicability::Unsafe // It is forced to Unsafe + ); + } + for applicability in &[Applicability::Safe, Applicability::Unsafe] { + assert_eq!( + table.resolve_applicability(Rule::UndefinedName, *applicability), + *applicability // Remains unchanged + ); + } + + for rule in &[ + Rule::RedefinedWhileUnused, + Rule::UnusedImport, + Rule::UndefinedName, + ] { + assert_eq!( + table.resolve_applicability(*rule, Applicability::DisplayOnly), + Applicability::DisplayOnly // Display is never changed + ); + } + } + + fn mk_table(safe_fixes: &[&str], unsafe_fixes: &[&str]) -> FixSafetyTable { + FixSafetyTable::from_rule_selectors( + &safe_fixes + .iter() + .map(|s| s.parse().unwrap()) + .collect::>(), + &unsafe_fixes + .iter() + .map(|s| s.parse().unwrap()) + .collect::>(), + &PreviewOptions::default(), + ) + } + + fn assert_rules_safety( + table: &FixSafetyTable, + assertions: &[(&str, Applicability, Applicability)], + ) { + for (code, applicability, expected) in assertions { + assert_eq!( + table.resolve_applicability(Rule::from_code(code).unwrap(), *applicability), + *expected + ); + } + } + + #[test] + fn test_from_rule_selectors_specificity() { + use Applicability::{Safe, Unsafe}; + let table = mk_table(&["UP"], &["ALL", "UP001"]); + + assert_rules_safety( + &table, + &[ + ("E101", Safe, Unsafe), + ("UP001", Safe, Unsafe), + ("UP003", Unsafe, Safe), + ], + ); + } + + #[test] + fn test_from_rule_selectors_unsafe_over_safe() { + use Applicability::{Safe, Unsafe}; + let table = mk_table(&["UP"], &["UP"]); + + assert_rules_safety(&table, &[("E101", Safe, Safe), ("UP001", Safe, Unsafe)]); + } +} diff --git a/crates/ruff_linter/src/settings/mod.rs b/crates/ruff_linter/src/settings/mod.rs index 6f36a5280c871..d8cf37eeb2f35 100644 --- a/crates/ruff_linter/src/settings/mod.rs +++ b/crates/ruff_linter/src/settings/mod.rs @@ -28,10 +28,12 @@ use crate::{codes, RuleSelector}; use super::line_width::IndentWidth; +use self::fix_safety_table::FixSafetyTable; use self::rule_table::RuleTable; use self::types::PreviewMode; use crate::rule_selector::PreviewOptions; +pub mod fix_safety_table; pub mod flags; pub mod rule_table; pub mod types; @@ -43,8 +45,7 @@ pub struct LinterSettings { pub rules: RuleTable, pub per_file_ignores: Vec<(GlobMatcher, GlobMatcher, RuleSet)>, - pub extend_unsafe_fixes: RuleSet, - pub extend_safe_fixes: RuleSet, + pub fix_safety: FixSafetyTable, pub target_version: PythonVersion, pub preview: PreviewMode, @@ -151,8 +152,7 @@ impl LinterSettings { namespace_packages: vec![], per_file_ignores: vec![], - extend_safe_fixes: RuleSet::empty(), - extend_unsafe_fixes: RuleSet::empty(), + fix_safety: FixSafetyTable::default(), src: vec![path_dedot::CWD.clone()], // Needs duplicating diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index 1adcd7c68c995..163d257b7b8f6 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -10,6 +10,7 @@ use std::path::{Path, PathBuf}; use anyhow::{anyhow, Result}; use glob::{glob, GlobError, Paths, PatternError}; use regex::Regex; +use ruff_linter::settings::fix_safety_table::FixSafetyTable; use rustc_hash::{FxHashMap, FxHashSet}; use shellexpand; use shellexpand::LookupError; @@ -239,26 +240,14 @@ impl Configuration { .collect(), )?, - extend_safe_fixes: lint - .extend_safe_fixes - .iter() - .flat_map(|selector| { - selector.rules(&PreviewOptions { - mode: lint_preview, - require_explicit: false, - }) - }) - .collect(), - extend_unsafe_fixes: lint - .extend_unsafe_fixes - .iter() - .flat_map(|selector| { - selector.rules(&PreviewOptions { - mode: lint_preview, - require_explicit: false, - }) - }) - .collect(), + fix_safety: FixSafetyTable::from_rule_selectors( + &lint.extend_safe_fixes, + &lint.extend_unsafe_fixes, + &PreviewOptions { + mode: lint_preview, + require_explicit: false, + }, + ), src: self.src.unwrap_or_else(|| vec![project_root.to_path_buf()]), explicit_preview_rules: lint.explicit_preview_rules.unwrap_or_default(),