Skip to content

Commit

Permalink
[refurb] Implement redundant-log-base (FURB163) (#8842)
Browse files Browse the repository at this point in the history
## Summary

Implement
[`simplify-math-log`](https://github.com/dosisod/refurb/blob/master/refurb/checks/math/simplify_log.py)
as `redundant-log-base` (`FURB163`).

Auto-fixes

```python
import math

math.log(2, 2)
```

to

```python
import math

math.log2(2)
```

Related to #1348.

## Test Plan

`cargo test`
  • Loading branch information
tjkuson authored Nov 27, 2023
1 parent 33caa2a commit 60eb11f
Show file tree
Hide file tree
Showing 8 changed files with 437 additions and 0 deletions.
47 changes: 47 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB163.py
Original file line number Diff line number Diff line change
@@ -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)
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 @@ -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);
}
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 @@ -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),
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 @@ -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(
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 @@ -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::*;
Expand All @@ -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;
Expand Down
149 changes: 149 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/redundant_log_base.rs
Original file line number Diff line number Diff line change
@@ -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<String> {
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<Fix> {
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],
))
}
Loading

0 comments on commit 60eb11f

Please sign in to comment.