From 2faac1e7a866e77b60c4a7abe71b7b8985efa660 Mon Sep 17 00:00:00 2001 From: Tuomas Siipola Date: Fri, 17 Nov 2023 19:37:44 +0200 Subject: [PATCH] [`refurb`] Implement `math-constant` (`FURB152`) (#8727) ## Summary Implements [FURB152](https://github.com/dosisod/refurb/blob/master/docs/checks.md#furb152-use-math-constant) that checks for literals that are similar to constants in `math` module, for example: ```python A = 3.141592 * r ** 2 ``` Use instead: ```python A = math.pi * r ** 2 ``` Related to #1348. --- .../resources/test/fixtures/refurb/FURB152.py | 7 ++ .../src/checkers/ast/analyze/expression.rs | 5 +- crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/refurb/mod.rs | 1 + .../src/rules/refurb/rules/math_constant.rs | 90 +++++++++++++++++++ .../ruff_linter/src/rules/refurb/rules/mod.rs | 2 + ...es__refurb__tests__FURB152_FURB152.py.snap | 67 ++++++++++++++ crates/ruff_workspace/src/configuration.rs | 1 + ruff.schema.json | 2 + 9 files changed, 175 insertions(+), 1 deletion(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/refurb/FURB152.py create mode 100644 crates/ruff_linter/src/rules/refurb/rules/math_constant.rs create mode 100644 crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB152_FURB152.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB152.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB152.py new file mode 100644 index 0000000000000..5e1bfbb16640d --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB152.py @@ -0,0 +1,7 @@ +r = 3.1 # OK + +A = 3.14 * r ** 2 # FURB152 + +C = 6.28 * r # FURB152 + +e = 2.71 # FURB152 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 5e26059969650..9e599ccca9cd8 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1245,10 +1245,13 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { refurb::rules::single_item_membership_test(checker, expr, left, ops, comparators); } } - Expr::NumberLiteral(_) => { + Expr::NumberLiteral(number_literal @ ast::ExprNumberLiteral { .. }) => { if checker.source_type.is_stub() && checker.enabled(Rule::NumericLiteralTooLong) { flake8_pyi::rules::numeric_literal_too_long(checker, expr); } + if checker.enabled(Rule::MathConstant) { + refurb::rules::math_constant(checker, number_literal); + } } Expr::BytesLiteral(_) => { if checker.source_type.is_stub() && checker.enabled(Rule::StringOrBytesTooLong) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 9c4c295fb182d..b6d26476b3442 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -951,6 +951,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Refurb, "140") => (RuleGroup::Preview, rules::refurb::rules::ReimplementedStarmap), (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, "168") => (RuleGroup::Preview, rules::refurb::rules::IsinstanceTypeNone), (Refurb, "169") => (RuleGroup::Preview, rules::refurb::rules::TypeNoneComparison), (Refurb, "171") => (RuleGroup::Preview, rules::refurb::rules::SingleItemMembershipTest), diff --git a/crates/ruff_linter/src/rules/refurb/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index 91cf5ce41fbd5..49a2c49bfe6c2 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/mod.rs @@ -22,6 +22,7 @@ mod tests { #[test_case(Rule::ReimplementedStarmap, Path::new("FURB140.py"))] #[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::PrintEmptyString, Path::new("FURB105.py"))] #[test_case(Rule::ImplicitCwd, Path::new("FURB177.py"))] #[test_case(Rule::SingleItemMembershipTest, Path::new("FURB171.py"))] diff --git a/crates/ruff_linter/src/rules/refurb/rules/math_constant.rs b/crates/ruff_linter/src/rules/refurb/rules/math_constant.rs new file mode 100644 index 0000000000000..6b590275a38ba --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/math_constant.rs @@ -0,0 +1,90 @@ +use anyhow::Result; + +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, Number}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; +use crate::importer::ImportRequest; + +/// ## What it does +/// Checks for literals that are similar to constants in `math` module. +/// +/// ## Why is this bad? +/// Hard-coding mathematical constants like π increases code duplication, +/// reduces readability, and may lead to a lack of precision. +/// +/// ## Example +/// ```python +/// A = 3.141592 * r**2 +/// ``` +/// +/// Use instead: +/// ```python +/// A = math.pi * r**2 +/// ``` +/// +/// ## References +/// - [Python documentation: `math` constants](https://docs.python.org/3/library/math.html#constants) +#[violation] +pub struct MathConstant { + literal: String, + constant: &'static str, +} + +impl Violation for MathConstant { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + let MathConstant { literal, constant } = self; + format!("Replace `{literal}` with `math.{constant}`") + } + + fn fix_title(&self) -> Option { + let MathConstant { constant, .. } = self; + Some(format!("Use `math.{constant}`")) + } +} + +/// FURB152 +pub(crate) fn math_constant(checker: &mut Checker, literal: &ast::ExprNumberLiteral) { + let Number::Float(value) = literal.value else { + return; + }; + for (real_value, constant) in [ + (std::f64::consts::PI, "pi"), + (std::f64::consts::E, "e"), + (std::f64::consts::TAU, "tau"), + ] { + if (value - real_value).abs() < 1e-2 { + let mut diagnostic = Diagnostic::new( + MathConstant { + literal: checker.locator().slice(literal).into(), + constant, + }, + literal.range(), + ); + diagnostic.try_set_fix(|| convert_to_constant(literal, constant, checker)); + checker.diagnostics.push(diagnostic); + return; + } + } +} + +fn convert_to_constant( + literal: &ast::ExprNumberLiteral, + constant: &'static str, + checker: &Checker, +) -> Result { + let (edit, binding) = checker.importer().get_or_import_symbol( + &ImportRequest::import("math", constant), + literal.start(), + checker.semantic(), + )?; + Ok(Fix::safe_edits( + Edit::range_replacement(binding, literal.range()), + [edit], + )) +} diff --git a/crates/ruff_linter/src/rules/refurb/rules/mod.rs b/crates/ruff_linter/src/rules/refurb/rules/mod.rs index 014d9a632fde1..6729202272905 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/mod.rs @@ -3,6 +3,7 @@ pub(crate) use delete_full_slice::*; pub(crate) use if_expr_min_max::*; pub(crate) use implicit_cwd::*; pub(crate) use isinstance_type_none::*; +pub(crate) use math_constant::*; pub(crate) use print_empty_string::*; pub(crate) use read_whole_file::*; pub(crate) use reimplemented_starmap::*; @@ -17,6 +18,7 @@ mod delete_full_slice; mod if_expr_min_max; mod implicit_cwd; mod isinstance_type_none; +mod math_constant; mod print_empty_string; mod read_whole_file; mod reimplemented_starmap; diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB152_FURB152.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB152_FURB152.py.snap new file mode 100644 index 0000000000000..aa97aead864cb --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB152_FURB152.py.snap @@ -0,0 +1,67 @@ +--- +source: crates/ruff_linter/src/rules/refurb/mod.rs +--- +FURB152.py:3:5: FURB152 [*] Replace `3.14` with `math.pi` + | +1 | r = 3.1 # OK +2 | +3 | A = 3.14 * r ** 2 # FURB152 + | ^^^^ FURB152 +4 | +5 | C = 6.28 * r # FURB152 + | + = help: Use `math.pi` + +ℹ Safe fix + 1 |+import math +1 2 | r = 3.1 # OK +2 3 | +3 |-A = 3.14 * r ** 2 # FURB152 + 4 |+A = math.pi * r ** 2 # FURB152 +4 5 | +5 6 | C = 6.28 * r # FURB152 +6 7 | + +FURB152.py:5:5: FURB152 [*] Replace `6.28` with `math.tau` + | +3 | A = 3.14 * r ** 2 # FURB152 +4 | +5 | C = 6.28 * r # FURB152 + | ^^^^ FURB152 +6 | +7 | e = 2.71 # FURB152 + | + = help: Use `math.tau` + +ℹ Safe fix + 1 |+import math +1 2 | r = 3.1 # OK +2 3 | +3 4 | A = 3.14 * r ** 2 # FURB152 +4 5 | +5 |-C = 6.28 * r # FURB152 + 6 |+C = math.tau * r # FURB152 +6 7 | +7 8 | e = 2.71 # FURB152 + +FURB152.py:7:5: FURB152 [*] Replace `2.71` with `math.e` + | +5 | C = 6.28 * r # FURB152 +6 | +7 | e = 2.71 # FURB152 + | ^^^^ FURB152 + | + = help: Use `math.e` + +ℹ Safe fix + 1 |+import math +1 2 | r = 3.1 # OK +2 3 | +3 4 | A = 3.14 * r ** 2 # FURB152 +4 5 | +5 6 | C = 6.28 * r # FURB152 +6 7 | +7 |-e = 2.71 # FURB152 + 8 |+e = math.e # FURB152 + + diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index 77d9c3af19536..4c38249b0e5dc 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -1159,6 +1159,7 @@ mod tests { Rule::TooManyPublicMethods, Rule::UndocumentedWarn, Rule::UnnecessaryEnumerate, + Rule::MathConstant, ]; #[allow(clippy::needless_pass_by_value)] diff --git a/ruff.schema.json b/ruff.schema.json index 22756f2dc68d1..9ede2c3f8a793 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2828,6 +2828,8 @@ "FURB140", "FURB145", "FURB148", + "FURB15", + "FURB152", "FURB16", "FURB168", "FURB169",