mirrors/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-11-02 04:48:06 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

[pylint] Implement unnecessary-lambda (W0108) (#7953)

Browse source 
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?
https://github.com/pylint-dev/pylint/issues/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)
 - https://github.com/astral-sh/ruff/issues/970
This commit is contained in:
Clément Schreiner 2023-10-20 19:25:24 +02:00 • committed by GitHub
parent bc49492085
commit b1072049bf
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 388 additions and 7 deletions
Download patch file Download diff file Expand all files Collapse all files

59
crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_lambda.py vendored Normal file
View file

@ -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
crates/ruff_linter/src/checkers/ast/analyze/deferred_lambdas.rs Normal file
View file

@ -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
crates/ruff_linter/src/checkers/ast/analyze/mod.rs
View file

@ -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;
@ -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;

2
crates/ruff_linter/src/checkers/ast/deferred.rs
View file

@ -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
crates/ruff_linter/src/checkers/ast/mod.rs
View file

@ -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;
@ -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,
@ -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);
@ -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);
}
@ -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);

1
crates/ruff_linter/src/codes.rs
View file

@ -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),

1
crates/ruff_linter/src/rules/pylint/mod.rs
View file

@ -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(

2
crates/ruff_linter/src/rules/pylint/rules/mod.rs
View file

@ -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::*;
@ -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;

209
crates/ruff_linter/src/rules/pylint/rules/unnecessary_lambda.rs Normal file
View file

@ -0,0 +1,209 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::visitor::Visitor;
use ruff_python_ast::{self as ast, visitor, Expr, ExprLambda, Parameter, ParameterWithDefault};
use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;
/// ## What it does
/// Checks for `lambda` definitions that consist of a single function call
/// with the same arguments as the `lambda` itself.
///
/// ## Why is this bad?
/// When a `lambda` is used to wrap a function call, and merely propagates
/// the `lambda` arguments to that function, it can typically be replaced with
/// the function itself, removing a level of indirection.
///
/// ## Example
/// ```python
/// df.apply(lambda x: str(x))
/// ```
///
/// Use instead:
/// ```python
/// df.apply(str)
/// ```
#[violation]
pub struct UnnecessaryLambda;
impl Violation for UnnecessaryLambda {
#[derive_message_formats]
fn message(&self) -> String {
format!("Lambda may be unnecessary; consider inlining inner function")
}
}
/// PLW0108
pub(crate) fn unnecessary_lambda(checker: &mut Checker, lambda: &ExprLambda) {
let ExprLambda {
parameters,
body,
range: _,
} = lambda;
// The lambda should consist of a single function call.
let Expr::Call(ast::ExprCall {
arguments, func, ..
}) = body.as_ref()
else {
return;
};
// Ignore call chains.
if let Expr::Attribute(ast::ExprAttribute { value, .. }) = func.as_ref() {
if value.is_call_expr() {
return;
}
}
// If at least one of the lambda parameters has a default value, abort. We can't know if the
// defaults provided by the lambda are the same as the defaults provided by the inner
// function.
if parameters.as_ref().is_some_and(|parameters| {
parameters
.args
.iter()
.any(|ParameterWithDefault { default, .. }| default.is_some())
}) {
return;
}
match parameters.as_ref() {
None => {
if !arguments.is_empty() {
return;
}
}
Some(parameters) => {
// Collect all starred arguments (e.g., `lambda *args: func(*args)`).
let call_varargs: Vec<&Expr> = arguments
.args
.iter()
.filter_map(|arg| {
if let Expr::Starred(ast::ExprStarred { value, .. }) = arg {
Some(value.as_ref())
} else {
None
}
})
.collect::<Vec<_>>();
// Collect all keyword arguments (e.g., `lambda x, y: func(x=x, y=y)`).
let call_kwargs: Vec<&Expr> = arguments
.keywords
.iter()
.map(|kw| &kw.value)
.collect::<Vec<_>>();
// Collect all positional arguments (e.g., `lambda x, y: func(x, y)`).
let call_posargs: Vec<&Expr> = arguments
.args
.iter()
.filter(|arg| !arg.is_starred_expr())
.collect::<Vec<_>>();
// Ex) `lambda **kwargs: func(**kwargs)`
match parameters.kwarg.as_ref() {
None => {
if !call_kwargs.is_empty() {
return;
}
}
Some(kwarg) => {
let [call_kwarg] = &call_kwargs[..] else {
return;
};
let Expr::Name(ast::ExprName { id, .. }) = call_kwarg else {
return;
};
if id.as_str() != kwarg.name.as_str() {
return;
}
}
}
// Ex) `lambda *args: func(*args)`
match parameters.vararg.as_ref() {
None => {
if !call_varargs.is_empty() {
return;
}
}
Some(vararg) => {
let [call_vararg] = &call_varargs[..] else {
return;
};
let Expr::Name(ast::ExprName { id, .. }) = call_vararg else {
return;
};
if id.as_str() != vararg.name.as_str() {
return;
}
}
}
// Ex) `lambda x, y: func(x, y)`
let lambda_posargs: Vec<&Parameter> = parameters
.args
.iter()
.map(|ParameterWithDefault { parameter, .. }| parameter)
.collect::<Vec<_>>();
if call_posargs.len() != lambda_posargs.len() {
return;
}
for (param, arg) in lambda_posargs.iter().zip(call_posargs.iter()) {
let Expr::Name(ast::ExprName { id, .. }) = arg else {
return;
};
if id.as_str() != param.name.as_str() {
return;
}
}
}
}
// The lambda is necessary if it uses one of its parameters _as_ the function call.
// Ex) `lambda x, y: x(y)`
let names = {
let mut finder = NameFinder::default();
finder.visit_expr(func);
finder.names
};
for name in names {
if let Some(binding_id) = checker.semantic().resolve_name(name) {
let binding = checker.semantic().binding(binding_id);
if checker.semantic().is_current_scope(binding.scope) {
return;
}
}
}
checker
.diagnostics
.push(Diagnostic::new(UnnecessaryLambda, lambda.range()));
}
/// Identify all `Expr::Name` nodes in an AST.
#[derive(Debug, Default)]
struct NameFinder<'a> {
/// A map from identifier to defining expression.
names: Vec<&'a ast::ExprName>,
}
impl<'a, 'b> Visitor<'b> for NameFinder<'a>
where
'b: 'a,
{
fn visit_expr(&mut self, expr: &'a Expr) {
if let Expr::Name(expr_name) = expr {
self.names.push(expr_name);
}
visitor::walk_expr(self, expr);
}
}

76
crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0108_unnecessary_lambda.py.snap Normal file
View file

@ -0,0 +1,76 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
unnecessary_lambda.py:1:5: PLW0108 Lambda may be unnecessary; consider inlining inner function
|
1 | _ = lambda: print() # [unnecessary-lambda]
| ^^^^^^^^^^^^^^^ PLW0108
2 | _ = lambda x, y: min(x, y) # [unnecessary-lambda]
|
unnecessary_lambda.py:2:5: PLW0108 Lambda may be unnecessary; consider inlining inner function
|
1 | _ = lambda: print() # [unnecessary-lambda]
2 | _ = lambda x, y: min(x, y) # [unnecessary-lambda]
| ^^^^^^^^^^^^^^^^^^^^^^ PLW0108
3 |
4 | _ = lambda *args: f(*args) # [unnecessary-lambda]
|
unnecessary_lambda.py:4:5: PLW0108 Lambda may be unnecessary; consider inlining inner function
|
2 | _ = lambda x, y: min(x, y) # [unnecessary-lambda]
3 |
4 | _ = lambda *args: f(*args) # [unnecessary-lambda]
| ^^^^^^^^^^^^^^^^^^^^^^ PLW0108
5 | _ = lambda **kwargs: f(**kwargs) # [unnecessary-lambda]
6 | _ = lambda *args, **kwargs: f(*args, **kwargs) # [unnecessary-lambda]
|
unnecessary_lambda.py:5:5: PLW0108 Lambda may be unnecessary; consider inlining inner function
|
4 | _ = lambda *args: f(*args) # [unnecessary-lambda]
5 | _ = lambda **kwargs: f(**kwargs) # [unnecessary-lambda]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0108
6 | _ = lambda *args, **kwargs: f(*args, **kwargs) # [unnecessary-lambda]
7 | _ = lambda x, y, z, *args, **kwargs: f(x, y, z, *args, **kwargs) # [unnecessary-lambda]
|
unnecessary_lambda.py:6:5: PLW0108 Lambda may be unnecessary; consider inlining inner function
|
4 | _ = lambda *args: f(*args) # [unnecessary-lambda]
5 | _ = lambda **kwargs: f(**kwargs) # [unnecessary-lambda]
6 | _ = lambda *args, **kwargs: f(*args, **kwargs) # [unnecessary-lambda]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0108
7 | _ = lambda x, y, z, *args, **kwargs: f(x, y, z, *args, **kwargs) # [unnecessary-lambda]
|
unnecessary_lambda.py:7:5: PLW0108 Lambda may be unnecessary; consider inlining inner function
|
5 | _ = lambda **kwargs: f(**kwargs) # [unnecessary-lambda]
6 | _ = lambda *args, **kwargs: f(*args, **kwargs) # [unnecessary-lambda]
7 | _ = lambda x, y, z, *args, **kwargs: f(x, y, z, *args, **kwargs) # [unnecessary-lambda]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0108
8 |
9 | _ = lambda x: f(lambda x: x)(x) # [unnecessary-lambda]
|
unnecessary_lambda.py:9:5: PLW0108 Lambda may be unnecessary; consider inlining inner function
|
7 | _ = lambda x, y, z, *args, **kwargs: f(x, y, z, *args, **kwargs) # [unnecessary-lambda]
8 |
9 | _ = lambda x: f(lambda x: x)(x) # [unnecessary-lambda]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0108
10 | _ = lambda x, y: f(lambda x, y: x + y)(x, y) # [unnecessary-lambda]
|
unnecessary_lambda.py:10:5: PLW0108 Lambda may be unnecessary; consider inlining inner function
|
9 | _ = lambda x: f(lambda x: x)(x) # [unnecessary-lambda]
10 | _ = lambda x, y: f(lambda x, y: x + y)(x, y) # [unnecessary-lambda]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW0108
11 |
12 | # default value in lambda parameters
|