Skip to content

Commit 46507b8

Browse files
[perflint] Fix comment duplication in fixes (PERF401, PERF403) (#23729)
## Summary - Fix comments inside if test and for target being duplicated when transforming a for-loop into a comprehension ## Test Plan - Added test cases for both PERF401 and PERF403 - Verified comments appear exactly once in preview snapshot fixes ## Closes: [#18787](#18787) <!-- How was it tested? -->
1 parent 7365268 commit 46507b8

8 files changed

Lines changed: 216 additions & 21 deletions

crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -304,4 +304,25 @@ def x():
304304
nonlocal NL_INDEX
305305
result = []
306306
for NL_INDEX in range(3):
307-
result.append(NL_INDEX)
307+
result.append(NL_INDEX)
308+
309+
def f():
310+
# comment duplication in if test (https://github.com/astral-sh/ruff/issues/18787)
311+
original = list(range(10000))
312+
filtered = []
313+
for i in original:
314+
if (
315+
i
316+
# comment
317+
):
318+
filtered.append(i)
319+
320+
def f():
321+
# comment duplication in target (https://github.com/astral-sh/ruff/issues/18787)
322+
original = list(range(10000))
323+
filtered = []
324+
for (
325+
i # comment
326+
) in original:
327+
if i > 0:
328+
filtered.append(i)

crates/ruff_linter/resources/test/fixtures/perflint/PERF403.py

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,24 @@ def issue_19153_2():
210210

211211
def issue_19153_3():
212212
v = {}
213-
for o, (x,) in ["ox"]:
214-
v[(x,)] = o
215-
return v
213+
for o, (x,) in ["ox"]:
214+
v[(x,)] = o
215+
return v
216+
217+
def f():
218+
# comment duplication in if test (https://github.com/astral-sh/ruff/issues/18787)
219+
result = {}
220+
for k in ["a", "b", "c"]:
221+
if (
222+
k
223+
# comment
224+
):
225+
result[k] = k
226+
227+
def f():
228+
# comment duplication in target (https://github.com/astral-sh/ruff/issues/18787)
229+
result = {}
230+
for (
231+
k # comment
232+
) in ["a", "b", "c"]:
233+
result[k] = k

crates/ruff_linter/src/rules/perflint/rules/manual_dict_comprehension.rs

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -387,11 +387,17 @@ fn convert_to_dict_comprehension(
387387
let comprehension_str =
388388
format!("{{{key_str}: {value_str} {for_type} {target_str} in {iter_str}{if_str}}}");
389389

390-
let for_loop_inline_comments = comment_strings_in_range(
391-
checker,
392-
for_stmt.range,
393-
&[key.range(), value.range(), for_stmt.iter.range()],
394-
);
390+
let mut ranges_to_ignore = vec![
391+
key.range(),
392+
value.range(),
393+
for_stmt.iter.range(),
394+
for_stmt.target.range(),
395+
];
396+
if let Some(test) = if_test {
397+
ranges_to_ignore.push(test.range());
398+
}
399+
let for_loop_inline_comments =
400+
comment_strings_in_range(checker, for_stmt.range, &ranges_to_ignore);
395401

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

crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -422,11 +422,16 @@ fn convert_to_list_extend(
422422
};
423423

424424
let variable_name = locator.slice(binding);
425-
let for_loop_inline_comments = comment_strings_in_range(
426-
checker,
427-
for_stmt.range,
428-
&[to_append.range(), for_stmt.iter.range()],
429-
);
425+
let mut ranges_to_ignore = vec![
426+
to_append.range(),
427+
for_stmt.iter.range(),
428+
for_stmt.target.range(),
429+
];
430+
if let Some(test) = if_test {
431+
ranges_to_ignore.push(test.range());
432+
}
433+
let for_loop_inline_comments =
434+
comment_strings_in_range(checker, for_stmt.range, &ranges_to_ignore);
430435

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

crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,3 +294,25 @@ PERF401 Use a list comprehension to create a transformed list
294294
294 | G_INDEX = None
295295
|
296296
help: Replace for loop with list comprehension
297+
298+
PERF401 Use a list comprehension to create a transformed list
299+
--> PERF401.py:318:13
300+
|
301+
316 | # comment
302+
317 | ):
303+
318 | filtered.append(i)
304+
| ^^^^^^^^^^^^^^^^^^
305+
319 |
306+
320 | def f():
307+
|
308+
help: Replace for loop with list comprehension
309+
310+
PERF401 Use a list comprehension to create a transformed list
311+
--> PERF401.py:328:13
312+
|
313+
326 | ) in original:
314+
327 | if i > 0:
315+
328 | filtered.append(i)
316+
| ^^^^^^^^^^^^^^^^^^
317+
|
318+
help: Replace for loop with list comprehension

crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF403_PERF403.py.snap

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -203,9 +203,31 @@ PERF403 Use a dictionary comprehension instead of a for-loop
203203
--> PERF403.py:214:9
204204
|
205205
212 | v = {}
206-
213 | for o, (x,) in ["ox"]:
207-
214 | v[(x,)] = o
206+
213 | for o, (x,) in ["ox"]:
207+
214 | v[(x,)] = o
208208
| ^^^^^^^^^^^
209209
215 | return v
210210
|
211211
help: Replace for loop with dict comprehension
212+
213+
PERF403 Use a dictionary comprehension instead of a for-loop
214+
--> PERF403.py:225:13
215+
|
216+
223 | # comment
217+
224 | ):
218+
225 | result[k] = k
219+
| ^^^^^^^^^^^^^
220+
226 |
221+
227 | def f():
222+
|
223+
help: Replace for loop with dict comprehension
224+
225+
PERF403 Use a dictionary comprehension instead of a for-loop
226+
--> PERF403.py:233:9
227+
|
228+
231 | k # comment
229+
232 | ) in ["a", "b", "c"]:
230+
233 | result[k] = k
231+
| ^^^^^^^^^^^^^
232+
|
233+
help: Replace for loop with dict comprehension

crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF401_PERF401.py.snap

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -628,3 +628,53 @@ help: Replace for loop with list comprehension
628628
292 | G_INDEX = None
629629
293 | def f():
630630
note: This is an unsafe fix and may change runtime behavior
631+
632+
PERF401 [*] Use a list comprehension to create a transformed list
633+
--> PERF401.py:318:13
634+
|
635+
316 | # comment
636+
317 | ):
637+
318 | filtered.append(i)
638+
| ^^^^^^^^^^^^^^^^^^
639+
319 |
640+
320 | def f():
641+
|
642+
help: Replace for loop with list comprehension
643+
309 | def f():
644+
310 | # comment duplication in if test (https://github.com/astral-sh/ruff/issues/18787)
645+
311 | original = list(range(10000))
646+
- filtered = []
647+
- for i in original:
648+
- if (
649+
- i
650+
- # comment
651+
- ):
652+
- filtered.append(i)
653+
312 + # comment
654+
313 + filtered = [i for i in original if i]
655+
314 |
656+
315 | def f():
657+
316 | # comment duplication in target (https://github.com/astral-sh/ruff/issues/18787)
658+
note: This is an unsafe fix and may change runtime behavior
659+
660+
PERF401 [*] Use a list comprehension to create a transformed list
661+
--> PERF401.py:328:13
662+
|
663+
326 | ) in original:
664+
327 | if i > 0:
665+
328 | filtered.append(i)
666+
| ^^^^^^^^^^^^^^^^^^
667+
|
668+
help: Replace for loop with list comprehension
669+
320 | def f():
670+
321 | # comment duplication in target (https://github.com/astral-sh/ruff/issues/18787)
671+
322 | original = list(range(10000))
672+
- filtered = []
673+
- for (
674+
- i # comment
675+
- ) in original:
676+
- if i > 0:
677+
- filtered.append(i)
678+
323 + # comment
679+
324 + filtered = [i for i in original if i > 0]
680+
note: This is an unsafe fix and may change runtime behavior

crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF403_PERF403.py.snap

Lines changed: 56 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -426,8 +426,8 @@ PERF403 [*] Use a dictionary comprehension instead of a for-loop
426426
--> PERF403.py:214:9
427427
|
428428
212 | v = {}
429-
213 | for o, (x,) in ["ox"]:
430-
214 | v[(x,)] = o
429+
213 | for o, (x,) in ["ox"]:
430+
214 | v[(x,)] = o
431431
| ^^^^^^^^^^^
432432
215 | return v
433433
|
@@ -436,8 +436,59 @@ help: Replace for loop with dict comprehension
436436
210 |
437437
211 | def issue_19153_3():
438438
- v = {}
439-
- for o, (x,) in ["ox"]:
440-
- v[(x,)] = o
441-
212 + v = {(x,): o for o, (x,) in ["ox"]}
439+
- for o, (x,) in ["ox"]:
440+
- v[(x,)] = o
441+
212 + v = {(x,): o for o, (x,) in ["ox"]}
442442
213 | return v
443+
214 |
444+
215 | def f():
445+
note: This is an unsafe fix and may change runtime behavior
446+
447+
PERF403 [*] Use a dictionary comprehension instead of a for-loop
448+
--> PERF403.py:225:13
449+
|
450+
223 | # comment
451+
224 | ):
452+
225 | result[k] = k
453+
| ^^^^^^^^^^^^^
454+
226 |
455+
227 | def f():
456+
|
457+
help: Replace for loop with dict comprehension
458+
216 |
459+
217 | def f():
460+
218 | # comment duplication in if test (https://github.com/astral-sh/ruff/issues/18787)
461+
- result = {}
462+
- for k in ["a", "b", "c"]:
463+
- if (
464+
- k
465+
- # comment
466+
- ):
467+
- result[k] = k
468+
219 + # comment
469+
220 + result = {k: k for k in ["a", "b", "c"] if k}
470+
221 |
471+
222 | def f():
472+
223 | # comment duplication in target (https://github.com/astral-sh/ruff/issues/18787)
473+
note: This is an unsafe fix and may change runtime behavior
474+
475+
PERF403 [*] Use a dictionary comprehension instead of a for-loop
476+
--> PERF403.py:233:9
477+
|
478+
231 | k # comment
479+
232 | ) in ["a", "b", "c"]:
480+
233 | result[k] = k
481+
| ^^^^^^^^^^^^^
482+
|
483+
help: Replace for loop with dict comprehension
484+
226 |
485+
227 | def f():
486+
228 | # comment duplication in target (https://github.com/astral-sh/ruff/issues/18787)
487+
- result = {}
488+
- for (
489+
- k # comment
490+
- ) in ["a", "b", "c"]:
491+
- result[k] = k
492+
229 + # comment
493+
230 + result = {k: k for k in ["a", "b", "c"]}
443494
note: This is an unsafe fix and may change runtime behavior

0 commit comments

Comments
 (0)