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

[refurb] Implement math-constant (FURB152) #8727

Merged
merged 4 commits into from
Nov 17, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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
7 changes: 7 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB152.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
r = 3.1 # OK

A = 3.14 * r ** 2 # FURB152

C = 6.28 * r # FURB152

e = 2.71 # FURB152
5 changes: 4 additions & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
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 @@ -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),
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 @@ -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"))]
Expand Down
88 changes: 88 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/math_constant.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
use crate::checkers::ast::Checker;
use crate::importer::ImportRequest;
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;

/// ## 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 lack 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<String> {
let MathConstant { constant, .. } = self;
Some(format!("Use `math.{constant}`"))
}
}

fn convert_to_constant(
literal: &ast::ExprNumberLiteral,
constant: &'static str,
checker: &Checker,
) -> Result<Fix> {
let (edit, binding) = checker.importer().get_or_import_symbol(
&ImportRequest::import("math", constant),
literal.start(),
checker.semantic(),
)?;
Ok(Fix::unsafe_edits(
Edit::range_replacement(binding, literal.range()),
[edit],
))
Copy link
Member

Choose a reason for hiding this comment

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

Why is this an unsafe fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most likely it changes result of calculations. For example, math.pi is more precise than 3.14.

Copy link
Member

@zanieb zanieb Nov 16, 2023

Choose a reason for hiding this comment

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

I'm unsure if that means we need to mark it as unsafe, I could go either way though. @charliermarsh?

If we do leave it as unsafe, we should explain why in the rule docs.

Copy link
Member

Choose a reason for hiding this comment

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

Hah, this is an interesting case. I guess I'd lean towards unsafe since it could break tests, etc., if you had hard-coded expectations that differed slightly?

Copy link
Member

Choose a reason for hiding this comment

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

I guess but tests written to check floating point math already should include tolerances. Idk I'm struggling to think of a case where this would change runtime behavior in a meaningful way.

Copy link
Member

Choose a reason for hiding this comment

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

Since it's in preview, it may be fine to just use safe and see if we get feedback.

}

/// 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;
}
}
}
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 @@ -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::*;
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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`

ℹ Unsafe 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`

ℹ Unsafe 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`

ℹ Unsafe 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


1 change: 1 addition & 0 deletions crates/ruff_workspace/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1159,6 +1159,7 @@ mod tests {
Rule::TooManyPublicMethods,
Rule::UndocumentedWarn,
Rule::UnnecessaryEnumerate,
Rule::MathConstant,
];

#[allow(clippy::needless_pass_by_value)]
Expand Down
2 changes: 2 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading