Skip to content

Commit

Permalink
Make safe; remove regex
Browse files Browse the repository at this point in the history
  • Loading branch information
@charliermarsh
charliermarsh committed Mar 25, 2024
1 parent 9517e9a commit 0f3c5fe
Show file tree
Hide file tree
Showing 2 changed files with 95 additions and 86 deletions.
117 changes: 63 additions & 54 deletions crates/ruff_linter/src/rules/refurb/rules/verbose_decimal_constructor.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use regex::Regex;
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr, ExprCall};
Expand All @@ -8,11 +7,20 @@ use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for the use of `Decimal` constructor that can be made more succinct.
/// This includes unnecessary string literal or special literal of float.
/// Checks for unnecessary string literal or float casts in `Decimal`
/// constructors.
///
/// ## Why is this bad?
/// This will make code longer and harder to read.
/// 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
Expand All @@ -26,16 +34,11 @@ use crate::checkers::ast::Checker;
/// Decimal("Infinity")
/// ```
///
/// ## Fix safety
/// This rule's fix is marked as unsafe, as the `Decimal` could be user-defined
/// function or constructor, which is not intended for the fixtures like above.
///
/// ## References
/// - [Python documentation: `decimal`](https://docs.python.org/3/library/decimal.html)
#[violation]
pub struct VerboseDecimalConstructor {
replace_old: String,
replace_new: String,
replacement: String,
}

impl Violation for VerboseDecimalConstructor {
Expand All @@ -47,10 +50,8 @@ impl Violation for VerboseDecimalConstructor {
}

fn fix_title(&self) -> Option<String> {
Some(format!(
"Replace {} with {}",
self.replace_old, self.replace_new
))
let VerboseDecimalConstructor { replacement, .. } = self;
Some(format!("Replace with `{replacement}`"))
}
}

Expand All @@ -63,89 +64,97 @@ pub(crate) fn verbose_decimal_constructor(checker: &mut Checker, call: &ExprCall
{
return;
}
let ast::Arguments { args, keywords, .. } = &call.arguments;
// Decimal accepts arguments of the form Decimal(value='0', context=None).
let Some(value) = args.first().or_else(|| {
keywords
.iter()
.find(|&keyword| keyword.arg.as_ref().map(ast::Identifier::as_str) == Some("value"))
.map(|keyword| &keyword.value)
}) else {

// Decimal accepts arguments of the form: `Decimal(value='0', context=None)`
let Some(value) = call.arguments.find_argument("value", 0) else {
return;
};

let decimal_constructor = checker.locator().slice(call.func.range());

let diagnostic = match value {
Expr::StringLiteral(ast::ExprStringLiteral {
value: str_literal, ..
}) => {
let trimmed = str_literal
.to_str()
.trim_whitespace()
.trim_start_matches('+');
let integer_string = Regex::new(r"^([+\-]?)0*(\d+)$").unwrap();
if !integer_string.is_match(trimmed) {
// 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 trimmed.starts_with('+') {
("+", &trimmed[1..])
} else if trimmed.starts_with('-') {
("-", &trimmed[1..])
} 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_digit(10)) {
return;
};

let intg = integer_string.replace(trimmed, "$1$2").into_owned();
// 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 {
replace_old: format!("{}(\"{}\")", decimal_constructor, str_literal.to_str()),
replace_new: format!("{decimal_constructor}({intg})"),
replacement: replacement.clone(),
},
call.range(),
value.range(),
);

diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
format!("{decimal_constructor}({intg})"),
call.range(),
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
replacement,
value.range(),
)));

diagnostic
}
Expr::Call(
floatcall @ ast::ExprCall {
func, arguments, ..
},
) => {
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;
};
if !checker.semantic().is_builtin(&func_name.id) {

// Must have exactly one argument, which is a string literal.
if arguments.keywords.len() != 0 {
return;
};

if arguments.args.len() != 1 || arguments.keywords.len() > 0 {
let [float] = arguments.args.as_ref() else {
return;
};
let Some(value_float) = arguments.args[0].as_string_literal_expr() else {
let Some(float) = float.as_string_literal_expr() else {
return;
};
let value_float_str = value_float.value.to_str();
if !matches!(
value_float_str.to_lowercase().as_str(),
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 {
replace_old: format!("float(\"{value_float_str}\")"),
replace_new: format!("\"{value_float_str}\""),
replacement: replacement.clone(),
},
call.range(),
value.range(),
);

diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
format!("\"{value_float_str}\""),
floatcall.range(),
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
replacement,
value.range(),
)));

diagnostic
Expand Down
64 changes: 32 additions & 32 deletions ...ter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB157_FURB157.py.snap
View file
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
---
source: crates/ruff_linter/src/rules/refurb/mod.rs
---
FURB157.py:5:1: FURB157 [*] Verbose expression in `Decimal` constructor
FURB157.py:5:9: FURB157 [*] Verbose expression in `Decimal` constructor
|
4 | # Errors
5 | Decimal("0")
| ^^^^^^^^^^^^ FURB157
| ^^^ FURB157
6 | Decimal("-42")
7 | Decimal(float("Infinity"))
|
= help: Replace Decimal("0") with Decimal(0)
= help: Replace with `0`

ℹ Unsafe fix
ℹ Safe fix
2 2 | from decimal import Decimal
3 3 |
4 4 | # Errors
Expand All @@ -21,18 +21,18 @@ FURB157.py:5:1: FURB157 [*] Verbose expression in `Decimal` constructor
7 7 | Decimal(float("Infinity"))
8 8 | Decimal(float("-Infinity"))

FURB157.py:6:1: FURB157 [*] Verbose expression in `Decimal` constructor
FURB157.py:6:9: FURB157 [*] Verbose expression in `Decimal` constructor
|
4 | # Errors
5 | Decimal("0")
6 | Decimal("-42")
| ^^^^^^^^^^^^^^ FURB157
| ^^^^^ FURB157
7 | Decimal(float("Infinity"))
8 | Decimal(float("-Infinity"))
|
= help: Replace Decimal("-42") with Decimal(-42)
= help: Replace with `-42`

ℹ Unsafe fix
ℹ Safe fix
3 3 |
4 4 | # Errors
5 5 | Decimal("0")
Expand All @@ -42,18 +42,18 @@ FURB157.py:6:1: FURB157 [*] Verbose expression in `Decimal` constructor
8 8 | Decimal(float("-Infinity"))
9 9 | Decimal(float("inf"))

FURB157.py:7:1: FURB157 [*] Verbose expression in `Decimal` constructor
FURB157.py:7:9: FURB157 [*] Verbose expression in `Decimal` constructor
|
5 | Decimal("0")
6 | Decimal("-42")
7 | Decimal(float("Infinity"))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB157
| ^^^^^^^^^^^^^^^^^ FURB157
8 | Decimal(float("-Infinity"))
9 | Decimal(float("inf"))
|
= help: Replace float("Infinity") with "Infinity"
= help: Replace with `"Infinity"`

ℹ Unsafe fix
ℹ Safe fix
4 4 | # Errors
5 5 | Decimal("0")
6 6 | Decimal("-42")
Expand All @@ -63,18 +63,18 @@ FURB157.py:7:1: FURB157 [*] Verbose expression in `Decimal` constructor
9 9 | Decimal(float("inf"))
10 10 | Decimal(float("-inf"))

FURB157.py:8:1: FURB157 [*] Verbose expression in `Decimal` constructor
FURB157.py:8:9: FURB157 [*] Verbose expression in `Decimal` constructor
|
6 | Decimal("-42")
7 | Decimal(float("Infinity"))
8 | Decimal(float("-Infinity"))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB157
| ^^^^^^^^^^^^^^^^^^ FURB157
9 | Decimal(float("inf"))
10 | Decimal(float("-inf"))
|
= help: Replace float("-Infinity") with "-Infinity"
= help: Replace with `"-Infinity"`

ℹ Unsafe fix
ℹ Safe fix
5 5 | Decimal("0")
6 6 | Decimal("-42")
7 7 | Decimal(float("Infinity"))
Expand All @@ -84,18 +84,18 @@ FURB157.py:8:1: FURB157 [*] Verbose expression in `Decimal` constructor
10 10 | Decimal(float("-inf"))
11 11 | Decimal(float("nan"))

FURB157.py:9:1: FURB157 [*] Verbose expression in `Decimal` constructor
FURB157.py:9:9: FURB157 [*] Verbose expression in `Decimal` constructor
|
7 | Decimal(float("Infinity"))
8 | Decimal(float("-Infinity"))
9 | Decimal(float("inf"))
| ^^^^^^^^^^^^^^^^^^^^^ FURB157
| ^^^^^^^^^^^^ FURB157
10 | Decimal(float("-inf"))
11 | Decimal(float("nan"))
|
= help: Replace float("inf") with "inf"
= help: Replace with `"inf"`

ℹ Unsafe fix
ℹ Safe fix
6 6 | Decimal("-42")
7 7 | Decimal(float("Infinity"))
8 8 | Decimal(float("-Infinity"))
Expand All @@ -105,18 +105,18 @@ FURB157.py:9:1: FURB157 [*] Verbose expression in `Decimal` constructor
11 11 | Decimal(float("nan"))
12 12 | decimal.Decimal("0")

FURB157.py:10:1: FURB157 [*] Verbose expression in `Decimal` constructor
FURB157.py:10:9: FURB157 [*] Verbose expression in `Decimal` constructor
|
8 | Decimal(float("-Infinity"))
9 | Decimal(float("inf"))
10 | Decimal(float("-inf"))
| ^^^^^^^^^^^^^^^^^^^^^^ FURB157
| ^^^^^^^^^^^^^ FURB157
11 | Decimal(float("nan"))
12 | decimal.Decimal("0")
|
= help: Replace float("-inf") with "-inf"
= help: Replace with `"-inf"`

ℹ Unsafe fix
ℹ Safe fix
7 7 | Decimal(float("Infinity"))
8 8 | Decimal(float("-Infinity"))
9 9 | Decimal(float("inf"))
Expand All @@ -126,17 +126,17 @@ FURB157.py:10:1: FURB157 [*] Verbose expression in `Decimal` constructor
12 12 | decimal.Decimal("0")
13 13 |

FURB157.py:11:1: FURB157 [*] Verbose expression in `Decimal` constructor
FURB157.py:11:9: FURB157 [*] Verbose expression in `Decimal` constructor
|
9 | Decimal(float("inf"))
10 | Decimal(float("-inf"))
11 | Decimal(float("nan"))
| ^^^^^^^^^^^^^^^^^^^^^ FURB157
| ^^^^^^^^^^^^ FURB157
12 | decimal.Decimal("0")
|
= help: Replace float("nan") with "nan"
= help: Replace with `"nan"`

ℹ Unsafe fix
ℹ Safe fix
8 8 | Decimal(float("-Infinity"))
9 9 | Decimal(float("inf"))
10 10 | Decimal(float("-inf"))
Expand All @@ -146,18 +146,18 @@ FURB157.py:11:1: FURB157 [*] Verbose expression in `Decimal` constructor
13 13 |
14 14 | # OK

FURB157.py:12:1: FURB157 [*] Verbose expression in `Decimal` constructor
FURB157.py:12:17: FURB157 [*] Verbose expression in `Decimal` constructor
|
10 | Decimal(float("-inf"))
11 | Decimal(float("nan"))
12 | decimal.Decimal("0")
| ^^^^^^^^^^^^^^^^^^^^ FURB157
| ^^^ FURB157
13 |
14 | # OK
|
= help: Replace decimal.Decimal("0") with decimal.Decimal(0)
= help: Replace with `0`

ℹ Unsafe fix
ℹ Safe fix
9 9 | Decimal(float("inf"))
10 10 | Decimal(float("-inf"))
11 11 | Decimal(float("nan"))
Expand Down

0 comments on commit 0f3c5fe

Please sign in to comment.