-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
[pylint] Implement unnecessary-lambda (W0108) #7953
[pylint] Implement unnecessary-lambda (W0108) #7953
Conversation
d22666b
to
d5f4488
Compare
PR Check ResultsEcosystem✅ ecosystem check detected no changes. |
1a6812b
to
a0a77e5
Compare
Before I get deep into the review, do you mind skimming through some of the ecosystem checks and confirming that the implemented behavior matches Pylint's? |
There are a few less errors with ruff than with pylint in pandas. For example, this: expected2 = frame_or_series(rolling.apply(lambda x: np_func(x, **np_kwargs))) I might be missing something but I'd say it's a bug in pylint. I'll look into it. |
Issue submitted on pylint side: pylint-dev/pylint#9148 |
@@ -1859,11 +1860,21 @@ impl<'a> Checker<'a> { | |||
self.visit_parameters(parameters); | |||
} | |||
self.visit_expr(body); | |||
|
|||
visited_lambdas.push((expr, snapshot)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't expect you to accept that, but I couldn't figure out how to make it work otherwise :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to tweak this a bit, but the idea makes sense :)
crates/ruff_linter/src/codes.rs
Outdated
@@ -254,6 +254,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { | |||
(Pylint, "R5501") => (RuleGroup::Unspecified, rules::pylint::rules::CollapsibleElseIf), | |||
#[allow(deprecated)] | |||
(Pylint, "R6301") => (RuleGroup::Nursery, rules::pylint::rules::NoSelfUse), | |||
(Pylint, "W0108") => (RuleGroup::Unspecified, rules::pylint::rules::UnnecessaryLambda), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New rules should be added in RuleGroup::Preview
then stabilized in a later release. We can improve the naming of the stable rule group :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
a0a77e5
to
125378a
Compare
Gonna review this today, sorry for the delay. |
}; | ||
|
||
for name in names { | ||
if let Some(binding_id) = checker.semantic().resolve_name(name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the reason that lookup_symbol
didn't work here is a bit hard to explain, but consider:
_ = lambda x: f(lambda x: x)(x)
Here, we're trying to see if f(lambda x: x)
contains any references to the x
in _ = lambda x
. If we use lookup_symbol
, it will try to resolve the inner x
relative to the outer scope, that of _ = lambda x
, so it thinks it's a reference to the outer x
. Using resolve_name
ensures that we use the correct binding which was already computed in the binding phase.
This is my first PR and I'm new at rust, so feel free to ask me to rewrite everything if needed ;)
The rule must be called after deferred lambas have been visited because of the last check (whether the lambda parameters are used in the body of the function that's being called). I didn't know where to do it, so I did what I could to be able to work on the rule itself:
ruff_linter::checkers::ast::analyze::lambda
modulevisit_deferred_lambdas
analyze::lambda
on the vec after they all have been visitedBuilding that vec of visited lambdas was necessary so that bindings could be properly resolved in the case of nested lambdas.
Note that there is an open issue in pylint for some false positives, do we need to fix that before merging the rule? pylint-dev/pylint#8192
Also, I did not provide any fixes (yet), maybe we want do avoid merging new rules without fixes?
Summary
Checks for lambdas whose body is a function call on the same arguments as the lambda itself.
Bad
Good
Test Plan
Added unit test and snapshot.
Manually compared pylint and ruff output on pylint's test cases.
References