Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
aafb689
fix invalid hoist
w0nder1ng Nov 15, 2024
fd71328
tentative fix
w0nder1ng Nov 15, 2024
f30118e
don't use a comprehension when the list is referenced
w0nder1ng Nov 15, 2024
238d17a
check for external references to the for loop variable
w0nder1ng Nov 18, 2024
46bd851
stop duplicating comments inside the fixed append
w0nder1ng Nov 18, 2024
9a336fd
fix annotated returns, but lose type info
w0nder1ng Nov 18, 2024
63b0e1e
apply fix to type-annotated lists
w0nder1ng Nov 18, 2024
527a050
fix async extends, implicit tuples, and comments in iterator
w0nder1ng Dec 3, 2024
f339d70
stop deleting semicolon statements in the same line as binding
w0nder1ng Dec 6, 2024
f6ccb39
move manual list comprehension to deferred check
w0nder1ng Dec 6, 2024
6a98542
add debugging code to show binding
w0nder1ng Dec 6, 2024
e2eec2e
find the right binding for the loop variable
w0nder1ng Dec 9, 2024
9d41fb7
check that the correct for loop is picked
w0nder1ng Dec 9, 2024
104b69e
don't stop fix if a reference to target is actually another binding
w0nder1ng Dec 9, 2024
e44fcaa
allow shadowed bindings from any scope
w0nder1ng Dec 9, 2024
4696878
simplify check for loop variable usages
w0nder1ng Dec 10, 2024
74afdb0
cargo fmt
w0nder1ng Dec 10, 2024
db8d0d6
Few simplifications
MichaReiser Dec 11, 2024
77a0290
Merge branch 'main' into perf401_invalid_hoist
w0nder1ng Dec 11, 2024
c9b394c
fix merge
w0nder1ng Dec 11, 2024
9c55f1b
replace tokenization with simple character iterator
w0nder1ng Dec 11, 2024
523808d
Use simple tokenizer
MichaReiser Dec 12, 2024
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
Prev Previous commit
Next Next commit
check for external references to the for loop variable
  • Loading branch information
w0nder1ng committed Nov 18, 2024
commit 238d17a54fbf797b1ef1e314163fd64401c1b37e
12 changes: 12 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,3 +156,15 @@ def f():
result.append(1)
for i in range(10):
result.append(i*2) # PERF401

def f():
result = []
result += [1]
for i in range(10):
result.append(i*2) # PERF401

def f():
result = []
for val in range(5):
result.append(val * 2) # Ok
print(val)
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,34 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S
return;
}

// Avoid if the for-loop target is used outside of the for loop, e.g.,
//
// ```python
// for x in y:
// filtered.append(x)
// print(x)
// ```
//
// If this were a comprehension, x would no longer have the correct scope:
//
// ```python
// filtered = [x for x in y]
// print(x)
// ```
let for_loop_target = checker
.semantic()
.lookup_symbol(id.as_str())
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For some reason, resolve_name returns None for the for-loop target. Also, references to it outside of the for-loop are not included in its list of references; e.g.

def f():
    result = []
    for val in range(5):
        result.append(val * 2)
    print(val) # this reference is not included in the list of references

Copy link
Copy Markdown
Member

@MichaReiser MichaReiser Nov 18, 2024

Choose a reason for hiding this comment

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

I think the problem here is that we build the semantic model lazily as we traverse the AST for checking and it hasn't reached the point after the for loop yet

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@charliermarsh any suggestions on how to best find all usages of target?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think a working version would look something like this:

let for_loop_read = checker.semantic().resolve_load(for_loop_target_name_expr);
let for_loop_binding = match for_loop_read {
    ReadResult::Resolved(id) => checker.semantic().binding(id),
    _ => unreachable!("for loop target must exist"),
};

Unfortunately, resolve_load requires an &mut SemanticModel and I don't see a way to get one from the checker. Am I missing something here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, I didn't look deeply, but often in those cases you need to make it a deferred rule, so it runs after the semantic model has been built (e.g., run it in deferred_for_loops).

.map(|id| checker.semantic().binding(id))
.expect("for loop target must exist");
// TODO: this currently does not properly find usages outside the for loop; figure out why
if for_loop_target
.references()
.map(|id| checker.semantic().reference(id))
.any(|reference| !for_stmt.range.contains_range(reference.range()))
{
return;
}

let binding_stmt = binding
.statement(checker.semantic())
.and_then(|stmt| stmt.as_assign_stmt());
Expand Down Expand Up @@ -261,7 +289,7 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S
// if there's more than one, then it's been accessed in the middle somewhere, so it's not safe to change into a list comprehension
count < 2
});

// If the binding has multiple statements on its line, it gets more complicated to fix, so we use an extend
// TODO: make this work
let binding_only_stmt_on_line = binding_stmt.is_some_and(|_binding_stmt| true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,5 +121,17 @@ PERF401.py:158:9: PERF401 Use `list.extend` to create a transformed list
157 | for i in range(10):
158 | result.append(i*2) # PERF401
| ^^^^^^^^^^^^^^^^^^ PERF401
159 |
160 | def f():
|
= help: Replace for loop with list.extend

PERF401.py:169:9: PERF401 Use a list comprehension to create a transformed list
|
167 | result = []
168 | for val in range(5):
169 | result.append(val * 2) # Ok
| ^^^^^^^^^^^^^^^^^^^^^^ PERF401
170 | print(val)
|
= help: Replace for loop with list comprehension
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,8 @@ PERF401.py:158:9: PERF401 [*] Use `list.extend` to create a transformed list
157 | for i in range(10):
158 | result.append(i*2) # PERF401
| ^^^^^^^^^^^^^^^^^^ PERF401
159 |
160 | def f():
|
= help: Replace for loop with list.extend

Expand All @@ -289,3 +291,26 @@ PERF401.py:158:9: PERF401 [*] Use `list.extend` to create a transformed list
157 |- for i in range(10):
158 |- result.append(i*2) # PERF401
157 |+ result.extend(i*2 for i in range(10)) # PERF401
159 158 |
160 159 | def f():
161 160 | result = []

PERF401.py:169:9: PERF401 [*] Use a list comprehension to create a transformed list
|
167 | result = []
168 | for val in range(5):
169 | result.append(val * 2) # Ok
| ^^^^^^^^^^^^^^^^^^^^^^ PERF401
170 | print(val)
|
= help: Replace for loop with list comprehension

ℹ Unsafe fix
164 164 | result.append(i*2) # PERF401
165 165 |
166 166 | def f():
167 |- result = []
168 |- for val in range(5):
169 |- result.append(val * 2) # Ok
167 |+ result = [val * 2 for val in range(5)] # Ok
170 168 | print(val)