[perflint] Add PERF403
#5313
[perflint] Add PERF403
#5313
Conversation
PR Check ResultsEcosystemℹ️ ecosystem check detected changes. (+21, -0, 0 error(s)) airflow (+13, -0)
+ airflow/dag_processing/manager.py:997:17: PERF403 Use a dictionary comprehension instead of a for-loop + airflow/executors/kubernetes_executor_utils.py:137:17: PERF403 Use a dictionary comprehension instead of a for-loop + airflow/providers/exasol/hooks/exasol.py:66:17: PERF403 Use a dictionary comprehension instead of a for-loop + airflow/providers/google/cloud/hooks/bigquery.py:3063:13: PERF403 Use a dictionary comprehension instead of a for-loop + airflow/providers/google/cloud/transfers/gcs_to_bigquery.py:708:17: PERF403 Use a dictionary comprehension instead of a for-loop + airflow/providers/mysql/hooks/mysql.py:163:17: PERF403 Use a dictionary comprehension instead of a for-loop + airflow/providers/postgres/hooks/postgres.py:149:17: PERF403 Use a dictionary comprehension instead of a for-loop + airflow/providers/smtp/hooks/smtp.py:282:17: PERF403 Use a dictionary comprehension instead of a for-loop + airflow/utils/email.py:215:13: PERF403 Use a dictionary comprehension instead of a for-loop + dev/breeze/src/airflow_breeze/utils/run_utils.py:185:13: PERF403 Use a dictionary comprehension instead of a for-loop + setup.py:524:9: PERF403 Use a dictionary comprehension instead of a for-loop + tests/test_utils/config.py:57:13: PERF403 Use a dictionary comprehension instead of a for-loop + tests/test_utils/config.py:82:13: PERF403 Use a dictionary comprehension instead of a for-loop bokeh (+3, -0)
+ src/bokeh/core/has_props.py:128:13: PERF403 Use a dictionary comprehension instead of a for-loop + src/bokeh/core/property/wrappers.py:517:21: PERF403 Use a dictionary comprehension instead of a for-loop + src/bokeh/models/sources.py:283:13: PERF403 Use a dictionary comprehension instead of a for-loop zulip (+5, -0)
+ analytics/views/stats.py:456:9: PERF403 Use a dictionary comprehension instead of a for-loop + analytics/views/stats.py:531:9: PERF403 Use a dictionary comprehension instead of a for-loop + zerver/lib/response.py:128:9: PERF403 Use a dictionary comprehension instead of a for-loop + zerver/views/auth.py:371:13: PERF403 Use a dictionary comprehension instead of a for-loop + zerver/views/development/registration.py:28:9: PERF403 Use a dictionary comprehension instead of a for-loop
BenchmarkLinuxWindows |
| for idx, name in enumerate(fruit): | ||
| result[idx] = name # PERF403 |
There was a problem hiding this comment.
(I wonder similar to PERF402 why there isn't the separation for dictionary, I'm assuming the corresponding rule for dictionary doesn't exists upstream as well).
There was a problem hiding this comment.
Yeah don't know why it's split into copy() and comprehension for lists but only one for dicts. My implementation was just a straight copy from https://github.com/tonybaloney/perflint/blob/main/perflint/comprehension_checker.py
As we are already deviating with the if conditional checks, we could add a separate rule PERF404 if that has value.
|
The delay on this one is that I'm worried about false positives. Most of the examples surfaced in the ecosystem CI checks seem to be false positives. I don't think we have a great way to solve this right now. The best we can do, probably, is check if the variable to which we're assigning was assigned the empty dictionary immediately prior to the loop. That's probably a necessary precondition to merging here just based on what I see in the ecosystem CI -- wdyt, @qdegraaf? |
@charliermarsh it's been on my TODO list to look at this one again since I got wind of PERF401 and PERF402 being quite false positive heavy as well, wanted to pick it up after the Dlint/ReDoS thing but will prioritise this over that now. I am away until after the weekend but plan after that is to:
Happy to wait on merging until that's sorted and we are satisfied with the precision. If we can't get it to perform like we want I'll check in again then to see what we want to do with it. I'll set the PR to draft until I get further. |
|
@charliermarsh For false positives based on whether the dictionary is empty. def foo():
result = {"a": 1, "b": 2}
fruit = ["apple", "pear", "orange"]
for idx, name in enumerate(fruit):
if idx % 2:
result[idx] = nameas We can add the heuristic. But checking other def f():
items = [1, 2, 3, 4]
result = [5, 6]
for i in items:
result.append(i * i) flags: So we'd have to add the heuristic to all |
|
I think this is okay to flag: def f():
items = [1, 2, 3, 4]
result = [5, 6]
for i in items:
result.append(i * i) Because it can be expressed as What if the heuristic was: check if the variable was assigned to a dictionary literal prior to the loop? |
|
Ah yeah, of course, check. Lot of context switches muddying up the mind recently. I'll implement that heuristic. Two questions:
d = {}
d[1] = 2
for x in some_other_list:
d[x] = x? If so, is there an example of a rule where we do something similar, I can take inspiration from? I assume it involves scanning the parent scope for the binding and then checking that assignment. But unsure what the most efficient way is to get the relative position of that assignment (resolving the range to line number somehow?) |
Summary
Adds
PERF403mirroringW8403from https://github.com/tonybaloney/perflintImplementation is similar to original/upstream implementation, with the exception of not yet checking whether the
ExprSubscriptvalue is a dictionary type value. I tried adding this but ended up doing a lot ofagain for what felt like very little added value.
Open to any feedback/input to do this more efficiently. If there is no more efficient way to do this, also let me know and I'll see if I can make some kind of helper or util for this, as I've bumped into this a few times now with various rules.
I've set the violation to only flag the first append call in a long
if-elsestatement. Happy to change this to some other location or make it multiple violations if that makes more sense.Test Plan
Fixtures were added based on perflint tests
Issue Links
Refers: #4789