Skip to content

Commit

Permalink
[refurb] Parse more exotic decimal strings in `verbose-decimal-constr…
Browse files Browse the repository at this point in the history
…uctor (FURB157)` (#14098)

FURB157 suggests replacing expressions like `Decimal("123")` with
`Decimal(123)`. This PR extends the rule to cover cases where the input
string to `Decimal` can be easily transformed into an integer literal.

For example:

```python
Decimal("1__000")   # fix: `Decimal(1000)`
```

Note: we do not implement the full decimal parsing logic from CPython on
the grounds that certain acceptable string inputs to the `Decimal`
constructor may be presumed purposeful on the part of the developer. For
example, as in the linked issue, `Decimal("١٢٣")` is valid and equal to
`Decimal(123)`, but we do not suggest a replacement in this case.

Closes #13807
  • Loading branch information
dylwil3 authored Nov 5, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent 239cbc6 commit 2b76fa8
Showing 3 changed files with 96 additions and 5 deletions.
20 changes: 20 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB157.py
Original file line number Diff line number Diff line change
@@ -15,3 +15,23 @@
Decimal(0)
Decimal("Infinity")
decimal.Decimal(0)

# Handle Python's Decimal parsing
# See https://github.com/astral-sh/ruff/issues/13807

# Errors
Decimal("1_000")
Decimal("__1____000")

# Ok
Decimal("2e-4")
Decimal("2E-4")
Decimal("_1.234__")
Decimal("2e4")
Decimal("2e+4")
Decimal("2E4")
Decimal("1.2")
# Ok: even though this is equal to `Decimal(123)`,
# we assume that a developer would
# only write it this way if they meant to.
Decimal("١٢٣")
Original file line number Diff line number Diff line change
@@ -3,6 +3,7 @@ use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr};
use ruff_python_trivia::PythonWhitespace;
use ruff_text_size::Ranged;
use std::borrow::Cow;

use crate::checkers::ast::Checker;

@@ -75,13 +76,20 @@ pub(crate) fn verbose_decimal_constructor(checker: &mut Checker, call: &ast::Exp
value: str_literal, ..
}) => {
// Parse the inner string as an integer.
let trimmed = str_literal.to_str().trim_whitespace();

//
// For reference, a string argument to `Decimal` is parsed in CPython
// using this regex:
// https://github.com/python/cpython/blob/ac556a2ad1213b8bb81372fe6fb762f5fcb076de/Lib/_pydecimal.py#L6060-L6077
// _after_ trimming whitespace from the string and removing all occurrences of "_".
let mut trimmed = Cow::from(str_literal.to_str().trim_whitespace());
if memchr::memchr(b'_', trimmed.as_bytes()).is_some() {
trimmed = Cow::from(trimmed.replace('_', ""));
}
// Extract the unary sign, if any.
let (unary, rest) = if let Some(trimmed) = trimmed.strip_prefix('+') {
("+", trimmed)
("+", Cow::from(trimmed))
} else if let Some(trimmed) = trimmed.strip_prefix('-') {
("-", trimmed)
("-", Cow::from(trimmed))
} else {
("", trimmed)
};
@@ -90,7 +98,7 @@ pub(crate) fn verbose_decimal_constructor(checker: &mut Checker, call: &ast::Exp
let rest = rest.trim_start_matches('0');

// Verify that the rest of the string is a valid integer.
if !rest.chars().all(|c| c.is_ascii_digit()) {
if !rest.bytes().all(|c| c.is_ascii_digit()) {
return;
};

@@ -159,3 +167,26 @@ pub(crate) fn verbose_decimal_constructor(checker: &mut Checker, call: &ast::Exp

checker.diagnostics.push(diagnostic);
}

// // Slightly modified from [CPython regex] to ignore https://github.com/python/cpython/blob/ac556a2ad1213b8bb81372fe6fb762f5fcb076de/Lib/_pydecimal.py#L6060-L6077
// static DECIMAL_PARSER_REGEX: LazyLock<Regex> = LazyLock::new(|| {
// Regex::new(
// r"(?x) # Verbose mode for comments
// ^ # Start of string
// (?P<sign>[-+])? # Optional sign
// (?:
// (?P<int>\d*) # Integer part (can be empty)
// (\.(?P<frac>\d+))? # Optional fractional part
// (E(?P<exp>[-+]?\d+))? # Optional exponent
// |
// Inf(inity)? # Infinity
// |
// (?P<signal>s)? # Optional signal
// NaN # NaN
// (?P<diag>\d*) # Optional diagnostic info
// )
// $ # End of string
// ",
// )
// .unwrap()
// });
Original file line number Diff line number Diff line change
@@ -166,3 +166,43 @@ FURB157.py:12:17: FURB157 [*] Verbose expression in `Decimal` constructor
13 13 |
14 14 | # OK
15 15 | Decimal(0)

FURB157.py:23:9: FURB157 [*] Verbose expression in `Decimal` constructor
|
22 | # Errors
23 | Decimal("1_000")
| ^^^^^^^ FURB157
24 | Decimal("__1____000")
|
= help: Replace with `1000`

Safe fix
20 20 | # See https://github.com/astral-sh/ruff/issues/13807
21 21 |
22 22 | # Errors
23 |-Decimal("1_000")
23 |+Decimal(1000)
24 24 | Decimal("__1____000")
25 25 |
26 26 | # Ok

FURB157.py:24:9: FURB157 [*] Verbose expression in `Decimal` constructor
|
22 | # Errors
23 | Decimal("1_000")
24 | Decimal("__1____000")
| ^^^^^^^^^^^^ FURB157
25 |
26 | # Ok
|
= help: Replace with `1000`

Safe fix
21 21 |
22 22 | # Errors
23 23 | Decimal("1_000")
24 |-Decimal("__1____000")
24 |+Decimal(1000)
25 25 |
26 26 | # Ok
27 27 | Decimal("2e-4")

0 comments on commit 2b76fa8

Please sign in to comment.