Implement safety checks for affix removal in slice_to_remove_prefix_or_suffix rule

- Added `FixSafety` enum to determine if affix removal is safe or unsafe.
- Updated diagnostic fixes to use safety checks when applying replacements for prefix and suffix removals.
- Introduced helper functions to classify expression types and assess safety based on semantic analysis.
- Enhanced test cases to cover new functionality and ensure correct behavior of affix removal logic.

This change improves the reliability of the affix removal feature by preventing unsafe modifications that could alter runtime behavior.

crates/ruff_linter/resources/test/fixtures/refurb/FURB188.py

@@ -205,3 +205,27 @@ def func():
     if a.endswith("foo"):
         a = a[: -len("foo")]
     print(a)
+
+
+class Seq:
+    def __init__(self, inner):
+        self.inner = inner
+
+    def startswith(self, prefix):
+        return tuple(self.inner[: len(prefix)]) == tuple(prefix)
+
+    def __getitem__(self, item):
+        return type(self)(self.inner[item])
+
+
+def tuple_affix_should_be_suppressed(text: str) -> str:
+    prefix = ("123",)
+    if text.startswith(prefix):
+        text = text[len(prefix):]
+    return text
+
+
+def custom_sequence_fix_is_unsafe(seq: Seq) -> Seq:
+    if seq.startswith("123"):
+        seq = seq[3:]
+    return seq

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

@@ -1,6 +1,9 @@
+use rustc_hash::FxHashSet;
+
 use ruff_macros::{ViolationMetadata, derive_message_formats};
 use ruff_python_ast::{self as ast, PythonVersion};
-use ruff_python_semantic::SemanticModel;
+use ruff_python_semantic::analyze::typing::find_binding_value;
+use ruff_python_semantic::{BindingId, SemanticModel};
 use ruff_text_size::Ranged;
 
 use crate::Locator;
@@ -78,6 +81,11 @@ 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 {
+                return;
+            };
+
             let mut diagnostic = checker.report_diagnostic(
                 SliceToRemovePrefixOrSuffix {
                     affix_kind: kind,
@@ -88,11 +96,11 @@ pub(crate) fn slice_to_remove_affix_expr(checker: &Checker, if_expr: &ast::ExprI
             let replacement =
                 generate_removeaffix_expr(text, &removal_data.affix_query, checker.locator());
 
-            diagnostic.set_fix(Fix::safe_edit(Edit::replacement(
+            let edit = Edit::replacement(replacement, if_expr.start(), if_expr.end());
-                replacement,
+            diagnostic.set_fix(match fix_safety {
-                if_expr.start(),
+                FixSafety::Safe => Fix::safe_edit(edit),
-                if_expr.end(),
+                FixSafety::Unsafe => Fix::unsafe_edit(edit),
-            )));
+            });
         }
     }
 }
@@ -107,6 +115,11 @@ 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 {
+                return;
+            };
+
             let mut diagnostic = checker.report_diagnostic(
                 SliceToRemovePrefixOrSuffix {
                     affix_kind: kind,
@@ -121,11 +134,11 @@ pub(crate) fn slice_to_remove_affix_stmt(checker: &Checker, if_stmt: &ast::StmtI
                 checker.locator(),
             );
 
-            diagnostic.set_fix(Fix::safe_edit(Edit::replacement(
+            let edit = Edit::replacement(replacement, if_stmt.start(), if_stmt.end());
-                replacement,
+            diagnostic.set_fix(match fix_safety {
-                if_stmt.start(),
+                FixSafety::Safe => Fix::safe_edit(edit),
-                if_stmt.end(),
+                FixSafety::Unsafe => Fix::unsafe_edit(edit),
-            )));
+            });
         }
     }
 }
@@ -500,3 +513,84 @@ struct RemoveAffixData<'a> {
     /// Contains the prefix or suffix used in `text.startswith(prefix)` or `text.endswith(suffix)`
     affix_query: AffixQuery<'a>,
 }
+
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+enum FixSafety {
+    Safe,
+    Unsafe,
+}
+
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+enum TypeKnowledge {
+    KnownStr,
+    KnownBytes,
+    KnownNonStr,
+    Unknown,
+}
+
+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),
+    }
+}
+
+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;
+    }
+
+    let binding = semantic.binding(binding_id);
+    if let Some(value) = find_binding_value(binding, semantic) {
+        return classify_expr_type_inner(value, semantic, seen);
+    }
+
+    TypeKnowledge::Unknown
+}

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

@@ -1,5 +1,6 @@
 ---
 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
@@ -21,6 +22,7 @@ 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
@@ -42,6 +44,7 @@ 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
@@ -59,6 +62,7 @@ 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
@@ -76,6 +80,7 @@ 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
@@ -93,6 +98,7 @@ 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
@@ -110,6 +116,7 @@ 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
@@ -130,6 +137,7 @@ help: Use removesuffix instead of ternary expression conditional upon endswith.
 147 | 
 148 | def remove_prefix_comparable_literal_expr() -> None:
 149 |     return ("abc" "def")[3:] if ("abc" "def").startswith("abc") else "abc" "def"
+note: This is an unsafe fix and may change runtime behavior
 
 FURB188 [*] Prefer `str.removeprefix()` over conditionally replacing with slice.
    --> FURB188.py:149:12
@@ -169,6 +177,7 @@ 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
@@ -213,6 +222,7 @@ help: Use removeprefix instead of assignment conditional upon startswith.
 161 |     if text.startswith("!"):
 162 |         text = text[1::None]
 163 |     print(text)
+note: This is an unsafe fix and may change runtime behavior
 
 FURB188 [*] Prefer `str.removeprefix()` over conditionally replacing with slice.
    --> FURB188.py:162:5
@@ -234,6 +244,7 @@ help: Use removeprefix instead of assignment conditional upon startswith.
 163 |     print(text)
 164 | 
 165 | 
+note: This is an unsafe fix and may change runtime behavior
 
 FURB188 [*] Prefer `str.removeprefix()` over conditionally replacing with slice.
    --> FURB188.py:183:5
@@ -254,6 +265,7 @@ help: Use removeprefix instead of assignment conditional upon startswith.
 184 | 
 185 | 
 186 | def handle_surrogates():
+note: This is an unsafe fix and may change runtime behavior
 
 FURB188 [*] Prefer `str.removeprefix()` over conditionally replacing with slice.
    --> FURB188.py:190:5
@@ -298,6 +310,7 @@ help: Use removeprefix instead of assignment conditional upon startswith.
 194 |     
 195 |     # should not be linted
 196 |     text = "\ud800\udc00heythere"
+note: This is an unsafe fix and may change runtime behavior
 
 FURB188 [*] Prefer `str.removesuffix()` over conditionally replacing with slice.
    --> FURB188.py:205:5
@@ -317,3 +330,24 @@ help: Use removesuffix instead of assignment conditional upon endswith.
     -         a = a[: -len("foo")]
 205 +     a = a.removesuffix("foo")
 206 |     print(a)
+207 | 
+208 | 
+
+FURB188 [*] Prefer `str.removeprefix()` over conditionally replacing with slice.
+   --> FURB188.py:229:5
+    |
+228 |   def custom_sequence_fix_is_unsafe(seq: Seq) -> Seq:
+229 | /     if seq.startswith("123"):
+230 | |         seq = seq[3:]
+    | |_____________________^
+231 |       return seq
+    |
+help: Use removeprefix instead of assignment conditional upon startswith.
+226 | 
+227 | 
+228 | def custom_sequence_fix_is_unsafe(seq: Seq) -> Seq:
+    -     if seq.startswith("123"):
+    -         seq = seq[3:]
+229 +     seq = seq.removeprefix("123")
+230 |     return seq
+note: This is an unsafe fix and may change runtime behavior