From 47e41ac6b66d81e2aa8a5249985d255e03e52aaa Mon Sep 17 00:00:00 2001 From: Dan Parizher <105245560+danparizher@users.noreply.github.com> Date: Tue, 4 Nov 2025 11:02:50 -0500 Subject: [PATCH] [`refurb`] Fix false negative for underscores before sign in `Decimal` constructor (`FURB157`) (#21190) ## Summary Fixes FURB157 false negative where `Decimal("_-1")` was not flagged as verbose when underscores precede the sign character. This fixes #21186. ## Problem Analysis The `verbose-decimal-constructor` (FURB157) rule failed to detect verbose `Decimal` constructors when the sign character (`+` or `-`) was preceded by underscores. For example, `Decimal("_-1")` was not flagged, even though it can be simplified to `Decimal(-1)`. The bug occurred because the rule checked for the sign character at the start of the string before stripping leading underscores. According to Python's `Decimal` parser behavior (as documented in CPython's `_pydecimal.py`), underscores are removed before parsing the sign. The rule's logic didn't match this behavior, causing a false negative for cases like `"_-1"` where the underscore came before the sign. This was a regression introduced in version 0.14.3, as these cases were correctly flagged in version 0.14.2. ## Approach The fix updates the sign extraction logic to: 1. Strip leading underscores first (matching Python's Decimal parser behavior) 2. Extract the sign from the underscore-stripped string 3. Preserve the string after the sign for normalization purposes This ensures that cases like `Decimal("_-1")`, `Decimal("_+1")`, and `Decimal("_-1_000")` are correctly detected and flagged. The normalization logic was also updated to use the string after the sign (without underscores) to avoid double signs in the replacement output. --- .../resources/test/fixtures/refurb/FURB157.py | 6 ++ .../rules/verbose_decimal_constructor.rs | 17 ++++-- ...es__refurb__tests__FURB157_FURB157.py.snap | 59 +++++++++++++++++++ 3 files changed, 76 insertions(+), 6 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB157.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB157.py index d795fd1941..db49315f54 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB157.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB157.py @@ -85,3 +85,9 @@ Decimal("1234_5678") # Safe fix: preserves non-thousands separators Decimal("0001_2345") Decimal("000_1_2345") Decimal("000_000") + +# Test cases for underscores before sign +# https://github.com/astral-sh/ruff/issues/21186 +Decimal("_-1") # Should flag as verbose +Decimal("_+1") # Should flag as verbose +Decimal("_-1_000") # Should flag as verbose diff --git a/crates/ruff_linter/src/rules/refurb/rules/verbose_decimal_constructor.rs b/crates/ruff_linter/src/rules/refurb/rules/verbose_decimal_constructor.rs index 50c98026d5..28779b021a 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/verbose_decimal_constructor.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/verbose_decimal_constructor.rs @@ -93,16 +93,21 @@ pub(crate) fn verbose_decimal_constructor(checker: &Checker, call: &ast::ExprCal // https://github.com/python/cpython/blob/ac556a2ad1213b8bb81372fe6fb762f5fcb076de/Lib/_pydecimal.py#L6060-L6077 // _after_ trimming whitespace from the string and removing all occurrences of "_". let original_str = str_literal.to_str().trim_whitespace(); + // Strip leading underscores before extracting the sign, as Python's Decimal parser + // removes underscores before parsing the sign. + let sign_check_str = original_str.trim_start_matches('_'); // Extract the unary sign, if any. - let (unary, original_str) = if let Some(trimmed) = original_str.strip_prefix('+') { + let (unary, sign_check_str) = if let Some(trimmed) = sign_check_str.strip_prefix('+') { ("+", trimmed) - } else if let Some(trimmed) = original_str.strip_prefix('-') { + } else if let Some(trimmed) = sign_check_str.strip_prefix('-') { ("-", trimmed) } else { - ("", original_str) + ("", sign_check_str) }; - let mut rest = Cow::from(original_str); - let has_digit_separators = memchr::memchr(b'_', rest.as_bytes()).is_some(); + // Save the string after the sign for normalization (before removing underscores) + let str_after_sign_for_normalization = sign_check_str; + let mut rest = Cow::from(sign_check_str); + let has_digit_separators = memchr::memchr(b'_', original_str.as_bytes()).is_some(); if has_digit_separators { rest = Cow::from(rest.replace('_', "")); } @@ -123,7 +128,7 @@ pub(crate) fn verbose_decimal_constructor(checker: &Checker, call: &ast::ExprCal // If the original string had digit separators, normalize them let rest = if has_digit_separators { - Cow::from(normalize_digit_separators(original_str)) + Cow::from(normalize_digit_separators(str_after_sign_for_normalization)) } else { Cow::from(rest) }; diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB157_FURB157.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB157_FURB157.py.snap index 92e8057055..3f0a1c2cf6 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB157_FURB157.py.snap +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB157_FURB157.py.snap @@ -669,6 +669,7 @@ help: Replace with `1_2345` 85 + Decimal(1_2345) 86 | Decimal("000_1_2345") 87 | Decimal("000_000") +88 | FURB157 [*] Verbose expression in `Decimal` constructor --> FURB157.py:86:9 @@ -686,6 +687,8 @@ help: Replace with `1_2345` - Decimal("000_1_2345") 86 + Decimal(1_2345) 87 | Decimal("000_000") +88 | +89 | # Test cases for underscores before sign FURB157 [*] Verbose expression in `Decimal` constructor --> FURB157.py:87:9 @@ -694,6 +697,8 @@ FURB157 [*] Verbose expression in `Decimal` constructor 86 | Decimal("000_1_2345") 87 | Decimal("000_000") | ^^^^^^^^^ +88 | +89 | # Test cases for underscores before sign | help: Replace with `0` 84 | # Separators _and_ leading zeros @@ -701,3 +706,57 @@ help: Replace with `0` 86 | Decimal("000_1_2345") - Decimal("000_000") 87 + Decimal(0) +88 | +89 | # Test cases for underscores before sign +90 | # https://github.com/astral-sh/ruff/issues/21186 + +FURB157 [*] Verbose expression in `Decimal` constructor + --> FURB157.py:91:9 + | +89 | # Test cases for underscores before sign +90 | # https://github.com/astral-sh/ruff/issues/21186 +91 | Decimal("_-1") # Should flag as verbose + | ^^^^^ +92 | Decimal("_+1") # Should flag as verbose +93 | Decimal("_-1_000") # Should flag as verbose + | +help: Replace with `-1` +88 | +89 | # Test cases for underscores before sign +90 | # https://github.com/astral-sh/ruff/issues/21186 + - Decimal("_-1") # Should flag as verbose +91 + Decimal(-1) # Should flag as verbose +92 | Decimal("_+1") # Should flag as verbose +93 | Decimal("_-1_000") # Should flag as verbose + +FURB157 [*] Verbose expression in `Decimal` constructor + --> FURB157.py:92:9 + | +90 | # https://github.com/astral-sh/ruff/issues/21186 +91 | Decimal("_-1") # Should flag as verbose +92 | Decimal("_+1") # Should flag as verbose + | ^^^^^ +93 | Decimal("_-1_000") # Should flag as verbose + | +help: Replace with `+1` +89 | # Test cases for underscores before sign +90 | # https://github.com/astral-sh/ruff/issues/21186 +91 | Decimal("_-1") # Should flag as verbose + - Decimal("_+1") # Should flag as verbose +92 + Decimal(+1) # Should flag as verbose +93 | Decimal("_-1_000") # Should flag as verbose + +FURB157 [*] Verbose expression in `Decimal` constructor + --> FURB157.py:93:9 + | +91 | Decimal("_-1") # Should flag as verbose +92 | Decimal("_+1") # Should flag as verbose +93 | Decimal("_-1_000") # Should flag as verbose + | ^^^^^^^^^ + | +help: Replace with `-1_000` +90 | # https://github.com/astral-sh/ruff/issues/21186 +91 | Decimal("_-1") # Should flag as verbose +92 | Decimal("_+1") # Should flag as verbose + - Decimal("_-1_000") # Should flag as verbose +93 + Decimal(-1_000) # Should flag as verbose