From 968c9786aa980badac6f9688579a3d75e5a5431a Mon Sep 17 00:00:00 2001 From: shubham humbe Date: Fri, 10 Oct 2025 19:02:05 +0530 Subject: [PATCH] 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. --- .../resources/test/fixtures/refurb/FURB188.py | 24 ++++ .../rules/slice_to_remove_prefix_or_suffix.rs | 116 ++++++++++++++++-- ...es__refurb__tests__FURB188_FURB188.py.snap | 34 +++++ 3 files changed, 163 insertions(+), 11 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB188.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB188.py index 8f58cb9d8f..e686d24c45 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB188.py +++ b/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 diff --git a/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs b/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs index 0cfd94f3bc..2a06d0aedc 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs +++ b/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( - replacement, - if_expr.start(), - if_expr.end(), - ))); + 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), + }); } } } @@ -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( - replacement, - if_stmt.start(), - if_stmt.end(), - ))); + 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), + }); } } } @@ -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 { + 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, +) -> 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, +) -> 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 +} diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB188_FURB188.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB188_FURB188.py.snap index 21e50d3130..599d41d3c9 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB188_FURB188.py.snap +++ b/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