Skip to content
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
23 changes: 22 additions & 1 deletion crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py
Original file line number Diff line number Diff line change
Expand Up @@ -304,4 +304,25 @@ def x():
nonlocal NL_INDEX
result = []
for NL_INDEX in range(3):
result.append(NL_INDEX)
result.append(NL_INDEX)

def f():
# comment duplication in if test (https://github.com/astral-sh/ruff/issues/18787)
original = list(range(10000))
filtered = []
for i in original:
if (
i
# comment
):
filtered.append(i)

def f():
# comment duplication in target (https://github.com/astral-sh/ruff/issues/18787)
original = list(range(10000))
filtered = []
for (
i # comment
) in original:
if i > 0:
filtered.append(i)
Comment on lines +320 to +328
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you have to change the code to handle this case as well as the one above? I ask because these look very similar to me, and I'm wondering if we can drop one of the tests, but if they exercise different parts of the code, it's fine to keep both.

24 changes: 21 additions & 3 deletions crates/ruff_linter/resources/test/fixtures/perflint/PERF403.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,24 @@ def issue_19153_2():

def issue_19153_3():
v = {}
for o, (x,) in ["ox"]:
v[(x,)] = o
return v
for o, (x,) in ["ox"]:
v[(x,)] = o
return v

def f():
# comment duplication in if test (https://github.com/astral-sh/ruff/issues/18787)
result = {}
for k in ["a", "b", "c"]:
if (
k
# comment
):
result[k] = k

def f():
# comment duplication in target (https://github.com/astral-sh/ruff/issues/18787)
result = {}
for (
k # comment
) in ["a", "b", "c"]:
result[k] = k
Original file line number Diff line number Diff line change
Expand Up @@ -387,11 +387,17 @@ fn convert_to_dict_comprehension(
let comprehension_str =
format!("{{{key_str}: {value_str} {for_type} {target_str} in {iter_str}{if_str}}}");

let for_loop_inline_comments = comment_strings_in_range(
checker,
for_stmt.range,
&[key.range(), value.range(), for_stmt.iter.range()],
);
let mut ranges_to_ignore = vec![
key.range(),
value.range(),
for_stmt.iter.range(),
for_stmt.target.range(),
];
if let Some(test) = if_test {
ranges_to_ignore.push(test.range());
}
let for_loop_inline_comments =
comment_strings_in_range(checker, for_stmt.range, &ranges_to_ignore);

let newline = checker.stylist().line_ending().as_str();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -422,11 +422,16 @@ fn convert_to_list_extend(
};

let variable_name = locator.slice(binding);
let for_loop_inline_comments = comment_strings_in_range(
checker,
for_stmt.range,
&[to_append.range(), for_stmt.iter.range()],
);
let mut ranges_to_ignore = vec![
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's a bit sad to allocate a Vec here, but I think it's okay. It would be a slightly larger refactor to make comment_strings_in_range take an iterator and too much duplication to do something like:

if let Some(test) = if_test {
    &[to_append.range(), for_stmt.iter.range(), for_stmt.target.range(), test.range()]
} else {
    &[to_append.range(), for_stmt.iter.range(), for_stmt.target.range()]
}

Although now that I write that out, I don't mind it as much as I expected.

to_append.range(),
for_stmt.iter.range(),
for_stmt.target.range(),
];
if let Some(test) = if_test {
ranges_to_ignore.push(test.range());
}
let for_loop_inline_comments =
comment_strings_in_range(checker, for_stmt.range, &ranges_to_ignore);

let newline = checker.stylist().line_ending().as_str();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -294,3 +294,25 @@ PERF401 Use a list comprehension to create a transformed list
294 | G_INDEX = None
|
help: Replace for loop with list comprehension

PERF401 Use a list comprehension to create a transformed list
--> PERF401.py:318:13
|
316 | # comment
317 | ):
318 | filtered.append(i)
| ^^^^^^^^^^^^^^^^^^
319 |
320 | def f():
|
help: Replace for loop with list comprehension

PERF401 Use a list comprehension to create a transformed list
--> PERF401.py:328:13
|
326 | ) in original:
327 | if i > 0:
328 | filtered.append(i)
| ^^^^^^^^^^^^^^^^^^
|
help: Replace for loop with list comprehension
Original file line number Diff line number Diff line change
Expand Up @@ -203,9 +203,31 @@ PERF403 Use a dictionary comprehension instead of a for-loop
--> PERF403.py:214:9
|
212 | v = {}
213 | for o, (x,) in ["ox"]:
214 | v[(x,)] = o
213 | for o, (x,) in ["ox"]:
214 | v[(x,)] = o
| ^^^^^^^^^^^
215 | return v
|
help: Replace for loop with dict comprehension

PERF403 Use a dictionary comprehension instead of a for-loop
--> PERF403.py:225:13
|
223 | # comment
224 | ):
225 | result[k] = k
| ^^^^^^^^^^^^^
226 |
227 | def f():
|
help: Replace for loop with dict comprehension

PERF403 Use a dictionary comprehension instead of a for-loop
--> PERF403.py:233:9
|
231 | k # comment
232 | ) in ["a", "b", "c"]:
233 | result[k] = k
| ^^^^^^^^^^^^^
|
help: Replace for loop with dict comprehension
Original file line number Diff line number Diff line change
Expand Up @@ -628,3 +628,53 @@ help: Replace for loop with list comprehension
292 | G_INDEX = None
293 | def f():
note: This is an unsafe fix and may change runtime behavior

PERF401 [*] Use a list comprehension to create a transformed list
--> PERF401.py:318:13
|
316 | # comment
317 | ):
318 | filtered.append(i)
| ^^^^^^^^^^^^^^^^^^
319 |
320 | def f():
|
help: Replace for loop with list comprehension
309 | def f():
310 | # comment duplication in if test (https://github.com/astral-sh/ruff/issues/18787)
311 | original = list(range(10000))
- filtered = []
- for i in original:
- if (
- i
- # comment
- ):
- filtered.append(i)
312 + # comment
313 + filtered = [i for i in original if i]
314 |
315 | def f():
316 | # comment duplication in target (https://github.com/astral-sh/ruff/issues/18787)
note: This is an unsafe fix and may change runtime behavior

PERF401 [*] Use a list comprehension to create a transformed list
--> PERF401.py:328:13
|
326 | ) in original:
327 | if i > 0:
328 | filtered.append(i)
| ^^^^^^^^^^^^^^^^^^
|
help: Replace for loop with list comprehension
320 | def f():
321 | # comment duplication in target (https://github.com/astral-sh/ruff/issues/18787)
322 | original = list(range(10000))
- filtered = []
- for (
- i # comment
- ) in original:
- if i > 0:
- filtered.append(i)
323 + # comment
324 + filtered = [i for i in original if i > 0]
note: This is an unsafe fix and may change runtime behavior
Original file line number Diff line number Diff line change
Expand Up @@ -426,8 +426,8 @@ PERF403 [*] Use a dictionary comprehension instead of a for-loop
--> PERF403.py:214:9
|
212 | v = {}
213 | for o, (x,) in ["ox"]:
214 | v[(x,)] = o
213 | for o, (x,) in ["ox"]:
214 | v[(x,)] = o
| ^^^^^^^^^^^
215 | return v
|
Expand All @@ -436,8 +436,59 @@ help: Replace for loop with dict comprehension
210 |
211 | def issue_19153_3():
- v = {}
- for o, (x,) in ["ox"]:
- v[(x,)] = o
212 + v = {(x,): o for o, (x,) in ["ox"]}
- for o, (x,) in ["ox"]:
- v[(x,)] = o
212 + v = {(x,): o for o, (x,) in ["ox"]}
213 | return v
214 |
215 | def f():
note: This is an unsafe fix and may change runtime behavior

PERF403 [*] Use a dictionary comprehension instead of a for-loop
--> PERF403.py:225:13
|
223 | # comment
224 | ):
225 | result[k] = k
| ^^^^^^^^^^^^^
226 |
227 | def f():
|
help: Replace for loop with dict comprehension
216 |
217 | def f():
218 | # comment duplication in if test (https://github.com/astral-sh/ruff/issues/18787)
- result = {}
- for k in ["a", "b", "c"]:
- if (
- k
- # comment
- ):
- result[k] = k
219 + # comment
220 + result = {k: k for k in ["a", "b", "c"] if k}
221 |
222 | def f():
223 | # comment duplication in target (https://github.com/astral-sh/ruff/issues/18787)
note: This is an unsafe fix and may change runtime behavior

PERF403 [*] Use a dictionary comprehension instead of a for-loop
--> PERF403.py:233:9
|
231 | k # comment
232 | ) in ["a", "b", "c"]:
233 | result[k] = k
| ^^^^^^^^^^^^^
|
help: Replace for loop with dict comprehension
226 |
227 | def f():
228 | # comment duplication in target (https://github.com/astral-sh/ruff/issues/18787)
- result = {}
- for (
- k # comment
- ) in ["a", "b", "c"]:
- result[k] = k
229 + # comment
230 + result = {k: k for k in ["a", "b", "c"]}
note: This is an unsafe fix and may change runtime behavior