From 03303a9edd310af4df059eb2f7c72b200f3fed35 Mon Sep 17 00:00:00 2001 From: Lukasz Piatkowski Date: Tue, 7 Nov 2023 17:33:40 +0100 Subject: [PATCH] Account for selector specificity when merging `extend_unsafe_fixes` and `override extend_safe_fixes` (#8444) ## Summary Prior to this change `extend_unsafe_fixes` took precedence over `extend_safe_fixes` selectors, so any conflicts were resolved in favour of `extend_unsafe_fixes`. Thanks to that ruff were conservatively assuming that if configs conlict the fix corresponding to selected rule will be treated as unsafe. After this change we take into account Specificity of the selectors. For conflicts between selectors of the same Specificity we will treat the corresponding fixes as unsafe. But if the conflicting selectors are of different specificity the more specific one will win. ## Test Plan Tests were added for the `FixSafetyTable` struct. The `check_extend_unsafe_fixes_conflict_with_extend_safe_fixes_by_specificity` integration test was added to test conflicting rules of different specificity. Fixes #8404 --------- Co-authored-by: Zanie --- crates/ruff_cli/tests/integration_test.rs | 44 ++++ crates/ruff_linter/src/linter.rs | 23 +-- .../src/settings/fix_safety_table.rs | 188 ++++++++++++++++++ crates/ruff_linter/src/settings/mod.rs | 8 +- crates/ruff_workspace/src/configuration.rs | 29 +-- 5 files changed, 251 insertions(+), 41 deletions(-) create mode 100644 crates/ruff_linter/src/settings/fix_safety_table.rs 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(),