[`perflint`] Implement fix for `manual-dict-comprehension` (`PERF403`) by w0nder1ng · Pull Request #16719 · astral-sh/ruff

w0nder1ng · 2025-03-14T03:53:39Z

Summary

This change adds an auto-fix for manual dict comprehensions. It also copies many of the improvements from #13919 (and associated PRs fixing issues with it), and moves some of the utility functions from manual_list_comprehension.rs into a separate helpers.rs to be used in both.

Test Plan

I added a preview test case to showcase the new fix and added a test case in PERF403.py to make sure lines with semicolons function. I didn't yet make similar tests to the ones I added earlier to PERF401.py, but the logic is the same, so it might be good to add those to make sure they work.

github-actions · 2025-03-14T04:10:30Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+6 -8 violations, +0 -0 fixes in 3 projects; 52 projects unchanged)

apache/airflow (+4 -4 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/executors/kubernetes_executor_utils.py:141:17: PERF403 Use `dict.update` instead of a for-loop
- providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/executors/kubernetes_executor_utils.py:141:17: PERF403 Use a dictionary comprehension instead of a for-loop
+ providers/exasol/src/airflow/providers/exasol/hooks/exasol.py:70:17: PERF403 Use `dict.update` instead of a for-loop
- providers/exasol/src/airflow/providers/exasol/hooks/exasol.py:70:17: PERF403 Use a dictionary comprehension instead of a for-loop
+ providers/mysql/src/airflow/providers/mysql/hooks/mysql.py:191:17: PERF403 Use `dict.update` instead of a for-loop
- providers/mysql/src/airflow/providers/mysql/hooks/mysql.py:191:17: PERF403 Use a dictionary comprehension instead of a for-loop
+ providers/postgres/src/airflow/providers/postgres/hooks/postgres.py:171:17: PERF403 Use `dict.update` instead of a for-loop
- providers/postgres/src/airflow/providers/postgres/hooks/postgres.py:171:17: PERF403 Use a dictionary comprehension instead of a for-loop

apache/superset (+2 -2 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ superset/migrations/versions/2021-04-12_12-38_fc3a3a8ff221_migrate_filter_sets_to_new_format.py:122:21: PERF403 Use `dict.update` instead of a for-loop
- superset/migrations/versions/2021-04-12_12-38_fc3a3a8ff221_migrate_filter_sets_to_new_format.py:122:21: PERF403 Use a dictionary comprehension instead of a for-loop
+ superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py:795:21: PERF403 Use `dict.update` instead of a for-loop
- superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py:795:21: PERF403 Use a dictionary comprehension instead of a for-loop

pandas-dev/pandas (+0 -2 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

- pandas/tests/arrays/sparse/test_accessor.py:247:30: RUF043 Pattern passed to `match=` contains metacharacters but is neither escaped nor raw
- pandas/tests/arrays/sparse/test_accessor.py:96:50: RUF043 Pattern passed to `match=` contains metacharacters but is neither escaped nor raw

Changes by rule (2 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PERF403	12	6	6	0	0
RUF043	2	0	2	0	0

Skylion007 · 2025-03-15T15:36:33Z

Slight nit: had a situation where it didn't simplify an annotated dict = dict() builtin to a single line comprehension. Still kept a sepereate call to the dict() builtin function followed by update. Manually fixed that.

    node_to_step: dict[BaseSchedulerNode, int] = dict()
    for step, node in enumerate(nodes):
        node_to_step[node] = step

got transformed to

     node_to_step: dict[BaseSchedulerNode, int] = dict()
     node_to_step.update(...)

w0nder1ng · 2025-03-16T17:33:38Z

I had forgotten about using a function call to make a dict, since it's not typically something I do in my own code. I've now modified the "is empty dict" check to allow builtin function calls for both PERF403 and PERF401.

merge with main

Skylion007 · 2025-03-17T16:19:47Z

Title is wrong, it's PERF403, right?

w0nder1ng · 2025-03-23T23:17:55Z

Do you know why the tests aren't running? The test jobs in my fork of the repo are timing out while waiting for a runner.

ntBre · 2025-03-24T14:25:17Z

I think the most recent commits came in while CI was disabled. You can try pushing another (possibly empty) commit, or closing and reopening the PR to retrigger CI.

Skylion007 · 2025-04-02T13:54:54Z

@ntBre CI is run now.

ntBre · 2025-04-03T20:48:49Z

Thanks for the ping, I'll try to review this tomorrow.

ntBre

Thanks for all of this work! It looks good, and most of my inline comments are just stylistic nits.

For tests, I think I'd like to see ~1 test for each of the (very helpful) comments that document some constraint, if that's not too hard. Is that what you had in mind in the PR summary?

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

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

...les/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF403_PERF403.py.snap

w0nder1ng · 2025-04-07T02:55:24Z

@ntBre I think I've addressed all of your feedback, so please let me know if you need anything else changed.

Also, do you know why the configuration.md output test is failing? I don't think I touched any files that affect it.

ntBre · 2025-04-07T12:54:49Z

Also, do you know why the configuration.md output test is failing? I don't think I touched any files that affect it.

At the very top of the error message it says

Error: docs/configuration.md changed, please run cargo dev generate-all:

I think adding a fix probably changed the docs slightly, and you just need to run cargo dev generate-all to fix it. Some of the diffs look kind of weird, though, so you may also need to merge main or something, but try generate-all first.

ntBre

Thanks! I'd still like to avoid the expect calls, and I added one more suggestion for Diagnostic::with_fix but this looks good to me otherwise, once the tests are passing.

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

w0nder1ng · 2025-04-07T18:33:47Z

Turns out I just needed to merge with main. Did you also want the .expect() in convert_to_dict_comprehension removed?

ntBre · 2025-04-07T21:06:54Z

Turns out I just needed to merge with main. Did you also want the .expect() in convert_to_dict_comprehension removed?

Yes please!

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

w0nder1ng · 2025-04-15T23:33:50Z

@ntBre let me know if any other changes are necessary

ntBre · 2025-04-16T17:08:55Z

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

+        list_binding_value.is_some_and(|binding_value| match binding_value {
+            // `value = []`
+            Expr::List(list_expr) => list_expr.is_empty(),
+            // `value = list()`
+            // This is probably be linted against, but turning it into a list comprehension will also remove it
+            Expr::Call(call) => {
+                checker
+                    .semantic()
+                    .resolve_builtin_symbol(&call.func)
+                    .is_some_and(|name| name == "list")
+                    && call.arguments.is_empty()
+            }
+            _ => false,


Sorry for not noticing this earlier (or for bringing it up again if we already discussed it), but should this be included here? I think these changes (and the corresponding ones on the dict side) are causing the differences in the ecosystem check, and I would kind of like to limit this PR strictly to the autofix.

I would have at least expected some PERF401 snapshots to change with this, since it's obviously affecting ecosystem packages.

We can split it off in a different PR, but it's an ecosystem bug we noticed when testing.

ntBre · 2025-04-18T17:10:28Z

Thanks, I thought we had to revert the Call check in the dict too, but that's in the new preview part anyway.

Skylion007 · 2025-04-19T15:17:24Z

@W0ndering Mind opening a new PR for that ecosystem fix to list Calls as well?

…nsion (`PERF401`) (#17519) This is an implementation of the discussion from #16719. This change will allow list function calls to be replaced with comprehensions: ```python result = list() for i in range(3): result.append(i + 1) # becomes result = [i + 1 for i in range(3)] ``` I added a new test to `PERF401.py` to verify that this fix will now work for `list()`.

w0nder1ng added 4 commits January 7, 2025 23:13

add fix

8e5f474

finalize fix for PERF403 manual dict comprehension

170715f

merge with main

97296f0

fix test output

874d7b5

MichaReiser added the fixes Related to suggested fixes for violations label Mar 14, 2025

w0nder1ng added 2 commits March 16, 2025 17:30

update PERF403 and PERF401 to work with value = dict() and list()

1b441a3

update test output

07753ff

Merge remote-tracking branch 'origin' into perf403_fix

720ff33

merge with main

w0nder1ng changed the title ~~[perflint] implement quick-fix for manual-dict-comprehension (PERF404)~~ [perflint] implement quick-fix for manual-dict-comprehension (PERF403) Mar 17, 2025

empty commit to rerun CI

836d1c7

ntBre self-assigned this Apr 3, 2025

ntBre reviewed Apr 4, 2025

View reviewed changes

w0nder1ng added 4 commits April 6, 2025 22:17

fix nits

3de805e

increase scope of feature gate

791cae8

modify comment ordering for both PERF401 and PERF403

2c30bdf

add tests for relevant constraints

8611782

ntBre approved these changes Apr 7, 2025

View reviewed changes

crates/ruff_linter/src/rules/perflint/rules/manual_dict_comprehension.rs Outdated Show resolved Hide resolved

w0nder1ng added 2 commits April 7, 2025 14:23

remove a .expect() and use Diagnostic::with_fix

60dddb5

Merge branch 'main' into perf403_fix

6077aa3

add debug_assert that target variable binding exists

7085ea5

ntBre approved these changes Apr 7, 2025

View reviewed changes

crates/ruff_linter/src/rules/perflint/rules/manual_dict_comprehension.rs Outdated Show resolved Hide resolved

w0nder1ng added 2 commits April 15, 2025 16:22

make fix return an Option

d3b7068

remove unused anyhow import

30bb035

ntBre approved these changes Apr 16, 2025

View reviewed changes

ntBre changed the title ~~[perflint] implement quick-fix for manual-dict-comprehension (PERF403)~~ [perflint] Implement fix for manual-dict-comprehension (PERF403) Apr 16, 2025

ntBre reviewed Apr 16, 2025

View reviewed changes

remove check that was affecting ecosystem results

22ab9c1

ntBre added the preview Related to preview mode features label Apr 18, 2025

ntBre merged commit 0822145 into astral-sh:main Apr 18, 2025
22 checks passed

w0nder1ng mentioned this pull request Apr 21, 2025

[perflint] Allow list function calls to be replaced with a comprehension (PERF401) #17519

Merged

BrewTestBot mentioned this pull request Apr 24, 2025

ruff 0.11.7 Homebrew/homebrew-core#221359

Merged

Conversation

w0nder1ng commented Mar 14, 2025

Summary

Test Plan

Uh oh!

github-actions bot commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

Skylion007 commented Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

w0nder1ng commented Mar 16, 2025

Uh oh!

Skylion007 commented Mar 17, 2025

Uh oh!

w0nder1ng commented Mar 23, 2025

Uh oh!

ntBre commented Mar 24, 2025

Uh oh!

Skylion007 commented Apr 2, 2025

Uh oh!

ntBre commented Apr 3, 2025

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

w0nder1ng commented Apr 7, 2025

Uh oh!

ntBre commented Apr 7, 2025

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

w0nder1ng commented Apr 7, 2025

Uh oh!

ntBre commented Apr 7, 2025

Uh oh!

Uh oh!

w0nder1ng commented Apr 15, 2025

Uh oh!

ntBre Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

Skylion007 Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre commented Apr 18, 2025

Uh oh!

Uh oh!

Skylion007 commented Apr 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Mar 14, 2025 •

edited

Loading

`ruff-ecosystem` results

Skylion007 commented Mar 15, 2025 •

edited

Loading