Skip to content

[refurb] Make the fix for FURB163 unsafe for log2, log10, *args, and deleted comments #18645

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jun 16, 2025
Merged

[refurb] Make the fix for FURB163 unsafe for log2, log10, *args, and deleted comments #18645

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ee8e09e
[`refurb`] make fix for FURB163 (`redundant-log-base`) unsafe if log2…
chirizxc Jun 12, 2025
74e07c3
fix: mark fix as unsafe only for float literals
chirizxc Jun 12, 2025
5bbe317
chore: simplify
chirizxc Jun 12, 2025
9cedd1b
doc: update
chirizxc Jun 12, 2025
54d09d0
doc: update
chirizxc Jun 12, 2025
9dec791
make docs a tiny bit more explicit
ntBre Jun 16, 2025
4dd0598
fmt
ntBre Jun 16, 2025
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
30 changes: 30 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB163.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,33 @@ def log(*args):

math.log(1, 2.0001)
math.log(1, 10.0001)


# see: https://github.com/astral-sh/ruff/issues/18639
math.log(1, 10 # comment
)

math.log(1,
10 # comment
)

math.log(1 # comment
, # comment
10 # comment
)

math.log(
1 # comment
,
10 # comment
)

math.log(4.13e223, 2)
math.log(4.14e223, 10)


def print_log(*args):
try:
print(math.log(*args, math.e))
except TypeError as e:
print(repr(e))
17 changes: 16 additions & 1 deletion crates/ruff_linter/src/rules/refurb/rules/redundant_log_base.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use anyhow::Result;
use ruff_diagnostics::Applicability;
use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_python_ast::{self as ast, Expr, Number};
use ruff_text_size::Ranged;
Expand Down Expand Up @@ -135,15 +136,29 @@ fn is_number_literal(expr: &Expr, value: i8) -> bool {
false
}

fn is_float(expr: &Expr) -> bool {
Copy link
Contributor Author

@chirizxc chirizxc Jun 12, 2025

Choose a reason for hiding this comment

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

i'm not sure, but maybe such a is_float already exists somewhere in the helper or something like that

Copy link
Contributor

@ntBre ntBre Jun 12, 2025

Choose a reason for hiding this comment

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

There's this:

ruff/crates/ruff_python_semantic/src/analyze/typing.rs

Lines 981 to 984 in 4cfaa39

/// Test whether the given binding can be considered an instance of `float`.
pub fn is_float(binding: &Binding, semantic: &SemanticModel) -> bool {
check_type::<FloatChecker>(binding, semantic)
}

which would work on a Binding if you wanted to resolve variables too, but do we need to check if the argument is a float? You can see the same floating point behavior with less exotic arguments:

>>> math.log2(10)
3.321928094887362
>>> math.log(10, 2)
3.3219280948873626

and I think we're already checking for number literals.

matches!(expr, Expr::NumberLiteral(num) if matches!(num.value, Number::Float(_)))
}

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(

Ok(Fix::applicable_edits(
Edit::range_replacement(format!("{binding}({number})"), call.range()),
[edit],
if (matches!(base, Base::Two | Base::Ten) && is_float(arg))
|| matches!(arg, Expr::Starred(_))
Copy link
Contributor

@ntBre ntBre Jun 12, 2025

Choose a reason for hiding this comment

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

nit: I think you can use arg.is_starred_expr here.

|| checker.comment_ranges().intersects(call.range())
{
Applicability::Unsafe
} else {
Applicability::Safe
},
))
}
161 changes: 161 additions & 0 deletions ...ter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB163_FURB163.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,3 +166,164 @@ FURB163.py:12:1: FURB163 [*] Prefer `math.log10(1)` over `math.log` with a redun
13 13 |
14 14 | # OK
15 15 | math.log2(1)

FURB163.py:49:1: FURB163 [*] Prefer `math.log10(1)` over `math.log` with a redundant base
|
48 | # see: https://github.com/astral-sh/ruff/issues/18639
49 | / math.log(1, 10 # comment
50 | | )
| |__________^ FURB163
51 |
52 | math.log(1,
|
= help: Replace with `math.log10(1)`

ℹ Unsafe fix
46 46 |
47 47 |
48 48 | # see: https://github.com/astral-sh/ruff/issues/18639
49 |-math.log(1, 10 # comment
50 |- )
49 |+math.log10(1)
51 50 |
52 51 | math.log(1,
53 52 | 10 # comment

FURB163.py:52:1: FURB163 [*] Prefer `math.log10(1)` over `math.log` with a redundant base
|
50 | )
51 |
52 | / math.log(1,
53 | | 10 # comment
54 | | )
| |__________^ FURB163
55 |
56 | math.log(1 # comment
|
= help: Replace with `math.log10(1)`

ℹ Unsafe fix
49 49 | math.log(1, 10 # comment
50 50 | )
51 51 |
52 |-math.log(1,
53 |- 10 # comment
54 |- )
52 |+math.log10(1)
55 53 |
56 54 | math.log(1 # comment
57 55 | , # comment

FURB163.py:56:1: FURB163 [*] Prefer `math.log10(1)` over `math.log` with a redundant base
|
54 | )
55 |
56 | / math.log(1 # comment
57 | | , # comment
58 | | 10 # comment
59 | | )
| |__________^ FURB163
60 |
61 | math.log(
|
= help: Replace with `math.log10(1)`

ℹ Unsafe fix
53 53 | 10 # comment
54 54 | )
55 55 |
56 |-math.log(1 # comment
57 |- , # comment
58 |- 10 # comment
59 |- )
56 |+math.log10(1)
60 57 |
61 58 | math.log(
62 59 | 1 # comment

FURB163.py:61:1: FURB163 [*] Prefer `math.log10(1)` over `math.log` with a redundant base
|
59 | )
60 |
61 | / math.log(
62 | | 1 # comment
63 | | ,
64 | | 10 # comment
65 | | )
| |_^ FURB163
66 |
67 | math.log(4.13e223, 2)
|
= help: Replace with `math.log10(1)`

ℹ Unsafe fix
58 58 | 10 # comment
59 59 | )
60 60 |
61 |-math.log(
62 |- 1 # comment
63 |- ,
64 |- 10 # comment
65 |-)
61 |+math.log10(1)
66 62 |
67 63 | math.log(4.13e223, 2)
68 64 | math.log(4.14e223, 10)

FURB163.py:67:1: FURB163 [*] Prefer `math.log2(4.13e223)` over `math.log` with a redundant base
|
65 | )
66 |
67 | math.log(4.13e223, 2)
| ^^^^^^^^^^^^^^^^^^^^^ FURB163
68 | math.log(4.14e223, 10)
|
= help: Replace with `math.log2(4.13e223)`

ℹ Unsafe fix
64 64 | 10 # comment
65 65 | )
66 66 |
67 |-math.log(4.13e223, 2)
67 |+math.log2(4.13e223)
68 68 | math.log(4.14e223, 10)
69 69 |
70 70 |

FURB163.py:68:1: FURB163 [*] Prefer `math.log10(4.14e223)` over `math.log` with a redundant base
|
67 | math.log(4.13e223, 2)
68 | math.log(4.14e223, 10)
| ^^^^^^^^^^^^^^^^^^^^^^ FURB163
|
= help: Replace with `math.log10(4.14e223)`

ℹ Unsafe fix
65 65 | )
66 66 |
67 67 | math.log(4.13e223, 2)
68 |-math.log(4.14e223, 10)
68 |+math.log10(4.14e223)
69 69 |
70 70 |
71 71 | def print_log(*args):

FURB163.py:73:15: FURB163 [*] Prefer `math.log(*args)` over `math.log` with a redundant base
|
71 | def print_log(*args):
72 | try:
73 | print(math.log(*args, math.e))
| ^^^^^^^^^^^^^^^^^^^^^^^ FURB163
74 | except TypeError as e:
75 | print(repr(e))
|
= help: Replace with `math.log(*args)`

ℹ Unsafe fix
70 70 |
71 71 | def print_log(*args):
72 72 | try:
73 |- print(math.log(*args, math.e))
73 |+ print(math.log(*args))
74 74 | except TypeError as e:
75 75 | print(repr(e))
Loading