[perflint] Add perflint plugin, add first rule PERF102

[qdegraaf]

Summary

Adds boilerplate for implementing the perflint plugin, plus a first rule.

• Add check that items() is called on a dict

  • Upstream implementation in perflint does not do this, neither does a similar check in Ruff (SIM118). Code checks for some heuristics (no args, return value is an iterable of tuples etc., unpacked into two vars one of which is dummied, etc.) which should minimise the risk. See also discussion in Discord. @charliermarsh it is an open question whether or not it is a good idea to autofix in this situation, I'll leave that call to you. Can remove if too risky.

• Add autofix

Test Plan

Fixture added for PER8102

Issue link

Refers: #4789

[github-actions]

PR Check Results

Ecosystem

ℹ️ ecosystem check detected changes. (+8, -0, 0 error(s))

airflow (+6, -0)

+ airflow/utils/log/trigger_handler.py:115:21: PERF102 [*] When using only the values of a dict use the `values()` method
+ tests/kubernetes/test_pod_generator.py:564:21: PERF102 [*] When using only the values of a dict use the `values()` method
+ tests/providers/amazon/aws/waiters/test_custom_waiters.py:101:24: PERF102 [*] When using only the keys of a dict use the `keys()` method
+ tests/providers/amazon/aws/waiters/test_custom_waiters.py:72:24: PERF102 [*] When using only the keys of a dict use the `keys()` method
+ tests/serialization/test_dag_serialization.py:380:21: PERF102 [*] When using only the values of a dict use the `values()` method
+ tests/www/views/test_views_acl.py:124:24: PERF102 [*] When using only the keys of a dict use the `keys()` method

bokeh (+2, -0)

+ src/bokeh/embed/util.py:333:30: PERF102 [*] When using only the values of a dict use the `values()` method
+ src/bokeh/models/sources.py:541:25: PERF102 [*] When using only the values of a dict use the `values()` method

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.01      6.4±0.02ms     6.4 MB/sec    1.00      6.3±0.01ms     6.5 MB/sec
formatter/numpy/ctypeslib.py               1.01   1327.8±3.94µs    12.5 MB/sec    1.00   1316.9±2.62µs    12.6 MB/sec
formatter/numpy/globals.py                 1.01    128.9±0.32µs    22.9 MB/sec    1.00    127.5±0.29µs    23.1 MB/sec
formatter/pydantic/types.py                1.00      2.6±0.00ms     9.9 MB/sec    1.00      2.6±0.02ms     9.9 MB/sec
linter/all-rules/large/dataset.py          1.00     13.7±0.08ms     3.0 MB/sec    1.00     13.8±0.10ms     3.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.3±0.01ms     5.0 MB/sec    1.00      3.3±0.01ms     5.0 MB/sec
linter/all-rules/numpy/globals.py          1.00    414.0±1.18µs     7.1 MB/sec    1.00    414.7±1.41µs     7.1 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.8±0.03ms     4.4 MB/sec    1.00      5.8±0.02ms     4.4 MB/sec
linter/default-rules/large/dataset.py      1.00      6.7±0.02ms     6.1 MB/sec    1.00      6.7±0.02ms     6.1 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1443.1±3.90µs    11.5 MB/sec    1.00   1446.6±3.63µs    11.5 MB/sec
linter/default-rules/numpy/globals.py      1.00    159.2±0.25µs    18.5 MB/sec    1.01    160.1±0.70µs    18.4 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.0±0.02ms     8.4 MB/sec    1.00      3.0±0.01ms     8.4 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      6.5±0.02ms     6.3 MB/sec    1.07      7.0±2.25ms     5.8 MB/sec
formatter/numpy/ctypeslib.py               1.00   1286.5±7.73µs    12.9 MB/sec    1.01  1297.9±35.90µs    12.8 MB/sec
formatter/numpy/globals.py                 1.01    124.4±9.32µs    23.7 MB/sec    1.00    123.0±1.46µs    24.0 MB/sec
formatter/pydantic/types.py                1.00      2.6±0.01ms     9.7 MB/sec    1.00      2.6±0.01ms     9.7 MB/sec
linter/all-rules/large/dataset.py          1.00     13.1±0.05ms     3.1 MB/sec    1.06     14.0±0.04ms     2.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.3±0.01ms     5.0 MB/sec    1.04      3.4±0.01ms     4.9 MB/sec
linter/all-rules/numpy/globals.py          1.00    342.3±3.35µs     8.6 MB/sec    1.04    354.4±2.11µs     8.3 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.7±0.03ms     4.5 MB/sec    1.05      5.9±0.03ms     4.3 MB/sec
linter/default-rules/large/dataset.py      1.00      6.8±0.02ms     5.9 MB/sec    1.13      7.8±0.04ms     5.2 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1382.7±5.95µs    12.0 MB/sec    1.11  1537.6±14.58µs    10.8 MB/sec
linter/default-rules/numpy/globals.py      1.00    149.0±0.92µs    19.8 MB/sec    1.08    160.8±2.16µs    18.4 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.0±0.02ms     8.4 MB/sec    1.11      3.4±0.02ms     7.6 MB/sec

[auscompgeek]

FWIW perflint's code prefix seems to be W8xxx

[qdegraaf]

FWIW perflint's code prefix seems to be W8xxx

Ah yeah, was worried that might clash with Pycodestyle plugin but that stops at W6XX both in the Ruff port and in the original upstream implementation by the looks of it. I'll make the prefix W8

[dhruvmanila]

nit: Can we use the let ... else { return } pattern instead of if let ... { ... } to avoid nesting. It'll also make the code linear and easier to read :)

I think you're already using this pattern in a few other places.

    let Expr::Call(ast::ExprCall { func, args, .. }) = iter else {
    	return;
    }

[qdegraaf]

Good suggestion! Done it for a bunch of if let's now. Could go one or two steps further (and now maybe refactoring to some fn that can be called on both elements instead of having 4 branches of code for both scenarios for both elements). But already liking its readability much more so your call AFAIAC

[dhruvmanila]

Shouldn't this be outside the if let Expr::Name(... block? The same question for other branches as well.

[qdegraaf]

Depends on what we want the behaviour to be. Current implementation does not continue if it finds a violation for the first of the two args. So a case of

for _, _ in d.items():
    pass

Will trigger a violation only for the first arg. Do we think it should be the responsibility of this rule to check for a completely ignored .items() call? And if so what do we want to do with autofixes?

[dhruvmanila]

This is an interesting case. perflint seems to be triggering for both key and value place which doesn't seem correct to me. I guess the reason someone would want to use a loop like this is to do something n number of times where n is the dictionary length. I would suggest to ignore such cases plus I don't think it's the responsibility of this rule to check it.

[dhruvmanila]

I think we could convert this into a function which can accept a subset parameter which could be an enum instead of a string and an expr parameter which would be the other expression node.

We could also simplify the extraction of fix_val to avoid creating an invalid state where the other element isn't a Name or Tuple node.

Suggested change

	let mut fix_val: &str = "";
	if let Expr::Name(ast::ExprName { id, .. }) = &elts[1] {
	fix_val = id.as_str();
	}
	if let Expr::Tuple(ast::ExprTuple {
	range: val_range, ..
	}) = &elts[1]
	{
	fix_val = checker.locator.slice(*val_range);
	}
	diagnostic.set_fix(Fix::automatic_edits(
	Edit::range_replacement(".values".to_string(), method_range),
	[Edit::range_replacement(fix_val.to_string(), *tuple_range)],
	));
	let fix_val = match &elts[1] {
	Expr::Name(ast::ExprName { id, .. }) => id.as_str(),
	Expr::Tuple(ast::ExprTuple {
	range: val_range, ..
	}) => checker.locator.slice(*val_range),
	_ => panic!("Expected Expr::Name | Expr::Tuple"),
	};
	diagnostic.set_fix(Fix::automatic_edits(
	Edit::range_replacement(".values".to_string(), method_range),
	[Edit::range_replacement(fix_val.to_string(), *tuple_range)],
	));

[dhruvmanila]

I'm not sure about the prefix here as we would want a 3 letter abbreviation for the plugin name (PER, PRF?). It's ok to deviate from the actual rule code for consistency :)

\cc @charliermarsh

[charliermarsh]

Maybe PERF? We've started to use some four-letter prefixes.

[charliermarsh]

Will give this a pass and merge today.

[qdegraaf]

Will give this a pass and merge today.

Let me know if any input is requested. Happy to step aside though, earliest I can get round to @dhruvmanila's comments and extracting some stuff to a function is tomorrow evening (GMT+1)