From a23e489c791ff8d41ce0500db84457b848084124 Mon Sep 17 00:00:00 2001 From: Dylan Date: Tue, 18 Feb 2025 12:52:26 -0600 Subject: [PATCH] [`refurb`] Correctly handle lengths of literal strings in `slice-to-remove-prefix-or-suffix` (`FURB188`) (#16237) Fixes false negative when slice bound uses length of string literal. We were meant to check the following, for example. Given: ```python text[:bound] if text.endswith(suffix) else text ``` We want to know whether: - `suffix` is a string literal and `bound` is a number literal - `suffix` is an expression and `bound` is exactly `-len(suffix)` (as AST nodes, prior to evaluation.) The issue is that negative number literals like `-10` are stored as unary operators applied to a number literal in the AST. So when `suffix` was a string literal but `bound` was `-len(suffix)` we were getting caught in the match arm where `bound` needed to be a number. This is now fixed with a guard. Closes #16231 --- .../resources/test/fixtures/refurb/FURB188.py | 9 ++++++++- .../rules/slice_to_remove_prefix_or_suffix.rs | 2 +- ...es__refurb__tests__FURB188_FURB188.py.snap | 20 +++++++++++++++++++ 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB188.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB188.py index 4593559518..8f58cb9d8f 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB188.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB188.py @@ -197,4 +197,11 @@ def handle_surrogates(): text = "\ud800\udc00heythere" if text.startswith("\ud800\udc00"): text = text[1:] - \ No newline at end of file + +# Regression test for +# https://github.com/astral-sh/ruff/issues/16231 +def func(): + a = "sjdfaskldjfakljklfoo" + if a.endswith("foo"): + a = a[: -len("foo")] + print(a) 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 c475bc75d1..4623c008c3 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 @@ -365,7 +365,7 @@ fn affix_matches_slice_bound(data: &RemoveAffixData, semantic: &SemanticModel) - range: _, value: string_val, }), - ) => operand.as_number_literal_expr().is_some_and( + ) if operand.is_number_literal_expr() => operand.as_number_literal_expr().is_some_and( |ast::ExprNumberLiteral { value, .. }| { // Only support prefix removal for size at most `u32::MAX` value 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 4627dab5ec..70966a24be 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 @@ -313,3 +313,23 @@ FURB188.py:193:5: FURB188 [*] Prefer `str.removeprefix()` over conditionally rep 195 194 | 196 195 | # should not be linted 197 196 | text = "\ud800\udc00heythere" + +FURB188.py:205:5: FURB188 [*] Prefer `str.removesuffix()` over conditionally replacing with slice. + | +203 | def func(): +204 | a = "sjdfaskldjfakljklfoo" +205 | / if a.endswith("foo"): +206 | | a = a[: -len("foo")] + | |____________________________^ FURB188 +207 | print(a) + | + = help: Use removesuffix instead of assignment conditional upon endswith. + +ℹ Safe fix +202 202 | # https://github.com/astral-sh/ruff/issues/16231 +203 203 | def func(): +204 204 | a = "sjdfaskldjfakljklfoo" +205 |- if a.endswith("foo"): +206 |- a = a[: -len("foo")] + 205 |+ a = a.removesuffix("foo") +207 206 | print(a)