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 10c0f8d8a3..790186ade9 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,8 @@ +use ruff_diagnostics::Applicability; 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, is_bytes, is_string}; +use ruff_python_semantic::{Binding, SemanticModel}; use ruff_text_size::Ranged; use crate::Locator; @@ -79,6 +81,10 @@ 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(applicability) = affix_applicability(&removal_data, checker.semantic()) else { + return; + }; + let mut diagnostic = checker.report_diagnostic( SliceToRemovePrefixOrSuffix { affix_kind: kind, @@ -89,11 +95,8 @@ 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(Fix::applicable_edit(edit, applicability)); } } } @@ -108,6 +111,10 @@ 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(applicability) = affix_applicability(&removal_data, checker.semantic()) else { + return; + }; + let mut diagnostic = checker.report_diagnostic( SliceToRemovePrefixOrSuffix { affix_kind: kind, @@ -122,11 +129,8 @@ 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(Fix::applicable_edit(edit, applicability)); } } } @@ -501,3 +505,90 @@ struct RemoveAffixData<'a> { /// Contains the prefix or suffix used in `text.startswith(prefix)` or `text.endswith(suffix)` affix_query: AffixQuery<'a>, } + +/// 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 { + // Check for tuple affixes - these should be suppressed + if is_tuple_affix(data.affix_query.affix, semantic) { + return None; + } + + 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); + + 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), + } +} + +/// 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 matches!(value, ast::Expr::Tuple(_)); + } + } + } + + 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 +} 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..09697246bc 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 @@ -130,6 +130,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 @@ -213,6 +214,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 +236,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 +257,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 +302,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 +322,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