Skip to content

Commit

Permalink
[refurb] Implement verbose-decimal-constructor (FURB157) (#10533)
Browse files Browse the repository at this point in the history
## 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`.
  • Loading branch information
boolean-light authored Mar 25, 2024
1 parent 22f237f commit 39fb6d9
Show file tree
Hide file tree
Showing 8 changed files with 361 additions and 0 deletions.
17 changes: 17 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB157.py
Original file line number Diff line number Diff line change
@@ -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)
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/refurb/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"))]
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -43,3 +44,4 @@ mod single_item_membership_test;
mod slice_copy;
mod type_none_comparison;
mod unnecessary_enumerate;
mod verbose_decimal_constructor;
Original file line number Diff line number Diff line change
@@ -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);
}
Original file line number Diff line number Diff line change
@@ -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)
Loading

0 comments on commit 39fb6d9

Please sign in to comment.