Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ruff] Implement check for Decimal called with a float literal (RUF032) #12909

Merged
merged 20 commits into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 120 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF032.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
import decimal

# Tests with fully qualified import
decimal.Decimal(0)

decimal.Decimal(0.0) # Should error

decimal.Decimal("0.0")

decimal.Decimal(10)

decimal.Decimal(10.0) # Should error

decimal.Decimal("10.0")

decimal.Decimal(-10)

decimal.Decimal(-10.0) # Should error

decimal.Decimal("-10.0")

a = 10.0

decimal.Decimal(a)


# Tests with relative import
from decimal import Decimal


val = Decimal(0)

val = Decimal(0.0) # Should error

val = Decimal("0.0")

val = Decimal(10)

val = Decimal(10.0) # Should error

val = Decimal("10.0")

val = Decimal(-10)

val = Decimal(-10.0) # Should error

val = Decimal("-10.0")

a = 10.0

val = Decimal(a)


# Tests with shadowed name
class Decimal():
value: float | int | str

def __init__(self, value: float | int | str) -> None:
self.value = value


val = Decimal(0.0)

val = Decimal("0.0")

val = Decimal(10.0)

val = Decimal("10.0")

val = Decimal(-10.0)

val = Decimal("-10.0")

a = 10.0

val = Decimal(a)


# Retest with fully qualified import

val = decimal.Decimal(0.0) # Should error

val = decimal.Decimal("0.0")

val = decimal.Decimal(10.0) # Should error

val = decimal.Decimal("10.0")

val = decimal.Decimal(-10.0) # Should error

val = decimal.Decimal("-10.0")

a = 10.0

val = decimal.Decimal(a)


class decimal():
class Decimal():
value: float | int | str

def __init__(self, value: float | int | str) -> None:
self.value = value


val = decimal.Decimal(0.0)

val = decimal.Decimal("0.0")

val = decimal.Decimal(10.0)

val = decimal.Decimal("10.0")

val = decimal.Decimal(-10.0)

val = decimal.Decimal("-10.0")

a = 10.0

val = decimal.Decimal(a)
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 @@ -1011,6 +1011,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::UnnecessaryIterableAllocationForFirstElement) {
ruff::rules::unnecessary_iterable_allocation_for_first_element(checker, expr);
}
if checker.enabled(Rule::DecimalFromFloatLiteral) {
ruff::rules::decimal_from_float_literal_syntax(checker, call);
}
if checker.enabled(Rule::IntOnSlicedStr) {
refurb::rules::int_on_sliced_str(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 @@ -958,6 +958,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "029") => (RuleGroup::Preview, rules::ruff::rules::UnusedAsync),
(Ruff, "030") => (RuleGroup::Preview, rules::ruff::rules::AssertWithPrintMessage),
(Ruff, "031") => (RuleGroup::Preview, rules::ruff::rules::IncorrectlyParenthesizedTupleInSubscript),
(Ruff, "032") => (RuleGroup::Preview, rules::ruff::rules::DecimalFromFloatLiteral),
(Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
(Ruff, "101") => (RuleGroup::Stable, rules::ruff::rules::RedirectedNOQA),

Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ mod tests {
#[test_case(Rule::UnusedAsync, Path::new("RUF029.py"))]
#[test_case(Rule::AssertWithPrintMessage, Path::new("RUF030.py"))]
#[test_case(Rule::IncorrectlyParenthesizedTupleInSubscript, Path::new("RUF031.py"))]
#[test_case(Rule::DecimalFromFloatLiteral, Path::new("RUF032.py"))]
#[test_case(Rule::RedirectedNOQA, Path::new("RUF101.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast};
use ruff_text_size::{Ranged, TextRange};

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for `Decimal` calls passing a float literal.
///
/// ## Why is this bad?
/// Float literals have limited precision that can lead to unexpected results. The `Decimal` class is designed to handle
/// numbers with fixed-point precision, so a string literal should be used instead.
///
/// ## Example
///
/// ```python
/// num = Decimal(1.2345)
/// ```
///
/// Use instead:
/// ```python
/// num = Decimal("1.2345")
/// ```
///
/// ## Fix Safety
/// This rule's fix is marked as unsafe because it changes the underlying value of the `Decimal` instance that is
/// constructed. This can lead to unexpected behavior if your program relies on the previous value
/// (whether deliberately or not).
#[violation]
pub struct DecimalFromFloatLiteral;

impl AlwaysFixableViolation for DecimalFromFloatLiteral {
#[derive_message_formats]
fn message(&self) -> String {
format!(r#"`Decimal()` called with float literal argument"#)
}

fn fix_title(&self) -> String {
"Use a string literal instead".into()
}
}

/// RUF032: `Decimal()` called with float literal argument
pub(crate) fn decimal_from_float_literal_syntax(checker: &mut Checker, call: &ast::ExprCall) {
let Some(arg) = call.arguments.args.first() else {
return;
};

if !is_arg_float_literal(arg) {
return;
}

if checker
.semantic()
.resolve_qualified_name(call.func.as_ref())
.is_some_and(|qualified_name| matches!(qualified_name.segments(), ["decimal", "Decimal"]))
{
let diagnostic = Diagnostic::new(DecimalFromFloatLiteral, arg.range()).with_fix(
fix_float_literal(arg.range(), &checker.generator().expr(arg)),
);
checker.diagnostics.push(diagnostic);
}
}

fn is_arg_float_literal(arg: &ast::Expr) -> bool {
match arg {
ast::Expr::NumberLiteral(ast::ExprNumberLiteral {
value: ast::Number::Float(_),
..
}) => true,
ast::Expr::UnaryOp(ast::ExprUnaryOp { operand, .. }) => {
matches!(
operand.as_ref(),
ast::Expr::NumberLiteral(ast::ExprNumberLiteral {
value: ast::Number::Float(_),
..
})
)
}
_ => false,
}
}

fn fix_float_literal(range: TextRange, float_literal: &str) -> Fix {
let content = format!("\"{float_literal}\"");
Fix::unsafe_edit(Edit::range_replacement(content, range))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thing. Here we should use the quote style that the user is generally using for the rest of their file, which won't necessarily be double quotes if they're not using autoformatting. We can do this with the checker.stylist():

Suggested change
fn fix_float_literal(range: TextRange, float_literal: &str) -> Fix {
let content = format!("\"{float_literal}\"");
Fix::unsafe_edit(Edit::range_replacement(content, range))
fn fix_float_literal(range: TextRange, float_literal: &str, stylist: &Stylist) -> Fix {
let quote = stylist.quote();
let content = format!("{quote}{float_literal}{quote}");
Fix::unsafe_edit(Edit::range_replacement(content, range))

You'll just want to pass checker.stylist() into this function at the callsite, and you'll want to add use ruff_python_codegen::Stylist; to the top of the file so that you can use it here in the type signature

}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ pub(crate) use assert_with_print_message::*;
pub(crate) use assignment_in_assert::*;
pub(crate) use asyncio_dangling_task::*;
pub(crate) use collection_literal_concatenation::*;
pub(crate) use decimal_from_float_literal::*;
pub(crate) use default_factory_kwarg::*;
pub(crate) use explicit_f_string_type_conversion::*;
pub(crate) use function_call_in_dataclass_default::*;
Expand Down Expand Up @@ -36,6 +37,7 @@ mod assignment_in_assert;
mod asyncio_dangling_task;
mod collection_literal_concatenation;
mod confusables;
mod decimal_from_float_literal;
mod default_factory_kwarg;
mod explicit_f_string_type_conversion;
mod function_call_in_dataclass_default;
Expand Down
Loading
Loading