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

Changes from 1 commit
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
60 changes: 42 additions & 18 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 @@ -84,7 +84,7 @@ pub(crate) fn unnecessary_map(
return;
}

match object_type {
let has_default_parameters = match object_type {
ObjectType::Generator => {
// Exclude the parent if already matched by other arms.
if parent
Expand All @@ -103,11 +103,19 @@ 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;
}

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

} else {
false
}
}
ObjectType::List | ObjectType::Set => {
Expand Down Expand Up @@ -137,11 +145,19 @@ 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;
}

parameters
.posonlyargs
.iter()
.chain(&parameters.args)
.chain(&parameters.kwonlyargs)
.any(|param| param.default.is_some())
} else {
false
}
}
ObjectType::Dict => {
Expand Down Expand Up @@ -177,17 +193,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;
}

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?

} else {
false
}
}
}
};

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 {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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)?

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, 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]]

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem at all :)

diagnostic.try_set_fix(|| {
fixes::fix_unnecessary_map(
checker.locator(),
Expand Down