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.

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

Merged
merged 4 commits into from Oct 20, 2023

Conversation

clemux
Copy link
Contributor

@clemux clemux commented Oct 13, 2023

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:

  • added a ruff_linter::checkers::ast::analyze::lambda module
  • build a vec of visited lambdas in visit_deferred_lambdas
  • call analyze::lambda on the vec after they all have been visited

Building 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

df.apply(lambda x: str(x))

Good

df.apply(str)

Test Plan

Added unit test and snapshot.
Manually compared pylint and ruff output on pylint's test cases.

References

@github-actions
Copy link

github-actions bot commented Oct 13, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

@clemux clemux force-pushed the plw0108_unnecessary_lambda branch 2 times, most recently from 1a6812b to a0a77e5 Compare October 13, 2023 20:22
@charliermarsh
Copy link
Member

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?

@clemux
Copy link
Contributor Author

clemux commented Oct 14, 2023

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.

@clemux
Copy link
Contributor Author

clemux commented Oct 15, 2023

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));
Copy link
Contributor Author

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

Copy link
Member

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

@@ -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),
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@charliermarsh
Copy link
Member

Gonna review this today, sorry for the delay.

};

for name in names {
if let Some(binding_id) = checker.semantic().resolve_name(name) {
Copy link
Member

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.

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Oct 20, 2023
@charliermarsh charliermarsh enabled auto-merge (squash) October 20, 2023 17:04
@charliermarsh charliermarsh enabled auto-merge (squash) October 20, 2023 17:15
@charliermarsh charliermarsh merged commit b107204 into astral-sh:main Oct 20, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants