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

@@ -32,6 +32,7 @@ use crate::message::Message;
 use crate::noqa::add_noqa;
 use crate::registry::{AsRule, Rule};
 use crate::rules::pycodestyle;
+use crate::settings::fix_safety_table::FixSafety;
 use crate::settings::types::UnsafeFixes;
 use crate::settings::{flags, LinterSettings};
 use crate::source_kind::SourceKind;
@@ -268,23 +269,30 @@ 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_table.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);
+                match fix.applicability() {
+                    Applicability::Display => {
+                        // If the fix is display-only, we don't need to do anything
+                        // Retain the existing fix (will be dropped from `.take()` otherwise)
+                        diagnostic.set_fix(fix);
+                    }
+                    Applicability::Safe | Applicability::Unsafe => match settings
+                        .fix_safety_table
+                        .resolve_rule(diagnostic.kind.rule())
+                    {
+                        FixSafety::ForcedSafe => {
+                            diagnostic.set_fix(fix.with_applicability(Applicability::Safe));
+                        }
+                        FixSafety::ForcedUnsafe => {
+                            diagnostic.set_fix(fix.with_applicability(Applicability::Unsafe));
+                        }
+                        FixSafety::Default => {
+                            // Retain the existing fix (will be dropped from `.take()` otherwise)
+                            diagnostic.set_fix(fix);
+                        }
+                    },
                 }
             }
         }

crates/ruff_linter/src/settings/fix_safety_table.rs

@@ -0,0 +1,166 @@
+use std::fmt::Debug;
+
+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,
+}
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum FixSafety {
+    ForcedSafe,
+    ForcedUnsafe,
+    Default,
+}
+
+impl FixSafetyTable {
+    pub const fn resolve_rule(&self, rule: Rule) -> FixSafety {
+        if self.forced_safe.contains(rule) {
+            FixSafety::ForcedSafe
+        } else if self.forced_unsafe.contains(rule) {
+            FixSafety::ForcedUnsafe
+        } else {
+            FixSafety::Default
+        }
+    }
+
+    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,
+        }
+        use 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, Safe));
+
+                    let unsafe_overrides = extend_unsafe_fixes
+                        .iter()
+                        .filter(|selector| selector.specificity() == spec)
+                        .flat_map(|selector| selector.rules(preview_options))
+                        .map(|rule| (rule, 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 {
+                    Safe => Some(*rule),
+                    Unsafe => None,
+                })
+                .collect(),
+            forced_unsafe: safety_override_map
+                .iter()
+                .filter_map(|(rule, o)| match o {
+                    Unsafe => Some(*rule),
+                    Safe => None,
+                })
+                .collect(),
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_resolve_rule() {
+        let table = FixSafetyTable {
+            forced_safe: RuleSet::from_iter([Rule::RedefinedWhileUnused]),
+            forced_unsafe: RuleSet::from_iter([Rule::UnusedImport]),
+        };
+
+        assert_eq!(
+            table.resolve_rule(Rule::RedefinedWhileUnused),
+            FixSafety::ForcedSafe
+        );
+        assert_eq!(
+            table.resolve_rule(Rule::UnusedImport),
+            FixSafety::ForcedUnsafe
+        );
+        assert_eq!(table.resolve_rule(Rule::UndefinedName), FixSafety::Default);
+    }
+
+    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, FixSafety)]) {
+        for (code, expected) in assertions {
+            assert_eq!(
+                table.resolve_rule(Rule::from_code(code).unwrap()),
+                *expected
+            );
+        }
+    }
+
+    #[test]
+    fn test_from_rule_selectors_specificity() {
+        let table = mk_table(&["UP"], &["ALL", "UP001"]);
+
+        assert_rules_safety(
+            &table,
+            &[
+                ("E101", FixSafety::ForcedUnsafe),
+                ("UP001", FixSafety::ForcedUnsafe),
+                ("UP003", FixSafety::ForcedSafe),
+            ],
+        );
+    }
+
+    #[test]
+    fn test_from_rule_selectors_unsafe_over_safe() {
+        let table = mk_table(&["UP"], &["UP"]);
+
+        assert_rules_safety(
+            &table,
+            &[
+                ("E101", FixSafety::Default),
+                ("UP001", FixSafety::ForcedUnsafe),
+            ],
+        );
+    }
+}

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