Account for selector specificity when merging extend_unsafe_fixes and override extend_safe_fixes

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(())
+}

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

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<Rule, Override> = {
+            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::<Vec<_>>()
+                })
+                // 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::Display),
+                Applicability::Display // 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::<Vec<_>>(),
+            &unsafe_fixes
+                .iter()
+                .map(|s| s.parse().unwrap())
+                .collect::<Vec<_>>(),
+            &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)]);
+    }
+}

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

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(),