Refactor affix removal logic in slice_to_remove_prefix_or_suffix rule

- Replaced `FixSafety` enum with `Applicability` to determine the suitability of affix removal fixes.
- Introduced new helper functions to assess expression types and their applicability for affix removal.
- Updated diagnostic fixes to utilize the new applicability checks, enhancing the safety of modifications.
- Improved code clarity and maintainability by consolidating type-checking logic.

crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs

@@ -1,9 +1,8 @@
-use rustc_hash::FxHashSet;
-
+use ruff_diagnostics::Applicability;
 use ruff_macros::{ViolationMetadata, derive_message_formats};
 use ruff_python_ast::{self as ast, PythonVersion};
-use ruff_python_semantic::analyze::typing::find_binding_value;
-use ruff_python_semantic::{BindingId, SemanticModel};
+use ruff_python_semantic::analyze::typing::{find_binding_value, is_bytes, is_string};
+use ruff_python_semantic::{Binding, SemanticModel};
 use ruff_text_size::Ranged;
 
 use crate::Locator;
@@ -81,8 +80,7 @@ pub(crate) fn slice_to_remove_affix_expr(checker: &Checker, if_expr: &ast::ExprI
             let kind = removal_data.affix_query.kind;
             let text = removal_data.text;
 
-            let Some(fix_safety) = remove_affix_fix_safety(&removal_data, checker.semantic())
-            else {
+            let Some(applicability) = affix_applicability(&removal_data, checker.semantic()) else {
                 return;
             };
 
@@ -97,10 +95,7 @@ pub(crate) fn slice_to_remove_affix_expr(checker: &Checker, if_expr: &ast::ExprI
                 generate_removeaffix_expr(text, &removal_data.affix_query, checker.locator());
 
             let edit = Edit::replacement(replacement, if_expr.start(), if_expr.end());
-            diagnostic.set_fix(match fix_safety {
-                FixSafety::Safe => Fix::safe_edit(edit),
-                FixSafety::Unsafe => Fix::unsafe_edit(edit),
-            });
+            diagnostic.set_fix(Fix::applicable_edit(edit, applicability));
         }
     }
 }
@@ -115,8 +110,7 @@ pub(crate) fn slice_to_remove_affix_stmt(checker: &Checker, if_stmt: &ast::StmtI
             let kind = removal_data.affix_query.kind;
             let text = removal_data.text;
 
-            let Some(fix_safety) = remove_affix_fix_safety(&removal_data, checker.semantic())
-            else {
+            let Some(applicability) = affix_applicability(&removal_data, checker.semantic()) else {
                 return;
             };
 
@@ -135,10 +129,7 @@ pub(crate) fn slice_to_remove_affix_stmt(checker: &Checker, if_stmt: &ast::StmtI
             );
 
             let edit = Edit::replacement(replacement, if_stmt.start(), if_stmt.end());
-            diagnostic.set_fix(match fix_safety {
-                FixSafety::Safe => Fix::safe_edit(edit),
-                FixSafety::Unsafe => Fix::unsafe_edit(edit),
-            });
+            diagnostic.set_fix(Fix::applicable_edit(edit, applicability));
         }
     }
 }
@@ -514,83 +505,89 @@ struct RemoveAffixData<'a> {
     affix_query: AffixQuery<'a>,
 }
 
-#[derive(Copy, Clone, Debug, Eq, PartialEq)]
-enum FixSafety {
-    Safe,
-    Unsafe,
+/// Determines the applicability of the affix removal fix based on type information.
+///
+/// Returns:
+/// - `None` if the fix should be suppressed (incompatible types like tuple affixes)
+/// - `Some(Applicability::Safe)` if both text and affix are deterministically typed as str or bytes
+/// - `Some(Applicability::Unsafe)` if type information is unknown or uncertain
+fn affix_applicability(data: &RemoveAffixData, semantic: &SemanticModel) -> Option<Applicability> {
+    // Check for tuple affixes - these should be suppressed
+    if is_tuple_affix(data.affix_query.affix, semantic) {
+        return None;
     }
 
-#[derive(Copy, Clone, Debug, Eq, PartialEq)]
-enum TypeKnowledge {
-    KnownStr,
-    KnownBytes,
-    KnownNonStr,
-    Unknown,
-}
+    let text_is_str = is_expr_string(data.text, semantic);
+    let text_is_bytes = is_expr_bytes(data.text, semantic);
+    let affix_is_str = is_expr_string(data.affix_query.affix, semantic);
+    let affix_is_bytes = is_expr_bytes(data.affix_query.affix, semantic);
 
-fn remove_affix_fix_safety(data: &RemoveAffixData, semantic: &SemanticModel) -> Option<FixSafety> {
-    let text_knowledge = classify_expr_type(data.text, semantic);
-    let affix_knowledge = classify_expr_type(data.affix_query.affix, semantic);
-
-    match (text_knowledge, affix_knowledge) {
-        (TypeKnowledge::KnownNonStr, _) | (_, TypeKnowledge::KnownNonStr) => None,
-        (TypeKnowledge::KnownStr, TypeKnowledge::KnownStr) => Some(FixSafety::Safe),
-        (TypeKnowledge::KnownBytes, TypeKnowledge::KnownBytes) => Some(FixSafety::Safe),
-        (TypeKnowledge::KnownStr, TypeKnowledge::KnownBytes)
-        | (TypeKnowledge::KnownBytes, TypeKnowledge::KnownStr) => None,
-        _ => Some(FixSafety::Unsafe),
+    match (text_is_str, text_is_bytes, affix_is_str, affix_is_bytes) {
+        // Both are deterministically str
+        (true, false, true, false) => Some(Applicability::Safe),
+        // Both are deterministically bytes
+        (false, true, false, true) => Some(Applicability::Safe),
+        // Type mismatch - suppress the fix
+        (true, false, false, true) | (false, true, true, false) => None,
+        // Unknown or ambiguous types - mark as unsafe
+        _ => Some(Applicability::Unsafe),
     }
 }
 
-fn classify_expr_type(expr: &ast::Expr, semantic: &SemanticModel) -> TypeKnowledge {
-    let mut seen = FxHashSet::default();
-    classify_expr_type_inner(expr, semantic, &mut seen)
-}
-
-fn classify_expr_type_inner(
-    expr: &ast::Expr,
-    semantic: &SemanticModel,
-    seen: &mut FxHashSet<BindingId>,
-) -> TypeKnowledge {
-    use ast::Expr;
-
-    match expr {
-        _ if expr.is_string_literal_expr() || expr.is_f_string_expr() => TypeKnowledge::KnownStr,
-        Expr::BytesLiteral(_) => TypeKnowledge::KnownBytes,
-        Expr::Name(name) => classify_name_expr_type(name, semantic, seen),
-        Expr::NumberLiteral(_)
-        | Expr::BooleanLiteral(_)
-        | Expr::NoneLiteral(_)
-        | Expr::EllipsisLiteral(_)
-        | Expr::Tuple(_)
-        | Expr::List(_)
-        | Expr::Set(_)
-        | Expr::Dict(_)
-        | Expr::ListComp(_)
-        | Expr::SetComp(_)
-        | Expr::DictComp(_)
-        | Expr::Generator(_) => TypeKnowledge::KnownNonStr,
-        _ => TypeKnowledge::Unknown,
-    }
-}
-
-fn classify_name_expr_type(
-    name: &ast::ExprName,
-    semantic: &SemanticModel,
-    seen: &mut FxHashSet<BindingId>,
-) -> TypeKnowledge {
-    let Some(binding_id) = semantic.only_binding(name) else {
-        return TypeKnowledge::Unknown;
-    };
-
-    if !seen.insert(binding_id) {
-        return TypeKnowledge::Unknown;
+/// Check if an expression is a tuple or a variable bound to a tuple.
+fn is_tuple_affix(expr: &ast::Expr, semantic: &SemanticModel) -> bool {
+    if matches!(expr, ast::Expr::Tuple(_)) {
+        return true;
     }
 
+    if let ast::Expr::Name(name) = expr {
+        if let Some(binding_id) = semantic.only_binding(name) {
             let binding = semantic.binding(binding_id);
             if let Some(value) = find_binding_value(binding, semantic) {
-        return classify_expr_type_inner(value, semantic, seen);
+                return matches!(value, ast::Expr::Tuple(_));
+            }
+        }
     }
 
-    TypeKnowledge::Unknown
+    false
+}
+
+/// Check if an expression is deterministically a string literal or a variable bound to a string.
+fn is_expr_string(expr: &ast::Expr, semantic: &SemanticModel) -> bool {
+    is_expr_type(
+        expr,
+        semantic,
+        |expr| expr.is_string_literal_expr() || expr.is_f_string_expr(),
+        is_string,
+    )
+}
+
+/// Check if an expression is deterministically a bytes literal or a variable bound to bytes.
+fn is_expr_bytes(expr: &ast::Expr, semantic: &SemanticModel) -> bool {
+    is_expr_type(
+        expr,
+        semantic,
+        |expr| matches!(expr, ast::Expr::BytesLiteral(_)),
+        is_bytes,
+    )
+}
+
+fn is_expr_type(
+    expr: &ast::Expr,
+    semantic: &SemanticModel,
+    literal_check: impl Fn(&ast::Expr) -> bool,
+    binding_check: impl Fn(&Binding, &SemanticModel) -> bool,
+) -> bool {
+    if literal_check(expr) {
+        return true;
+    }
+
+    if let ast::Expr::Name(name) = expr {
+        if let Some(binding_id) = semantic.only_binding(name) {
+            let binding = semantic.binding(binding_id);
+            return binding_check(binding, semantic);
+        }
+    }
+
+    false
 }

crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB188_FURB188.py.snap

@@ -1,6 +1,5 @@
 ---
 source: crates/ruff_linter/src/rules/refurb/mod.rs
-assertion_line: 62
 ---
 FURB188 [*] Prefer `str.removesuffix()` over conditionally replacing with slice.
   --> FURB188.py:7:5
@@ -22,7 +21,6 @@ help: Use removesuffix instead of assignment conditional upon endswith.
 8  | 
 9  |     return filename
 10 | 
-note: This is an unsafe fix and may change runtime behavior
 
 FURB188 [*] Prefer `str.removesuffix()` over conditionally replacing with slice.
   --> FURB188.py:14:5
@@ -44,7 +42,6 @@ help: Use removesuffix instead of assignment conditional upon endswith.
 15 | 
 16 |     return filename
 17 | 
-note: This is an unsafe fix and may change runtime behavior
 
 FURB188 [*] Prefer `str.removesuffix()` over conditionally replacing with slice.
   --> FURB188.py:21:12
@@ -62,7 +59,6 @@ help: Use removesuffix instead of ternary expression conditional upon endswith.
 22 | 
 23 | 
 24 | def remove_extension_via_ternary_with_len(filename: str, extension: str) -> str:
-note: This is an unsafe fix and may change runtime behavior
 
 FURB188 [*] Prefer `str.removesuffix()` over conditionally replacing with slice.
   --> FURB188.py:25:12
@@ -80,7 +76,6 @@ help: Use removesuffix instead of ternary expression conditional upon endswith.
 26 | 
 27 | 
 28 | def remove_prefix(filename: str) -> str:
-note: This is an unsafe fix and may change runtime behavior
 
 FURB188 [*] Prefer `str.removeprefix()` over conditionally replacing with slice.
   --> FURB188.py:29:12
@@ -98,7 +93,6 @@ help: Use removeprefix instead of ternary expression conditional upon startswith
 30 | 
 31 | 
 32 | def remove_prefix_via_len(filename: str, prefix: str) -> str:
-note: This is an unsafe fix and may change runtime behavior
 
 FURB188 [*] Prefer `str.removeprefix()` over conditionally replacing with slice.
   --> FURB188.py:33:12
@@ -116,7 +110,6 @@ help: Use removeprefix instead of ternary expression conditional upon startswith
 34 | 
 35 | 
 36 | # these should not
-note: This is an unsafe fix and may change runtime behavior
 
 FURB188 [*] Prefer `str.removesuffix()` over conditionally replacing with slice.
    --> FURB188.py:146:9
@@ -177,7 +170,6 @@ help: Use removesuffix instead of ternary expression conditional upon endswith.
 155 | 
 156 | def okay_steps():
 157 |     text = "!x!y!z"
-note: This is an unsafe fix and may change runtime behavior
 
 FURB188 [*] Prefer `str.removeprefix()` over conditionally replacing with slice.
    --> FURB188.py:158:5