[refurb] Implement verbose-decimal-constructor (FURB157) (#10533)

## Summary

Implement FURB157 in the issue #1348.
Relevant Refurb docs is here:
https://github.com/dosisod/refurb/blob/master/docs/checks.md#furb157-simplify-decimal-ctor

## Test Plan

I've written it in the `FURB157.py`.
This commit is contained in:
hikaru-kajita 2024-03-25 11:28:58 +09:00 committed by GitHub
parent 22f237fec6
commit 39fb6d9bfc
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 361 additions and 0 deletions

View file

@ -0,0 +1,17 @@
import decimal
from decimal import Decimal
# Errors
Decimal("0")
Decimal("-42")
Decimal(float("Infinity"))
Decimal(float("-Infinity"))
Decimal(float("inf"))
Decimal(float("-inf"))
Decimal(float("nan"))
decimal.Decimal("0")
# OK
Decimal(0)
Decimal("Infinity")
decimal.Decimal(0)

View file

@ -926,6 +926,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::RedundantLogBase) { if checker.enabled(Rule::RedundantLogBase) {
refurb::rules::redundant_log_base(checker, call); refurb::rules::redundant_log_base(checker, call);
} }
if checker.enabled(Rule::VerboseDecimalConstructor) {
refurb::rules::verbose_decimal_constructor(checker, call);
}
if checker.enabled(Rule::QuadraticListSummation) { if checker.enabled(Rule::QuadraticListSummation) {
ruff::rules::quadratic_list_summation(checker, call); ruff::rules::quadratic_list_summation(checker, call);
} }

View file

@ -1047,6 +1047,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Refurb, "145") => (RuleGroup::Preview, rules::refurb::rules::SliceCopy), (Refurb, "145") => (RuleGroup::Preview, rules::refurb::rules::SliceCopy),
(Refurb, "148") => (RuleGroup::Preview, rules::refurb::rules::UnnecessaryEnumerate), (Refurb, "148") => (RuleGroup::Preview, rules::refurb::rules::UnnecessaryEnumerate),
(Refurb, "152") => (RuleGroup::Preview, rules::refurb::rules::MathConstant), (Refurb, "152") => (RuleGroup::Preview, rules::refurb::rules::MathConstant),
(Refurb, "157") => (RuleGroup::Preview, rules::refurb::rules::VerboseDecimalConstructor),
(Refurb, "161") => (RuleGroup::Preview, rules::refurb::rules::BitCount), (Refurb, "161") => (RuleGroup::Preview, rules::refurb::rules::BitCount),
(Refurb, "163") => (RuleGroup::Preview, rules::refurb::rules::RedundantLogBase), (Refurb, "163") => (RuleGroup::Preview, rules::refurb::rules::RedundantLogBase),
(Refurb, "167") => (RuleGroup::Preview, rules::refurb::rules::RegexFlagAlias), (Refurb, "167") => (RuleGroup::Preview, rules::refurb::rules::RegexFlagAlias),

View file

@ -25,6 +25,7 @@ mod tests {
#[test_case(Rule::SliceCopy, Path::new("FURB145.py"))] #[test_case(Rule::SliceCopy, Path::new("FURB145.py"))]
#[test_case(Rule::UnnecessaryEnumerate, Path::new("FURB148.py"))] #[test_case(Rule::UnnecessaryEnumerate, Path::new("FURB148.py"))]
#[test_case(Rule::MathConstant, Path::new("FURB152.py"))] #[test_case(Rule::MathConstant, Path::new("FURB152.py"))]
#[test_case(Rule::VerboseDecimalConstructor, Path::new("FURB157.py"))]
#[test_case(Rule::PrintEmptyString, Path::new("FURB105.py"))] #[test_case(Rule::PrintEmptyString, Path::new("FURB105.py"))]
#[test_case(Rule::ImplicitCwd, Path::new("FURB177.py"))] #[test_case(Rule::ImplicitCwd, Path::new("FURB177.py"))]
#[test_case(Rule::SingleItemMembershipTest, Path::new("FURB171.py"))] #[test_case(Rule::SingleItemMembershipTest, Path::new("FURB171.py"))]

View file

@ -20,6 +20,7 @@ pub(crate) use single_item_membership_test::*;
pub(crate) use slice_copy::*; pub(crate) use slice_copy::*;
pub(crate) use type_none_comparison::*; pub(crate) use type_none_comparison::*;
pub(crate) use unnecessary_enumerate::*; pub(crate) use unnecessary_enumerate::*;
pub(crate) use verbose_decimal_constructor::*;
mod bit_count; mod bit_count;
mod check_and_remove_from_set; mod check_and_remove_from_set;
@ -43,3 +44,4 @@ mod single_item_membership_test;
mod slice_copy; mod slice_copy;
mod type_none_comparison; mod type_none_comparison;
mod unnecessary_enumerate; mod unnecessary_enumerate;
mod verbose_decimal_constructor;

View file

@ -0,0 +1,168 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr, ExprCall};
use ruff_python_trivia::PythonWhitespace;
use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;
/// ## What it does
/// Checks for unnecessary string literal or float casts in `Decimal`
/// constructors.
///
/// ## Why is this bad?
/// The `Decimal` constructor accepts a variety of arguments, including
/// integers, floats, and strings. However, it's not necessary to cast
/// integer literals to strings when passing them to the `Decimal`.
///
/// Similarly, `Decimal` accepts `inf`, `-inf`, and `nan` as string literals,
/// so there's no need to wrap those values in a `float` call when passing
/// them to the `Decimal` constructor.
///
/// Prefer the more concise form of argument passing for `Decimal`
/// constructors, as it's more readable and idiomatic.
///
/// ## Example
/// ```python
/// Decimal("0")
/// Decimal(float("Infinity"))
/// ```
///
/// Use instead:
/// ```python
/// Decimal(0)
/// Decimal("Infinity")
/// ```
///
/// ## References
/// - [Python documentation: `decimal`](https://docs.python.org/3/library/decimal.html)
#[violation]
pub struct VerboseDecimalConstructor {
replacement: String,
}
impl Violation for VerboseDecimalConstructor {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Always;
#[derive_message_formats]
fn message(&self) -> String {
format!("Verbose expression in `Decimal` constructor")
}
fn fix_title(&self) -> Option<String> {
let VerboseDecimalConstructor { replacement } = self;
Some(format!("Replace with `{replacement}`"))
}
}
/// FURB157
pub(crate) fn verbose_decimal_constructor(checker: &mut Checker, call: &ExprCall) {
if !checker
.semantic()
.resolve_qualified_name(&call.func)
.is_some_and(|qualified_name| matches!(qualified_name.segments(), ["decimal", "Decimal"]))
{
return;
}
// Decimal accepts arguments of the form: `Decimal(value='0', context=None)`
let Some(value) = call.arguments.find_argument("value", 0) else {
return;
};
let diagnostic = match value {
Expr::StringLiteral(ast::ExprStringLiteral {
value: str_literal, ..
}) => {
// 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 };
let replacement = format!("{unary}{rest}");
let mut diagnostic = Diagnostic::new(
VerboseDecimalConstructor {
replacement: replacement.clone(),
},
value.range(),
);
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
replacement,
value.range(),
)));
diagnostic
}
Expr::Call(ast::ExprCall {
func, arguments, ..
}) => {
// Must be a call to the `float` builtin.
let Some(func_name) = func.as_name_expr() else {
return;
};
if func_name.id != "float" {
return;
};
// Must have exactly one argument, which is a string literal.
if arguments.keywords.len() != 0 {
return;
};
let [float] = arguments.args.as_ref() else {
return;
};
let Some(float) = float.as_string_literal_expr() else {
return;
};
if !matches!(
float.value.to_str().to_lowercase().as_str(),
"inf" | "-inf" | "infinity" | "-infinity" | "nan"
) {
return;
}
if !checker.semantic().is_builtin("float") {
return;
};
let replacement = checker.locator().slice(float).to_string();
let mut diagnostic = Diagnostic::new(
VerboseDecimalConstructor {
replacement: replacement.clone(),
},
value.range(),
);
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
replacement,
value.range(),
)));
diagnostic
}
_ => {
return;
}
};
checker.diagnostics.push(diagnostic);
}

View file

@ -0,0 +1,168 @@
---
source: crates/ruff_linter/src/rules/refurb/mod.rs
---
FURB157.py:5:9: FURB157 [*] Verbose expression in `Decimal` constructor
|
4 | # Errors
5 | Decimal("0")
| ^^^ FURB157
6 | Decimal("-42")
7 | Decimal(float("Infinity"))
|
= help: Replace with `0`
Safe fix
2 2 | from decimal import Decimal
3 3 |
4 4 | # Errors
5 |-Decimal("0")
5 |+Decimal(0)
6 6 | Decimal("-42")
7 7 | Decimal(float("Infinity"))
8 8 | Decimal(float("-Infinity"))
FURB157.py:6:9: FURB157 [*] Verbose expression in `Decimal` constructor
|
4 | # Errors
5 | Decimal("0")
6 | Decimal("-42")
| ^^^^^ FURB157
7 | Decimal(float("Infinity"))
8 | Decimal(float("-Infinity"))
|
= help: Replace with `-42`
Safe fix
3 3 |
4 4 | # Errors
5 5 | Decimal("0")
6 |-Decimal("-42")
6 |+Decimal(-42)
7 7 | Decimal(float("Infinity"))
8 8 | Decimal(float("-Infinity"))
9 9 | Decimal(float("inf"))
FURB157.py:7:9: FURB157 [*] Verbose expression in `Decimal` constructor
|
5 | Decimal("0")
6 | Decimal("-42")
7 | Decimal(float("Infinity"))
| ^^^^^^^^^^^^^^^^^ FURB157
8 | Decimal(float("-Infinity"))
9 | Decimal(float("inf"))
|
= help: Replace with `"Infinity"`
Safe fix
4 4 | # Errors
5 5 | Decimal("0")
6 6 | Decimal("-42")
7 |-Decimal(float("Infinity"))
7 |+Decimal("Infinity")
8 8 | Decimal(float("-Infinity"))
9 9 | Decimal(float("inf"))
10 10 | Decimal(float("-inf"))
FURB157.py:8:9: FURB157 [*] Verbose expression in `Decimal` constructor
|
6 | Decimal("-42")
7 | Decimal(float("Infinity"))
8 | Decimal(float("-Infinity"))
| ^^^^^^^^^^^^^^^^^^ FURB157
9 | Decimal(float("inf"))
10 | Decimal(float("-inf"))
|
= help: Replace with `"-Infinity"`
Safe fix
5 5 | Decimal("0")
6 6 | Decimal("-42")
7 7 | Decimal(float("Infinity"))
8 |-Decimal(float("-Infinity"))
8 |+Decimal("-Infinity")
9 9 | Decimal(float("inf"))
10 10 | Decimal(float("-inf"))
11 11 | Decimal(float("nan"))
FURB157.py:9:9: FURB157 [*] Verbose expression in `Decimal` constructor
|
7 | Decimal(float("Infinity"))
8 | Decimal(float("-Infinity"))
9 | Decimal(float("inf"))
| ^^^^^^^^^^^^ FURB157
10 | Decimal(float("-inf"))
11 | Decimal(float("nan"))
|
= help: Replace with `"inf"`
Safe fix
6 6 | Decimal("-42")
7 7 | Decimal(float("Infinity"))
8 8 | Decimal(float("-Infinity"))
9 |-Decimal(float("inf"))
9 |+Decimal("inf")
10 10 | Decimal(float("-inf"))
11 11 | Decimal(float("nan"))
12 12 | decimal.Decimal("0")
FURB157.py:10:9: FURB157 [*] Verbose expression in `Decimal` constructor
|
8 | Decimal(float("-Infinity"))
9 | Decimal(float("inf"))
10 | Decimal(float("-inf"))
| ^^^^^^^^^^^^^ FURB157
11 | Decimal(float("nan"))
12 | decimal.Decimal("0")
|
= help: Replace with `"-inf"`
Safe fix
7 7 | Decimal(float("Infinity"))
8 8 | Decimal(float("-Infinity"))
9 9 | Decimal(float("inf"))
10 |-Decimal(float("-inf"))
10 |+Decimal("-inf")
11 11 | Decimal(float("nan"))
12 12 | decimal.Decimal("0")
13 13 |
FURB157.py:11:9: FURB157 [*] Verbose expression in `Decimal` constructor
|
9 | Decimal(float("inf"))
10 | Decimal(float("-inf"))
11 | Decimal(float("nan"))
| ^^^^^^^^^^^^ FURB157
12 | decimal.Decimal("0")
|
= help: Replace with `"nan"`
Safe fix
8 8 | Decimal(float("-Infinity"))
9 9 | Decimal(float("inf"))
10 10 | Decimal(float("-inf"))
11 |-Decimal(float("nan"))
11 |+Decimal("nan")
12 12 | decimal.Decimal("0")
13 13 |
14 14 | # OK
FURB157.py:12:17: FURB157 [*] Verbose expression in `Decimal` constructor
|
10 | Decimal(float("-inf"))
11 | Decimal(float("nan"))
12 | decimal.Decimal("0")
| ^^^ FURB157
13 |
14 | # OK
|
= help: Replace with `0`
Safe fix
9 9 | Decimal(float("inf"))
10 10 | Decimal(float("-inf"))
11 11 | Decimal(float("nan"))
12 |-decimal.Decimal("0")
12 |+decimal.Decimal(0)
13 13 |
14 14 | # OK
15 15 | Decimal(0)

1
ruff.schema.json generated
View file

@ -3018,6 +3018,7 @@
"FURB148", "FURB148",
"FURB15", "FURB15",
"FURB152", "FURB152",
"FURB157",
"FURB16", "FURB16",
"FURB161", "FURB161",
"FURB163", "FURB163",