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

[perflint] Parenthesize walrus expressions in autofix for manual-list-comprehension (PERF401) #15050

Merged
merged 5 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -251,3 +251,12 @@ def f():
for i in values:
result.append(i + 1) # Ok
del i

# The fix here must parenthesize the walrus operator
# https://github.com/astral-sh/ruff/issues/15047
def f():
items = []

for i in range(5):
if j := i:
items.append(j)
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ use ruff_text_size::{Ranged, TextRange};
/// original = list(range(10000))
/// filtered.extend(x for x in original if x % 2)
/// ```
///
/// Take care that if the original for-loop uses an assignment expression
/// as a conditional, such as `if match:=re.match("\d+","123")`, then
/// the corresponding comprehension must wrap the assignment
/// expression in parentheses to avoid a syntax error.
#[derive(ViolationMetadata)]
pub(crate) struct ManualListComprehension {
is_async: bool,
Expand Down Expand Up @@ -347,7 +352,21 @@ fn convert_to_list_extend(
let semantic = checker.semantic();
let locator = checker.locator();
let if_str = match if_test {
Some(test) => format!(" if {}", locator.slice(test.range())),
Some(test) => {
// If the test is an assignment expression,
// we must parenthesize it when it appears
// inside the comprehension to avoid a syntax error.
//
// Notice that we do not need `any_over_expr` here,
// since if the assignment expression appears
// internally (e.g. as an operand in a boolean
// operation) then it will already be parenthesized.
dylwil3 marked this conversation as resolved.
Show resolved Hide resolved
if test.is_named_expr() {
format!(" if ({})", locator.slice(test.range()))
} else {
format!(" if {}", locator.slice(test.range()))
}
}
None => String::new(),
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,3 +202,12 @@ PERF401.py:245:13: PERF401 Use `list.extend` to create a transformed list
246 | result = []
|
= help: Replace for loop with list.extend

PERF401.py:262:13: PERF401 Use a list comprehension to create a transformed list
|
260 | for i in range(5):
261 | if j := i:
262 | items.append(j)
| ^^^^^^^^^^^^^^^ PERF401
|
= help: Replace for loop with list comprehension
Original file line number Diff line number Diff line change
Expand Up @@ -486,3 +486,23 @@ PERF401.py:245:13: PERF401 [*] Use `list.extend` to create a transformed list
246 245 | result = []
247 246 |
248 247 | def f():

PERF401.py:262:13: PERF401 [*] Use a list comprehension to create a transformed list
|
260 | for i in range(5):
261 | if j := i:
262 | items.append(j)
| ^^^^^^^^^^^^^^^ PERF401
|
= help: Replace for loop with list comprehension

ℹ Unsafe fix
255 255 | # The fix here must parenthesize the walrus operator
256 256 | # https://github.com/astral-sh/ruff/issues/15047
257 257 | def f():
258 |- items = []
259 258 |
260 |- for i in range(5):
261 |- if j := i:
262 |- items.append(j)
259 |+ items = [j for i in range(5) if (j := i)]
Loading