Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[verbose-decimal-constructor (FURB157)] doesn't trigger on Decimal("1_000") #13807

Open
monosans opened this issue Oct 18, 2024 · 4 comments
Open
Labels
bug Something isn't working help wanted Contributions especially welcome preview Related to preview mode features

Comments

@monosans
Copy link
Contributor

Decimal(1000)  # OK
Decimal(1_000)  # OK
Decimal("1000")  # Triggers FURB157
Decimal("1_000")  # Should trigger FURB157 and become Decimal(1_000), but it doesn't

ruff v0.7.0

@AlexWaygood
Copy link
Member

It seems like the parsing we do here is too naive:

// Parse the inner string as an integer.
let trimmed = str_literal.to_str().trim_whitespace();
// Extract the unary sign, if any.
let (unary, rest) = if let Some(trimmed) = trimmed.strip_prefix('+') {
("+", trimmed)
} else if let Some(trimmed) = trimmed.strip_prefix('-') {
("-", trimmed)
} else {
("", trimmed)
};
// Skip leading zeros.
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()) {
return;
};
// If all the characters are zeros, then the value is zero.
let rest = if rest.is_empty() { "0" } else { rest };

@AlexWaygood AlexWaygood added bug Something isn't working help wanted Contributions especially welcome preview Related to preview mode features labels Oct 18, 2024
@AlexWaygood
Copy link
Member

AlexWaygood commented Oct 18, 2024

If there are more edge cases that we don't handle, it might be worth just using ruff_python_parser::parse_expression to determine if the contents of the string can be parsed as an int literal, rather than reimplementing all the edge cases in the linter. If this is the only edge case we don't currently handle, however, it may be best to avoid doing that and just reimplement the logic in the linter -- using the ruff_python_parser function might be comparatively quite expensive.

@dscorbett
Copy link

Decimal accepts more integer-valued strings than are valid as int literals. For example, Decimal("1__2") and Decimal("_1_") and Decimal("١٢٣") are valid. Should FURB157 trigger on those? If so, ruff_python_parser::parse_expression would not be enough.

@dylwil3
Copy link
Contributor

dylwil3 commented Oct 19, 2024

Looks like this is the official regex:

https://github.com/python/cpython/blob/322f14eeff9e3b5853eaac3233f7580ca0214cf8/Lib/_pydecimal.py#L6059-L6077

I can try to take this sometime this weekend (but if I don't, others should feel free to go for it!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Contributions especially welcome preview Related to preview mode features
Projects
None yet
Development

No branches or pull requests

4 participants