diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB163.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB163.py new file mode 100644 index 0000000000000..1f2255be8ef74 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB163.py @@ -0,0 +1,47 @@ +import math + +from math import e as special_e +from math import log as special_log + +# Errors. +math.log(1, 2) +math.log(1, 10) +math.log(1, math.e) +foo = ... +math.log(foo, 2) +math.log(foo, 10) +math.log(foo, math.e) +math.log(1, special_e) +special_log(1, 2) +special_log(1, 10) +special_log(1, math.e) +special_log(1, special_e) + +# Ok. +math.log2(1) +math.log10(1) +math.log(1) +math.log(1, 3) +math.log(1, math.pi) + +two = 2 +math.log(1, two) + +ten = 10 +math.log(1, ten) + +e = math.e +math.log(1, e) + +math.log2(1, 10) # math.log2 takes only one argument. +math.log10(1, 2) # math.log10 takes only one argument. + +math.log(1, base=2) # math.log does not accept keyword arguments. + +def log(*args): + print(f"Logging: {args}") + + +log(1, 2) +log(1, 10) +log(1, math.e) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index e5ca3bbee31f6..f3e8bbaecfe6e 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -928,6 +928,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::PrintEmptyString) { refurb::rules::print_empty_string(checker, call); } + if checker.enabled(Rule::RedundantLogBase) { + refurb::rules::redundant_log_base(checker, call); + } if checker.enabled(Rule::QuadraticListSummation) { ruff::rules::quadratic_list_summation(checker, call); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 1e04278e0f15b..4dc1f13c55e05 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -955,6 +955,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (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, "163") => (RuleGroup::Preview, rules::refurb::rules::RedundantLogBase), (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 49a2c49bfe6c2..a7640bcd4c72b 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/mod.rs @@ -28,6 +28,7 @@ mod tests { #[test_case(Rule::SingleItemMembershipTest, Path::new("FURB171.py"))] #[test_case(Rule::IsinstanceTypeNone, Path::new("FURB168.py"))] #[test_case(Rule::TypeNoneComparison, Path::new("FURB169.py"))] + #[test_case(Rule::RedundantLogBase, Path::new("FURB163.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/refurb/rules/mod.rs b/crates/ruff_linter/src/rules/refurb/rules/mod.rs index 6729202272905..d43f87bced0aa 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/mod.rs @@ -6,6 +6,7 @@ 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 redundant_log_base::*; pub(crate) use reimplemented_starmap::*; pub(crate) use repeated_append::*; pub(crate) use single_item_membership_test::*; @@ -21,6 +22,7 @@ mod isinstance_type_none; mod math_constant; mod print_empty_string; mod read_whole_file; +mod redundant_log_base; mod reimplemented_starmap; mod repeated_append; mod single_item_membership_test; diff --git a/crates/ruff_linter/src/rules/refurb/rules/redundant_log_base.rs b/crates/ruff_linter/src/rules/refurb/rules/redundant_log_base.rs new file mode 100644 index 0000000000000..a4a6e58959326 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/redundant_log_base.rs @@ -0,0 +1,149 @@ +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, Expr, Number}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; +use crate::importer::ImportRequest; + +/// ## What it does +/// Checks for `math.log` calls with a redundant base. +/// +/// ## Why is this bad? +/// The default base of `math.log` is `e`, so specifying it explicitly is +/// redundant. +/// +/// Instead of passing 2 or 10 as the base, use `math.log2` or `math.log10` +/// respectively, as these dedicated variants are typically more accurate +/// than `math.log`. +/// +/// ## Example +/// ```python +/// import math +/// +/// math.log(4, math.e) +/// math.log(4, 2) +/// math.log(4, 10) +/// ``` +/// +/// Use instead: +/// ```python +/// import math +/// +/// math.log(4) +/// math.log2(4) +/// math.log10(4) +/// ``` +/// +/// ## References +/// - [Python documentation: `math.log`](https://docs.python.org/3/library/math.html#math.log) +/// - [Python documentation: `math.log2`](https://docs.python.org/3/library/math.html#math.log2) +/// - [Python documentation: `math.log10`](https://docs.python.org/3/library/math.html#math.log10) +/// - [Python documentation: `math.e`](https://docs.python.org/3/library/math.html#math.e) +#[violation] +pub struct RedundantLogBase { + base: Base, + arg: String, +} + +impl Violation for RedundantLogBase { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + let RedundantLogBase { base, arg } = self; + let log_function = base.to_log_function(); + format!("Prefer `math.{log_function}({arg})` over `math.log` with a redundant base") + } + + fn fix_title(&self) -> Option { + let RedundantLogBase { base, arg } = self; + let log_function = base.to_log_function(); + Some(format!("Replace with `math.{log_function}({arg})`")) + } +} + +/// FURB163 +pub(crate) fn redundant_log_base(checker: &mut Checker, call: &ast::ExprCall) { + if !call.arguments.keywords.is_empty() { + return; + } + + let [arg, base] = &call.arguments.args.as_slice() else { + return; + }; + + if !checker + .semantic() + .resolve_call_path(&call.func) + .as_ref() + .is_some_and(|call_path| matches!(call_path.as_slice(), ["math", "log"])) + { + return; + } + + let base = if is_number_literal(base, 2) { + Base::Two + } else if is_number_literal(base, 10) { + Base::Ten + } else if checker + .semantic() + .resolve_call_path(base) + .as_ref() + .is_some_and(|call_path| matches!(call_path.as_slice(), ["math", "e"])) + { + Base::E + } else { + return; + }; + + let mut diagnostic = Diagnostic::new( + RedundantLogBase { + base, + arg: checker.locator().slice(arg).into(), + }, + call.range(), + ); + diagnostic.try_set_fix(|| generate_fix(checker, call, base, arg)); + checker.diagnostics.push(diagnostic); +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Base { + E, + Two, + Ten, +} + +impl Base { + fn to_log_function(self) -> &'static str { + match self { + Base::E => "log", + Base::Two => "log2", + Base::Ten => "log10", + } + } +} + +fn is_number_literal(expr: &Expr, value: i8) -> bool { + if let Expr::NumberLiteral(number_literal) = expr { + if let Number::Int(number) = &number_literal.value { + return number.as_i8().is_some_and(|number| number == value); + } + } + false +} + +fn generate_fix(checker: &Checker, call: &ast::ExprCall, base: Base, arg: &Expr) -> Result { + let (edit, binding) = checker.importer().get_or_import_symbol( + &ImportRequest::import("math", base.to_log_function()), + call.start(), + checker.semantic(), + )?; + let number = checker.locator().slice(arg); + Ok(Fix::safe_edits( + Edit::range_replacement(format!("{binding}({number})"), call.range()), + [edit], + )) +} diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB163_FURB163.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB163_FURB163.py.snap new file mode 100644 index 0000000000000..e6441a4c1e3e0 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB163_FURB163.py.snap @@ -0,0 +1,233 @@ +--- +source: crates/ruff_linter/src/rules/refurb/mod.rs +--- +FURB163.py:7:1: FURB163 [*] Prefer `math.log2(1)` over `math.log` with a redundant base + | +6 | # Errors. +7 | math.log(1, 2) + | ^^^^^^^^^^^^^^ FURB163 +8 | math.log(1, 10) +9 | math.log(1, math.e) + | + = help: Replace with `math.log2(1)` + +ℹ Safe fix +4 4 | from math import log as special_log +5 5 | +6 6 | # Errors. +7 |-math.log(1, 2) + 7 |+math.log2(1) +8 8 | math.log(1, 10) +9 9 | math.log(1, math.e) +10 10 | foo = ... + +FURB163.py:8:1: FURB163 [*] Prefer `math.log10(1)` over `math.log` with a redundant base + | + 6 | # Errors. + 7 | math.log(1, 2) + 8 | math.log(1, 10) + | ^^^^^^^^^^^^^^^ FURB163 + 9 | math.log(1, math.e) +10 | foo = ... + | + = help: Replace with `math.log10(1)` + +ℹ Safe fix +5 5 | +6 6 | # Errors. +7 7 | math.log(1, 2) +8 |-math.log(1, 10) + 8 |+math.log10(1) +9 9 | math.log(1, math.e) +10 10 | foo = ... +11 11 | math.log(foo, 2) + +FURB163.py:9:1: FURB163 [*] Prefer `math.log(1)` over `math.log` with a redundant base + | + 7 | math.log(1, 2) + 8 | math.log(1, 10) + 9 | math.log(1, math.e) + | ^^^^^^^^^^^^^^^^^^^ FURB163 +10 | foo = ... +11 | math.log(foo, 2) + | + = help: Replace with `math.log(1)` + +ℹ Safe fix +6 6 | # Errors. +7 7 | math.log(1, 2) +8 8 | math.log(1, 10) +9 |-math.log(1, math.e) + 9 |+math.log(1) +10 10 | foo = ... +11 11 | math.log(foo, 2) +12 12 | math.log(foo, 10) + +FURB163.py:11:1: FURB163 [*] Prefer `math.log2(foo)` over `math.log` with a redundant base + | + 9 | math.log(1, math.e) +10 | foo = ... +11 | math.log(foo, 2) + | ^^^^^^^^^^^^^^^^ FURB163 +12 | math.log(foo, 10) +13 | math.log(foo, math.e) + | + = help: Replace with `math.log2(foo)` + +ℹ Safe fix +8 8 | math.log(1, 10) +9 9 | math.log(1, math.e) +10 10 | foo = ... +11 |-math.log(foo, 2) + 11 |+math.log2(foo) +12 12 | math.log(foo, 10) +13 13 | math.log(foo, math.e) +14 14 | math.log(1, special_e) + +FURB163.py:12:1: FURB163 [*] Prefer `math.log10(foo)` over `math.log` with a redundant base + | +10 | foo = ... +11 | math.log(foo, 2) +12 | math.log(foo, 10) + | ^^^^^^^^^^^^^^^^^ FURB163 +13 | math.log(foo, math.e) +14 | math.log(1, special_e) + | + = help: Replace with `math.log10(foo)` + +ℹ Safe fix +9 9 | math.log(1, math.e) +10 10 | foo = ... +11 11 | math.log(foo, 2) +12 |-math.log(foo, 10) + 12 |+math.log10(foo) +13 13 | math.log(foo, math.e) +14 14 | math.log(1, special_e) +15 15 | special_log(1, 2) + +FURB163.py:13:1: FURB163 [*] Prefer `math.log(foo)` over `math.log` with a redundant base + | +11 | math.log(foo, 2) +12 | math.log(foo, 10) +13 | math.log(foo, math.e) + | ^^^^^^^^^^^^^^^^^^^^^ FURB163 +14 | math.log(1, special_e) +15 | special_log(1, 2) + | + = help: Replace with `math.log(foo)` + +ℹ Safe fix +10 10 | foo = ... +11 11 | math.log(foo, 2) +12 12 | math.log(foo, 10) +13 |-math.log(foo, math.e) + 13 |+math.log(foo) +14 14 | math.log(1, special_e) +15 15 | special_log(1, 2) +16 16 | special_log(1, 10) + +FURB163.py:14:1: FURB163 [*] Prefer `math.log(1)` over `math.log` with a redundant base + | +12 | math.log(foo, 10) +13 | math.log(foo, math.e) +14 | math.log(1, special_e) + | ^^^^^^^^^^^^^^^^^^^^^^ FURB163 +15 | special_log(1, 2) +16 | special_log(1, 10) + | + = help: Replace with `math.log(1)` + +ℹ Safe fix +11 11 | math.log(foo, 2) +12 12 | math.log(foo, 10) +13 13 | math.log(foo, math.e) +14 |-math.log(1, special_e) + 14 |+math.log(1) +15 15 | special_log(1, 2) +16 16 | special_log(1, 10) +17 17 | special_log(1, math.e) + +FURB163.py:15:1: FURB163 [*] Prefer `math.log2(1)` over `math.log` with a redundant base + | +13 | math.log(foo, math.e) +14 | math.log(1, special_e) +15 | special_log(1, 2) + | ^^^^^^^^^^^^^^^^^ FURB163 +16 | special_log(1, 10) +17 | special_log(1, math.e) + | + = help: Replace with `math.log2(1)` + +ℹ Safe fix +12 12 | math.log(foo, 10) +13 13 | math.log(foo, math.e) +14 14 | math.log(1, special_e) +15 |-special_log(1, 2) + 15 |+math.log2(1) +16 16 | special_log(1, 10) +17 17 | special_log(1, math.e) +18 18 | special_log(1, special_e) + +FURB163.py:16:1: FURB163 [*] Prefer `math.log10(1)` over `math.log` with a redundant base + | +14 | math.log(1, special_e) +15 | special_log(1, 2) +16 | special_log(1, 10) + | ^^^^^^^^^^^^^^^^^^ FURB163 +17 | special_log(1, math.e) +18 | special_log(1, special_e) + | + = help: Replace with `math.log10(1)` + +ℹ Safe fix +13 13 | math.log(foo, math.e) +14 14 | math.log(1, special_e) +15 15 | special_log(1, 2) +16 |-special_log(1, 10) + 16 |+math.log10(1) +17 17 | special_log(1, math.e) +18 18 | special_log(1, special_e) +19 19 | + +FURB163.py:17:1: FURB163 [*] Prefer `math.log(1)` over `math.log` with a redundant base + | +15 | special_log(1, 2) +16 | special_log(1, 10) +17 | special_log(1, math.e) + | ^^^^^^^^^^^^^^^^^^^^^^ FURB163 +18 | special_log(1, special_e) + | + = help: Replace with `math.log(1)` + +ℹ Safe fix +14 14 | math.log(1, special_e) +15 15 | special_log(1, 2) +16 16 | special_log(1, 10) +17 |-special_log(1, math.e) + 17 |+math.log(1) +18 18 | special_log(1, special_e) +19 19 | +20 20 | # Ok. + +FURB163.py:18:1: FURB163 [*] Prefer `math.log(1)` over `math.log` with a redundant base + | +16 | special_log(1, 10) +17 | special_log(1, math.e) +18 | special_log(1, special_e) + | ^^^^^^^^^^^^^^^^^^^^^^^^^ FURB163 +19 | +20 | # Ok. + | + = help: Replace with `math.log(1)` + +ℹ Safe fix +15 15 | special_log(1, 2) +16 16 | special_log(1, 10) +17 17 | special_log(1, math.e) +18 |-special_log(1, special_e) + 18 |+math.log(1) +19 19 | +20 20 | # Ok. +21 21 | math.log2(1) + + diff --git a/ruff.schema.json b/ruff.schema.json index 8ae7c3fe37fb2..38b1dbc5319ac 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2849,6 +2849,7 @@ "FURB15", "FURB152", "FURB16", + "FURB163", "FURB168", "FURB169", "FURB17",