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.

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

[perflint] Simplify finding the loop target in PERF401 #15025

Merged
merged 2 commits into from
Dec 17, 2024
+14 −24
Merged

[perflint] Simplify finding the loop target in PERF401 #15025

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a64781b
simplify finding the correct binding
w0nder1ng Dec 16, 2024
674c181
add test case
w0nder1ng Dec 16, 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
7 changes: 7 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,3 +244,10 @@ def g():
for a in values:
result.append(a + 1) # PERF401
result = []

def f():
values = [1, 2, 3]
result = []
for i in values:
result.append(i + 1) # Ok
del i
29 changes: 5 additions & 24 deletions crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,31 +237,12 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S
// filtered = [x for x in y]
// print(x)
// ```
let last_target_binding = checker
let target_binding = checker
.semantic()
.lookup_symbol(for_stmt_target_id)
.expect("for loop target must exist");

let target_binding = {
let mut bindings = [last_target_binding].into_iter().chain(
checker
.semantic()
.shadowed_bindings(checker.semantic().scope_id, last_target_binding)
.filter_map(|shadowed| shadowed.same_scope().then_some(shadowed.shadowed_id())),
);

bindings
.find_map(|binding_id| {
let binding = checker.semantic().binding(binding_id);
binding
.statement(checker.semantic())
.and_then(ast::Stmt::as_for_stmt)
.is_some_and(|stmt| stmt.range == for_stmt.range)
.then_some(binding)
})
.expect("for target binding must exist")
};

.bindings
.iter()
.find(|binding| for_stmt.target.range() == binding.range)
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Is this unwrap safe? If so, we should use expect but I'd possibly just do an early return if this is None.

Copy link
Member

Choose a reason for hiding this comment

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

It should be fine. The rule only applies to for-loop targets that are names; a binding must exist.

// If any references to the loop target variable are after the loop,
// then converting it into a comprehension would cause a NameError
if target_binding
Expand Down
2 changes: 2 additions & 0 deletions .../perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF401_PERF401.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,3 +484,5 @@ PERF401.py:245:13: PERF401 [*] Use `list.extend` to create a transformed list
245 |- result.append(a + 1) # PERF401
244 |+ result.extend(a + 1 for a in values) # PERF401
246 245 | result = []
247 246 |
248 247 | def f():
Loading