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