Avoid `C417` for `lambda` with default and variadic parameters #6752

dhruvmanila · 2023-08-22T04:21:59Z

Summary

Avoid C417 for lambda with default and variadic parameters.

Test Plan

cargo test and checking if it generates any autofix errors as test cases
for lambda with default parameters already exists.

fixes: #6715

dhruvmanila · 2023-08-22T04:22:10Z

Current dependencies on/for this PR:

main
- PR Avoid C417 for lambda with default and variadic parameters #6752 👈

This comment was auto-generated by Graphite.

dhruvmanila · 2023-08-22T04:22:57Z

crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs

+                parameters
+                    .posonlyargs
+                    .iter()
+                    .chain(&parameters.args)
+                    .chain(&parameters.kwonlyargs)
+                    .any(|param| param.default.is_some())


Would it be useful to have this as a method on Parameters node? (has_defaults?)

I think what we have here is reasonable, maybe if we find ourselves doing this in at least one other place.

github-actions · 2023-08-22T04:36:59Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      3.7±0.13ms    10.9 MB/sec    1.01      3.8±0.10ms    10.8 MB/sec
formatter/numpy/ctypeslib.py               1.00   785.3±18.37µs    21.2 MB/sec    1.01   791.8±20.09µs    21.0 MB/sec
formatter/numpy/globals.py                 1.00     84.2±3.18µs    35.0 MB/sec    1.02     85.9±3.08µs    34.4 MB/sec
formatter/pydantic/types.py                1.00  1532.1±54.52µs    16.6 MB/sec    1.01  1541.2±43.76µs    16.5 MB/sec
linter/all-rules/large/dataset.py          1.00     11.1±0.22ms     3.7 MB/sec    1.01     11.2±0.23ms     3.6 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.02      3.1±0.07ms     5.3 MB/sec    1.00      3.1±0.06ms     5.4 MB/sec
linter/all-rules/numpy/globals.py          1.01   435.5±10.76µs     6.8 MB/sec    1.00   430.9±10.94µs     6.8 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.9±0.18ms     4.3 MB/sec    1.03      6.1±0.17ms     4.2 MB/sec
linter/default-rules/large/dataset.py      1.00      6.1±0.15ms     6.7 MB/sec    1.01      6.1±0.13ms     6.7 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1340.7±41.72µs    12.4 MB/sec    1.01  1351.3±30.23µs    12.3 MB/sec
linter/default-rules/numpy/globals.py      1.02    157.1±3.86µs    18.8 MB/sec    1.00    154.3±4.06µs    19.1 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.8±0.07ms     9.2 MB/sec    1.00      2.8±0.08ms     9.2 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      3.8±0.07ms    10.7 MB/sec    1.03      3.9±0.08ms    10.5 MB/sec
formatter/numpy/ctypeslib.py               1.00   767.7±19.84µs    21.7 MB/sec    1.02   783.7±19.09µs    21.2 MB/sec
formatter/numpy/globals.py                 1.00     78.8±1.49µs    37.5 MB/sec    1.05     82.7±3.02µs    35.7 MB/sec
formatter/pydantic/types.py                1.00  1533.0±25.87µs    16.6 MB/sec    1.03  1574.3±37.07µs    16.2 MB/sec
linter/all-rules/large/dataset.py          1.00     13.1±0.22ms     3.1 MB/sec    1.01     13.2±0.14ms     3.1 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.5±0.05ms     4.7 MB/sec    1.01      3.6±0.05ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.00    443.8±7.18µs     6.6 MB/sec    1.00    445.5±5.46µs     6.6 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.8±0.11ms     3.8 MB/sec    1.03      7.0±0.13ms     3.6 MB/sec
linter/default-rules/large/dataset.py      1.00      7.2±0.08ms     5.7 MB/sec    1.04      7.5±0.09ms     5.4 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1516.6±15.92µs    11.0 MB/sec    1.04  1578.9±36.20µs    10.5 MB/sec
linter/default-rules/numpy/globals.py      1.00    175.6±3.90µs    16.8 MB/sec    1.03    180.1±4.17µs    16.4 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.2±0.04ms     7.9 MB/sec    1.03      3.3±0.05ms     7.6 MB/sec

charliermarsh · 2023-08-22T14:10:19Z

crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs

+                    .iter()
+                    .chain(&parameters.args)
+                    .chain(&parameters.kwonlyargs)
+                    .any(|param| param.default.is_some())


Should we also avoid starred arguments and kwargs?

Yeah, we should otherwise the fix isn't correct either:

map(lambda *x: len(x), nums) # Gets fixed to (len(x) for _ in nums) # where's x?

charliermarsh · 2023-08-22T17:16:34Z

crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs


    let mut diagnostic = Diagnostic::new(UnnecessaryMap { object_type }, expr.range());
-    if checker.patch(diagnostic.kind.rule()) {
+    if checker.patch(diagnostic.kind.rule()) && !has_default_parameters {


I think I'd vote not to flag this at all -- wdyt? Also should simplify the implementation.

Interestingly I was just playing around with that asking the same question. Why should we flag it if it isn't fixable (not from an implementation side but from the user side)?

Yeah, I'd go for the same route of not flagging this in the first place. Also, the fix would not yield the same result:

In [1]: nums = [[0, 1, 2, 3], [4, 5, 6]] In [2]: list(map(lambda x, *y: y, nums)) Out[2]: [(), ()] In [3]: [y for x, *y in nums] Out[3]: [[1, 2, 3], [5, 6]]

(Sorry for not mentioning this on the first review.)

No problem at all :)

charliermarsh

Thx!

crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs

Avoid C417 autofix for lambda with default parameters

6e1e8ea

dhruvmanila commented Aug 22, 2023

View reviewed changes

dhruvmanila requested a review from charliermarsh August 22, 2023 04:23

charliermarsh reviewed Aug 22, 2023

View reviewed changes

Avoid autofix if *args/**kwargs is present

8840895

charliermarsh reviewed Aug 22, 2023

View reviewed changes

Avoid flagging instead

45f538c

dhruvmanila changed the title ~~Avoid C417 autofix for lambda with default parameters~~ Avoid C417 for lambda with default and variadic parameters Aug 22, 2023

Update snapshots

ee94fb8

charliermarsh approved these changes Aug 22, 2023

View reviewed changes

crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs Show resolved Hide resolved

Move back to is_some_and pattern

2045f75

dhruvmanila merged commit 2e00983 into main Aug 22, 2023
16 checks passed

dhruvmanila deleted the dhruv/c417 branch August 22, 2023 19:08

chenrui333 mentioned this pull request Aug 26, 2023

ruff 0.0.286 Homebrew/homebrew-core#140518

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid `C417` for `lambda` with default and variadic parameters #6752

Avoid `C417` for `lambda` with default and variadic parameters #6752

dhruvmanila commented Aug 22, 2023 •

edited

Loading

dhruvmanila commented Aug 22, 2023

dhruvmanila Aug 22, 2023

charliermarsh Aug 22, 2023

github-actions bot commented Aug 22, 2023 •

edited

Loading

charliermarsh Aug 22, 2023

dhruvmanila Aug 22, 2023

charliermarsh Aug 22, 2023

dhruvmanila Aug 22, 2023

dhruvmanila Aug 22, 2023

charliermarsh Aug 22, 2023

dhruvmanila Aug 22, 2023

charliermarsh left a comment

Avoid C417 for lambda with default and variadic parameters #6752

Avoid C417 for lambda with default and variadic parameters #6752

Conversation

dhruvmanila commented Aug 22, 2023 • edited Loading

Summary

Test Plan

dhruvmanila commented Aug 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 22, 2023 • edited Loading

PR Check Results

Ecosystem

Benchmark

Linux

Windows

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh left a comment

Choose a reason for hiding this comment

Avoid `C417` for `lambda` with default and variadic parameters #6752

Avoid `C417` for `lambda` with default and variadic parameters #6752

dhruvmanila commented Aug 22, 2023 •

edited

Loading

github-actions bot commented Aug 22, 2023 •

edited

Loading