From 0f3c5fe986375a1587bcf6ecc3aa2c2ba2d60af1 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 24 Mar 2024 22:07:21 -0400 Subject: [PATCH] Make safe; remove regex --- .../rules/verbose_decimal_constructor.rs | 117 ++++++++++-------- ...es__refurb__tests__FURB157_FURB157.py.snap | 64 +++++----- 2 files changed, 95 insertions(+), 86 deletions(-) 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 index 5a0600e4f394b4..892d811d8d4bcd 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/verbose_decimal_constructor.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/verbose_decimal_constructor.rs @@ -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}; @@ -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 @@ -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 { @@ -47,10 +50,8 @@ impl Violation for VerboseDecimalConstructor { } fn fix_title(&self) -> Option { - Some(format!( - "Replace {} with {}", - self.replace_old, self.replace_new - )) + let VerboseDecimalConstructor { replacement, .. } = self; + Some(format!("Replace with `{replacement}`")) } } @@ -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 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 index 35ba0f464b8935..ef8680e3666d04 100644 --- 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 @@ -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 @@ -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") @@ -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") @@ -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")) @@ -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")) @@ -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")) @@ -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")) @@ -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"))