diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB157.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB157.py new file mode 100644 index 0000000000000..2d4b3aa089c02 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB157.py @@ -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) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 785ffd34a6d98..e2d9f94a63a6b 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -926,6 +926,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::RedundantLogBase) { refurb::rules::redundant_log_base(checker, call); } + if checker.enabled(Rule::VerboseDecimalConstructor) { + refurb::rules::verbose_decimal_constructor(checker, call); + } if checker.enabled(Rule::QuadraticListSummation) { ruff::rules::quadratic_list_summation(checker, call); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index f4b0a97b29b0e..efbf51abf39d0 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -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, "148") => (RuleGroup::Preview, rules::refurb::rules::UnnecessaryEnumerate), (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, "163") => (RuleGroup::Preview, rules::refurb::rules::RedundantLogBase), (Refurb, "167") => (RuleGroup::Preview, rules::refurb::rules::RegexFlagAlias), diff --git a/crates/ruff_linter/src/rules/refurb/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index 91af7e47d2060..f67ddee937a01 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/mod.rs @@ -25,6 +25,7 @@ mod tests { #[test_case(Rule::SliceCopy, Path::new("FURB145.py"))] #[test_case(Rule::UnnecessaryEnumerate, Path::new("FURB148.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::ImplicitCwd, Path::new("FURB177.py"))] #[test_case(Rule::SingleItemMembershipTest, Path::new("FURB171.py"))] diff --git a/crates/ruff_linter/src/rules/refurb/rules/mod.rs b/crates/ruff_linter/src/rules/refurb/rules/mod.rs index 97bc141fe0d46..a395e6065a71a 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/mod.rs @@ -20,6 +20,7 @@ pub(crate) use single_item_membership_test::*; pub(crate) use slice_copy::*; pub(crate) use type_none_comparison::*; pub(crate) use unnecessary_enumerate::*; +pub(crate) use verbose_decimal_constructor::*; mod bit_count; mod check_and_remove_from_set; @@ -43,3 +44,4 @@ mod single_item_membership_test; mod slice_copy; mod type_none_comparison; mod unnecessary_enumerate; +mod verbose_decimal_constructor; 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 new file mode 100644 index 0000000000000..154c0405c41fb --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/verbose_decimal_constructor.rs @@ -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 { + 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); +} 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 new file mode 100644 index 0000000000000..ef8680e3666d0 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB157_FURB157.py.snap @@ -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) diff --git a/ruff.schema.json b/ruff.schema.json index 0d5ae5a283eae..d57769f325477 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3018,6 +3018,7 @@ "FURB148", "FURB15", "FURB152", + "FURB157", "FURB16", "FURB161", "FURB163",