Skip to content

Commit

Permalink
[pylint] Implement unnecessary-lambda (W0108) (#7953)
Browse files Browse the repository at this point in the history
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

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

### Good

```python
df.apply(str)
```

## Test Plan

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

## References

- [pylint
documentation](https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/unnecessary-lambda.html)
- [pylint
implementation](https://github.com/pylint-dev/pylint/blob/main/pylint/checkers/base/basic_checker.py#L521-L587)
 - #970
  • Loading branch information
clemux authored Oct 20, 2023
1 parent bc49492 commit b107204
Show file tree
Hide file tree
Showing 11 changed files with 388 additions and 7 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
_ = lambda: print() # [unnecessary-lambda]
_ = lambda x, y: min(x, y) # [unnecessary-lambda]

_ = lambda *args: f(*args) # [unnecessary-lambda]
_ = lambda **kwargs: f(**kwargs) # [unnecessary-lambda]
_ = lambda *args, **kwargs: f(*args, **kwargs) # [unnecessary-lambda]
_ = lambda x, y, z, *args, **kwargs: f(x, y, z, *args, **kwargs) # [unnecessary-lambda]

_ = lambda x: f(lambda x: x)(x) # [unnecessary-lambda]
_ = lambda x, y: f(lambda x, y: x + y)(x, y) # [unnecessary-lambda]

# default value in lambda parameters
_ = lambda x=42: print(x)

# lambda body is not a call
_ = lambda x: x

# ignore chained calls
_ = lambda: chained().call()

# lambda has kwargs but not the call
_ = lambda **kwargs: f()

# lambda has kwargs and the call has kwargs named differently
_ = lambda **kwargs: f(**dict([('forty-two', 42)]))

# lambda has kwargs and the call has unnamed kwargs
_ = lambda **kwargs: f(**{1: 2})

# lambda has starred parameters but not the call
_ = lambda *args: f()

# lambda has starred parameters and the call has starargs named differently
_ = lambda *args: f(*list([3, 4]))
# lambda has starred parameters and the call has unnamed starargs
_ = lambda *args: f(*[3, 4])

# lambda has parameters but not the call
_ = lambda x: f()
_ = lambda *x: f()
_ = lambda **x: f()

# lambda parameters and call args are not the same length
_ = lambda x, y: f(x)

# lambda parameters and call args are not in the same order
_ = lambda x, y: f(y, x)

# lambda parameters and call args are not the same
_ = lambda x: f(y)

# the call uses the lambda parameters
_ = lambda x: x(x)
_ = lambda x, y: x(x, y)
_ = lambda x: z(lambda y: x + y)(x)

# lambda uses an additional keyword
_ = lambda *args: f(*args, y=1)
_ = lambda *args: f(*args, y=x)
23 changes: 23 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/deferred_lambdas.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
use ruff_python_ast::Expr;

use crate::checkers::ast::Checker;
use crate::codes::Rule;
use crate::rules::pylint;

/// Run lint rules over all deferred lambdas in the [`SemanticModel`].
pub(crate) fn deferred_lambdas(checker: &mut Checker) {
while !checker.deferred.lambdas.is_empty() {
let lambdas = std::mem::take(&mut checker.deferred.lambdas);
for snapshot in lambdas {
checker.semantic.restore(snapshot);

let Some(Expr::Lambda(lambda)) = checker.semantic.current_expression() else {
unreachable!("Expected Expr::Lambda");
};

if checker.enabled(Rule::UnnecessaryLambda) {
pylint::rules::unnecessary_lambda(checker, lambda);
}
}
}
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
pub(super) use bindings::bindings;
pub(super) use comprehension::comprehension;
pub(super) use deferred_for_loops::deferred_for_loops;
pub(super) use deferred_lambdas::deferred_lambdas;
pub(super) use deferred_scopes::deferred_scopes;
pub(super) use definitions::definitions;
pub(super) use except_handler::except_handler;
Expand All @@ -15,6 +16,7 @@ pub(super) use unresolved_references::unresolved_references;
mod bindings;
mod comprehension;
mod deferred_for_loops;
mod deferred_lambdas;
mod deferred_scopes;
mod definitions;
mod except_handler;
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/checkers/ast/deferred.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,6 @@ pub(crate) struct Deferred<'a> {
pub(crate) future_type_definitions: Vec<(&'a Expr, Snapshot)>,
pub(crate) type_param_definitions: Vec<(&'a Expr, Snapshot)>,
pub(crate) functions: Vec<Snapshot>,
pub(crate) lambdas: Vec<(&'a Expr, Snapshot)>,
pub(crate) lambdas: Vec<Snapshot>,
pub(crate) for_loops: Vec<Snapshot>,
}
18 changes: 12 additions & 6 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ use ruff_python_parser::typing::{parse_type_annotation, AnnotationKind};
use ruff_python_semantic::analyze::{typing, visibility};
use ruff_python_semantic::{
BindingFlags, BindingId, BindingKind, Exceptions, Export, FromImport, Globals, Import, Module,
ModuleKind, NodeId, ScopeId, ScopeKind, SemanticModel, SemanticModelFlags, StarImport,
SubmoduleImport,
ModuleKind, NodeId, ScopeId, ScopeKind, SemanticModel, SemanticModelFlags, Snapshot,
StarImport, SubmoduleImport,
};
use ruff_python_stdlib::builtins::{BUILTINS, MAGIC_GLOBALS};
use ruff_source_file::Locator;
Expand Down Expand Up @@ -902,7 +902,7 @@ where
}

self.semantic.push_scope(ScopeKind::Lambda(lambda));
self.deferred.lambdas.push((expr, self.semantic.snapshot()));
self.deferred.lambdas.push(self.semantic.snapshot());
}
Expr::IfExp(ast::ExprIfExp {
test,
Expand Down Expand Up @@ -1850,16 +1850,17 @@ impl<'a> Checker<'a> {

fn visit_deferred_lambdas(&mut self) {
let snapshot = self.semantic.snapshot();
let mut deferred: Vec<Snapshot> = Vec::with_capacity(self.deferred.lambdas.len());
while !self.deferred.lambdas.is_empty() {
let lambdas = std::mem::take(&mut self.deferred.lambdas);
for (expr, snapshot) in lambdas {
for snapshot in lambdas {
self.semantic.restore(snapshot);

if let Expr::Lambda(ast::ExprLambda {
if let Some(Expr::Lambda(ast::ExprLambda {
parameters,
body,
range: _,
}) = expr
})) = self.semantic.current_expression()
{
if let Some(parameters) = parameters {
self.visit_parameters(parameters);
Expand All @@ -1868,8 +1869,12 @@ impl<'a> Checker<'a> {
} else {
unreachable!("Expected Expr::Lambda");
}

deferred.push(snapshot);
}
}
// Reset the deferred lambdas, so we can analyze them later on.
self.deferred.lambdas = deferred;
self.semantic.restore(snapshot);
}

Expand Down Expand Up @@ -1989,6 +1994,7 @@ pub(crate) fn check_ast(
checker.visit_exports();

// Check docstrings, bindings, and unresolved references.
analyze::deferred_lambdas(&mut checker);
analyze::deferred_for_loops(&mut checker);
analyze::definitions(&mut checker);
analyze::bindings(&mut checker);
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "R6201") => (RuleGroup::Preview, rules::pylint::rules::LiteralMembership),
#[allow(deprecated)]
(Pylint, "R6301") => (RuleGroup::Nursery, rules::pylint::rules::NoSelfUse),
(Pylint, "W0108") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryLambda),
(Pylint, "W0120") => (RuleGroup::Stable, rules::pylint::rules::UselessElseOnLoop),
(Pylint, "W0127") => (RuleGroup::Stable, rules::pylint::rules::SelfAssigningVariable),
(Pylint, "W0129") => (RuleGroup::Stable, rules::pylint::rules::AssertOnStringLiteral),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ mod tests {
#[test_case(Rule::MisplacedBareRaise, Path::new("misplaced_bare_raise.py"))]
#[test_case(Rule::LiteralMembership, Path::new("literal_membership.py"))]
#[test_case(Rule::GlobalAtModuleLevel, Path::new("global_at_module_level.py"))]
#[test_case(Rule::UnnecessaryLambda, Path::new("unnecessary_lambda.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ pub(crate) use type_name_incorrect_variance::*;
pub(crate) use type_param_name_mismatch::*;
pub(crate) use unexpected_special_method_signature::*;
pub(crate) use unnecessary_direct_lambda_call::*;
pub(crate) use unnecessary_lambda::*;
pub(crate) use unspecified_encoding::*;
pub(crate) use useless_else_on_loop::*;
pub(crate) use useless_import_alias::*;
Expand Down Expand Up @@ -121,6 +122,7 @@ mod type_name_incorrect_variance;
mod type_param_name_mismatch;
mod unexpected_special_method_signature;
mod unnecessary_direct_lambda_call;
mod unnecessary_lambda;
mod unspecified_encoding;
mod useless_else_on_loop;
mod useless_import_alias;
Expand Down
Loading

0 comments on commit b107204

Please sign in to comment.