Skip to content

Commit

Permalink
[refurb] Implement unnecessary-from-float (FURB164) (#10647)
Browse files Browse the repository at this point in the history
## Summary

<!-- What's the purpose of the change? What does it do, and why? -->

Implement FURB164 in the issue #1348.
Relevant Refurb docs is here:
https://github.com/dosisod/refurb/blob/v2.0.0/docs/checks.md#furb164-no-from-float

I've changed the name from `no-from-float` to
`verbose-decimal-fraction-construction`.

## Test Plan

<!-- How was it tested? -->

I've written it in the `FURB164.py`.

---------

Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
  • Loading branch information
boolean-light and charliermarsh committed Mar 30, 2024
1 parent 75e0142 commit 9ad9cea
Show file tree
Hide file tree
Showing 9 changed files with 606 additions and 2 deletions.
34 changes: 34 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB164.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
from decimal import Decimal
from fractions import Fraction
import decimal
import fractions

# Errors
_ = Fraction.from_float(0.1)
_ = Fraction.from_float(-0.5)
_ = Fraction.from_float(5.0)
_ = fractions.Fraction.from_float(4.2)
_ = Fraction.from_decimal(Decimal("4.2"))
_ = Fraction.from_decimal(Decimal("-4.2"))
_ = Fraction.from_decimal(Decimal.from_float(4.2))
_ = Decimal.from_float(0.1)
_ = Decimal.from_float(-0.5)
_ = Decimal.from_float(5.0)
_ = decimal.Decimal.from_float(4.2)
_ = Decimal.from_float(float("inf"))
_ = Decimal.from_float(float("-inf"))
_ = Decimal.from_float(float("Infinity"))
_ = Decimal.from_float(float("-Infinity"))
_ = Decimal.from_float(float("nan"))

# OK
_ = Fraction(0.1)
_ = Fraction(-0.5)
_ = Fraction(5.0)
_ = fractions.Fraction(4.2)
_ = Fraction(Decimal("4.2"))
_ = Fraction(Decimal("-4.2"))
_ = Decimal(0.1)
_ = Decimal(-0.5)
_ = Decimal(5.0)
_ = decimal.Decimal(4.2)
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 @@ -929,6 +929,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::VerboseDecimalConstructor) {
refurb::rules::verbose_decimal_constructor(checker, call);
}
if checker.enabled(Rule::UnnecessaryFromFloat) {
refurb::rules::unnecessary_from_float(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 @@ -1051,6 +1051,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(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, "164") => (RuleGroup::Preview, rules::refurb::rules::UnnecessaryFromFloat),
(Refurb, "167") => (RuleGroup::Preview, rules::refurb::rules::RegexFlagAlias),
(Refurb, "168") => (RuleGroup::Preview, rules::refurb::rules::IsinstanceTypeNone),
(Refurb, "169") => (RuleGroup::Preview, rules::refurb::rules::TypeNoneComparison),
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 @@ -27,6 +27,7 @@ mod tests {
#[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::UnnecessaryFromFloat, Path::new("FURB164.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 @@ -21,6 +21,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 unnecessary_from_float::*;
pub(crate) use verbose_decimal_constructor::*;

mod bit_count;
Expand All @@ -46,4 +47,5 @@ mod single_item_membership_test;
mod slice_copy;
mod type_none_comparison;
mod unnecessary_enumerate;
mod unnecessary_from_float;
mod verbose_decimal_constructor;
205 changes: 205 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/unnecessary_from_float.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,205 @@
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_text_size::Ranged;

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

/// ## What it does
/// Checks for unnecessary `from_float` and `from_decimal` usages to construct
/// `Decimal` and `Fraction` instances.
///
/// ## Why is this bad?
/// Since Python 3.2, the `Fraction` and `Decimal` classes can be constructed
/// by passing float or decimal instances to the constructor directly. As such,
/// the use of `from_float` and `from_decimal` methods is unnecessary, and
/// should be avoided in favor of the more concise constructor syntax.
///
/// ## Examples
/// ```python
/// Decimal.from_float(4.2)
/// Decimal.from_float(float("inf"))
/// Fraction.from_float(4.2)
/// Fraction.from_decimal(Decimal("4.2"))
/// ```
///
/// Use instead:
/// ```python
/// Decimal(4.2)
/// Decimal("inf")
/// Fraction(4.2)
/// Fraction(Decimal(4.2))
/// ```
///
/// ## References
/// - [Python documentation: `decimal`](https://docs.python.org/3/library/decimal.html)
/// - [Python documentation: `fractions`](https://docs.python.org/3/library/fractions.html)
#[violation]
pub struct UnnecessaryFromFloat {
method_name: MethodName,
constructor: Constructor,
}

impl Violation for UnnecessaryFromFloat {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
let UnnecessaryFromFloat {
method_name,
constructor,
} = self;
format!("Verbose method `{method_name}` in `{constructor}` construction",)
}

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

/// FURB164
pub(crate) fn unnecessary_from_float(checker: &mut Checker, call: &ExprCall) {
let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = &*call.func else {
return;
};

// The method name must be either `from_float` or `from_decimal`.
let method_name = match attr.as_str() {
"from_float" => MethodName::FromFloat,
"from_decimal" => MethodName::FromDecimal,
_ => return,
};

// The value must be either `decimal.Decimal` or `fractions.Fraction`.
let Some(constructor) =
checker
.semantic()
.resolve_qualified_name(value)
.and_then(|qualified_name| match qualified_name.segments() {
["decimal", "Decimal"] => Some(Constructor::Decimal),
["fractions", "Fraction"] => Some(Constructor::Fraction),
_ => None,
})
else {
return;
};

// `Decimal.from_decimal` doesn't exist.
if matches!(
(method_name, constructor),
(MethodName::FromDecimal, Constructor::Decimal)
) {
return;
}

let mut diagnostic = Diagnostic::new(
UnnecessaryFromFloat {
method_name,
constructor,
},
call.range(),
);

let edit = Edit::range_replacement(
checker.locator().slice(&**value).to_string(),
call.func.range(),
);

// Short-circuit case for special values, such as: `Decimal.from_float(float("inf"))` to `Decimal("inf")`.
'short_circuit: {
if !matches!(constructor, Constructor::Decimal) {
break 'short_circuit;
};
if !(method_name == MethodName::FromFloat) {
break 'short_circuit;
};

let Some(value) = (match method_name {
MethodName::FromFloat => call.arguments.find_argument("f", 0),
MethodName::FromDecimal => call.arguments.find_argument("dec", 0),
}) else {
return;
};

let Expr::Call(
call @ ast::ExprCall {
func, arguments, ..
},
) = value
else {
break 'short_circuit;
};

// Must be a call to the `float` builtin.
let Some(func_name) = func.as_name_expr() else {
break 'short_circuit;
};
if func_name.id != "float" {
break 'short_circuit;
};

// Must have exactly one argument, which is a string literal.
if arguments.keywords.len() != 0 {
break 'short_circuit;
};
let [float] = arguments.args.as_ref() else {
break 'short_circuit;
};
let Some(float) = float.as_string_literal_expr() else {
break 'short_circuit;
};
if !matches!(
float.value.to_str().to_lowercase().as_str(),
"inf" | "-inf" | "infinity" | "-infinity" | "nan"
) {
break 'short_circuit;
}

if !checker.semantic().is_builtin("float") {
break 'short_circuit;
};

let replacement = checker.locator().slice(float).to_string();
diagnostic.set_fix(Fix::safe_edits(
edit,
[Edit::range_replacement(replacement, call.range())],
));
checker.diagnostics.push(diagnostic);

return;
}

diagnostic.set_fix(Fix::safe_edit(edit));
checker.diagnostics.push(diagnostic);
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum MethodName {
FromFloat,
FromDecimal,
}

impl std::fmt::Display for MethodName {
fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
match self {
MethodName::FromFloat => fmt.write_str("from_float"),
MethodName::FromDecimal => fmt.write_str("from_decimal"),
}
}
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum Constructor {
Decimal,
Fraction,
}

impl std::fmt::Display for Constructor {
fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
match self {
Constructor::Decimal => fmt.write_str("Decimal"),
Constructor::Fraction => fmt.write_str("Fraction"),
}
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
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_ast::{self as ast, Expr};
use ruff_python_trivia::PythonWhitespace;
use ruff_text_size::Ranged;

Expand Down Expand Up @@ -56,7 +56,7 @@ impl Violation for VerboseDecimalConstructor {
}

/// FURB157
pub(crate) fn verbose_decimal_constructor(checker: &mut Checker, call: &ExprCall) {
pub(crate) fn verbose_decimal_constructor(checker: &mut Checker, call: &ast::ExprCall) {
if !checker
.semantic()
.resolve_qualified_name(&call.func)
Expand Down

0 comments on commit 9ad9cea

Please sign in to comment.