Skip to content

Commit

Permalink
[perflint] Fix panic in perf401 (#14971)
Browse files Browse the repository at this point in the history
Fixes #14969.

The issue was that this line:

```rust
let from_assign_to_loop = TextRange::new(binding_stmt.end(), for_stmt.start());
```

was not safe if the binding was after the target. The only way (at least
that I can think of) this can happen is if they are in different scopes,
so it now checks for that before checking if there are usages between
the two.
  • Loading branch information
w0nder1ng authored Dec 15, 2024
1 parent 2d15d7d commit 4a7536d
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -237,3 +237,10 @@ def f():
print(a)
for a in values:
result.append(a + 1) # PERF401

def f():
values = [1, 2, 3]
def g():
for a in values:
result.append(a + 1) # PERF401
result = []
Original file line number Diff line number Diff line change
Expand Up @@ -305,15 +305,19 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S
});

// If the binding gets used in between the assignment and the for loop, a list comprehension is no longer safe
let binding_unused_between = list_binding_stmt.is_some_and(|binding_stmt| {
let from_assign_to_loop = TextRange::new(binding_stmt.end(), for_stmt.start());
// Test if there's any reference to the list symbol between its definition and the for loop.
// if there's at least one, then it's been accessed in the middle somewhere, so it's not safe to change into a list comprehension
!list_binding
.references()
.map(|ref_id| checker.semantic().reference(ref_id).range())
.any(|text_range| from_assign_to_loop.contains_range(text_range))
});

// If the binding is after the for loop, then it can't be fixed, and this check would panic,
// so we check that they are in the same statement first
let binding_unused_between = assignment_in_same_statement
&& list_binding_stmt.is_some_and(|binding_stmt| {
let from_assign_to_loop = TextRange::new(binding_stmt.end(), for_stmt.start());
// Test if there's any reference to the list symbol between its definition and the for loop.
// if there's at least one, then it's been accessed in the middle somewhere, so it's not safe to change into a list comprehension
!list_binding
.references()
.map(|ref_id| checker.semantic().reference(ref_id).range())
.any(|text_range| from_assign_to_loop.contains_range(text_range))
});

// A list extend works in every context, while a list comprehension only works when all the criteria are true
let comprehension_type = if binding_is_empty_list
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
---
source: crates/ruff_linter/src/rules/perflint/mod.rs
snapshot_kind: text
---
PERF401.py:6:13: PERF401 Use a list comprehension to create a transformed list
|
Expand Down Expand Up @@ -189,5 +188,17 @@ PERF401.py:239:9: PERF401 Use a list comprehension to create a transformed list
238 | for a in values:
239 | result.append(a + 1) # PERF401
| ^^^^^^^^^^^^^^^^^^^^ PERF401
240 |
241 | def f():
|
= help: Replace for loop with list comprehension

PERF401.py:245:13: PERF401 Use `list.extend` to create a transformed list
|
243 | def g():
244 | for a in values:
245 | result.append(a + 1) # PERF401
| ^^^^^^^^^^^^^^^^^^^^ PERF401
246 | result = []
|
= help: Replace for loop with list.extend
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
---
source: crates/ruff_linter/src/rules/perflint/mod.rs
snapshot_kind: text
---
PERF401.py:6:13: PERF401 [*] Use a list comprehension to create a transformed list
|
Expand Down Expand Up @@ -448,6 +447,8 @@ PERF401.py:239:9: PERF401 [*] Use a list comprehension to create a transformed l
238 | for a in values:
239 | result.append(a + 1) # PERF401
| ^^^^^^^^^^^^^^^^^^^^ PERF401
240 |
241 | def f():
|
= help: Replace for loop with list comprehension

Expand All @@ -461,3 +462,25 @@ PERF401.py:239:9: PERF401 [*] Use a list comprehension to create a transformed l
238 |- for a in values:
239 |- result.append(a + 1) # PERF401
237 |+ result = [a + 1 for a in values] # PERF401
240 238 |
241 239 | def f():
242 240 | values = [1, 2, 3]

PERF401.py:245:13: PERF401 [*] Use `list.extend` to create a transformed list
|
243 | def g():
244 | for a in values:
245 | result.append(a + 1) # PERF401
| ^^^^^^^^^^^^^^^^^^^^ PERF401
246 | result = []
|
= help: Replace for loop with list.extend

Unsafe fix
241 241 | def f():
242 242 | values = [1, 2, 3]
243 243 | def g():
244 |- for a in values:
245 |- result.append(a + 1) # PERF401
244 |+ result.extend(a + 1 for a in values) # PERF401
246 245 | result = []

0 comments on commit 4a7536d

Please sign in to comment.