Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid C417 for lambda with default and variadic parameters #6752

Merged
merged 5 commits into from
Aug 22, 2023
Merged

Avoid C417 for lambda with default and variadic parameters #6752

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6e1e8ea
Avoid `C417` autofix for `lambda` with default parameters
dhruvmanila Aug 22, 2023
8840895
Avoid autofix if `*args`/`**kwargs` is present
dhruvmanila Aug 22, 2023
45f538c
Avoid flagging instead
dhruvmanila Aug 22, 2023
ee94fb8
Update snapshots
dhruvmanila Aug 22, 2023
2045f75
Move back to `is_some_and` pattern
dhruvmanila Aug 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions crates/ruff/resources/test/fixtures/flake8_comprehensions/C417.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,6 @@
_ = f"{set(map(lambda x: x % 2 == 0, nums))}"
_ = f"{dict(map(lambda v: (v, v**2), nums))}"

# Error, but unfixable.
# For simple expressions, this could be: `(x if x else 1 for x in nums)`.
# For more complex expressions, this would differ: `(x + 2 if x else 3 for x in nums)`.
map(lambda x=1: x, nums)

# False negatives.
map(lambda x=2, y=1: x + y, nums, nums)
set(map(lambda x, y: x, nums, nums))
Expand All @@ -37,3 +32,8 @@ def func(arg1: int, arg2: int = 4):

# Error: the `x` is overridden by the inner lambda.
map(lambda x: lambda x: x, range(4))

# Ok because of the default parameters, and variadic arguments.
map(lambda x=1: x, nums)
map(lambda *args: len(args), range(4))
map(lambda **kwargs: len(kwargs), range(4))
65 changes: 49 additions & 16 deletions crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,22 @@ pub(crate) fn unnecessary_map(
return;
};

if parameters
.as_ref()
.is_some_and(|parameters| late_binding(parameters, body))
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
{
return;
if let Some(parameters) = parameters.as_ref() {
if late_binding(parameters, body) {
return;
}

if parameters
.posonlyargs
.iter()
.chain(&parameters.args)
.chain(&parameters.kwonlyargs)
.any(|param| param.default.is_some())
|| parameters.vararg.is_some()
|| parameters.kwarg.is_some()
{
return;
}
}
}
ObjectType::List | ObjectType::Set => {
Expand Down Expand Up @@ -137,11 +148,22 @@ pub(crate) fn unnecessary_map(
return;
};

if parameters
.as_ref()
.is_some_and(|parameters| late_binding(parameters, body))
{
return;
if let Some(parameters) = parameters.as_ref() {
if late_binding(parameters, body) {
return;
}

if parameters
.posonlyargs
.iter()
.chain(&parameters.args)
.chain(&parameters.kwonlyargs)
.any(|param| param.default.is_some())
|| parameters.vararg.is_some()
|| parameters.kwarg.is_some()
{
return;
}
}
}
ObjectType::Dict => {
Expand Down Expand Up @@ -177,14 +199,25 @@ pub(crate) fn unnecessary_map(
return;
}

if parameters
.as_ref()
.is_some_and(|parameters| late_binding(parameters, body))
{
return;
if let Some(parameters) = parameters.as_ref() {
if late_binding(parameters, body) {
return;
}

if parameters
.posonlyargs
.iter()
.chain(&parameters.args)
.chain(&parameters.kwonlyargs)
.any(|param| param.default.is_some())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also avoid starred arguments and kwargs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

|| parameters.vararg.is_some()
|| parameters.kwarg.is_some()
{
return;
}
}
}
}
};

let mut diagnostic = Diagnostic::new(UnnecessaryMap { object_type }, expr.range());
if checker.patch(diagnostic.kind.rule()) {
Expand Down
40 changes: 17 additions & 23 deletions ...ke8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C417_C417.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ C417.py:15:8: C417 [*] Unnecessary `map` usage (rewrite using a `set` comprehens
15 |+_ = f"{ {x % 2 == 0 for x in nums} }"
16 16 | _ = f"{dict(map(lambda v: (v, v**2), nums))}"
17 17 |
18 18 | # Error, but unfixable.
18 18 | # False negatives.

C417.py:16:8: C417 [*] Unnecessary `map` usage (rewrite using a `dict` comprehension)
|
Expand All @@ -235,7 +235,7 @@ C417.py:16:8: C417 [*] Unnecessary `map` usage (rewrite using a `dict` comprehen
16 | _ = f"{dict(map(lambda v: (v, v**2), nums))}"
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C417
17 |
18 | # Error, but unfixable.
18 | # False negatives.
|
= help: Replace `map` with a `dict` comprehension

Expand All @@ -246,33 +246,27 @@ C417.py:16:8: C417 [*] Unnecessary `map` usage (rewrite using a `dict` comprehen
16 |-_ = f"{dict(map(lambda v: (v, v**2), nums))}"
16 |+_ = f"{ {v: v**2 for v in nums} }"
17 17 |
18 18 | # Error, but unfixable.
19 19 | # For simple expressions, this could be: `(x if x else 1 for x in nums)`.
18 18 | # False negatives.
19 19 | map(lambda x=2, y=1: x + y, nums, nums)

C417.py:21:1: C417 Unnecessary `map` usage (rewrite using a generator expression)
C417.py:34:1: C417 [*] Unnecessary `map` usage (rewrite using a generator expression)
|
19 | # For simple expressions, this could be: `(x if x else 1 for x in nums)`.
20 | # For more complex expressions, this would differ: `(x + 2 if x else 3 for x in nums)`.
21 | map(lambda x=1: x, nums)
| ^^^^^^^^^^^^^^^^^^^^^^^^ C417
22 |
23 | # False negatives.
|
= help: Replace `map` with a generator expression

C417.py:39:1: C417 [*] Unnecessary `map` usage (rewrite using a generator expression)
|
38 | # Error: the `x` is overridden by the inner lambda.
39 | map(lambda x: lambda x: x, range(4))
33 | # Error: the `x` is overridden by the inner lambda.
34 | map(lambda x: lambda x: x, range(4))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C417
35 |
36 | # Ok because of the default parameters, and variadic arguments.
|
= help: Replace `map` with a generator expression

ℹ Suggested fix
36 36 | map(lambda x: lambda: x, range(4))
37 37 |
38 38 | # Error: the `x` is overridden by the inner lambda.
39 |-map(lambda x: lambda x: x, range(4))
39 |+(lambda x: x for x in range(4))
31 31 | map(lambda x: lambda: x, range(4))
32 32 |
33 33 | # Error: the `x` is overridden by the inner lambda.
34 |-map(lambda x: lambda x: x, range(4))
34 |+(lambda x: x for x in range(4))
35 35 |
36 36 | # Ok because of the default parameters, and variadic arguments.
37 37 | map(lambda x=1: x, nums)