Skip to content

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

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

Merged
merged 4 commits into from
Oct 20, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
125378a
[pylint] Implement unnecessary-lambda (W0108)
clemux Oct 13, 2023
f03cca5
Merge branch 'main' into plw0108_unnecessary_lambda
charliermarsh Oct 20, 2023
6666b59
Tweaks
charliermarsh Oct 20, 2023
aa270ca
Move around deferred logic
charliermarsh Oct 20, 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
Prev Previous commit
Next Next commit
Tweaks
  • Loading branch information
@charliermarsh
charliermarsh committed Oct 20, 2023
commit 6666b59936d8592048c54414da0509b765509bbe
4 changes: 4 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_lambda.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,7 @@
_ = 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)
249 changes: 135 additions & 114 deletions crates/ruff_linter/src/rules/pylint/rules/unnecessary_lambda.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
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, Arguments, Expr, ExprLambda, Parameter, ParameterWithDefault,
};
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 lambdas whose body is a function call on the same arguments as the lambda itself.
/// Checks for `lambda` definitions that consist of a single function call
/// with the same arguments as the `lambda` itself.
///
/// ## Why is this bad?
/// Such lambda expressions are in all but a few cases replaceable with the function being called
/// in the body of the lambda.
/// 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
Expand All @@ -24,32 +24,13 @@ use crate::checkers::ast::Checker;
/// ```python
/// df.apply(str)
/// ```

#[violation]
pub struct UnnecessaryLambda;

impl Violation for UnnecessaryLambda {
#[derive_message_formats]
fn message(&self) -> String {
format!("Lambda may not be necessary")
}
}

/// Identify all `Expr::Name` nodes in an AST.
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);
format!("Lambda may be unnecessary; consider inlining inner function")
}
}

Expand All @@ -60,129 +41,169 @@ pub(crate) fn unnecessary_lambda(checker: &mut Checker, lambda: &ExprLambda) {
body,
range: _,
} = lambda;
// At least one the parameters of the lambda include a default value. We can't know if the
// defaults provided by the lambda are the same as the defaults provided by the function
// being called.
if parameters.as_ref().map_or(false, |parameters| {

// 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;
}
if let Expr::Call(ast::ExprCall {
arguments, func, ..
}) = body.as_ref()
{
let Arguments { args, keywords, .. } = arguments;

// don't check chained calls
if let Expr::Attribute(ast::ExprAttribute { value, .. }) = func.as_ref() {
if let Expr::Call(_) = value.as_ref() {
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<_>>();

let call_starargs: Vec<&Expr> = args
.iter()
.filter_map(|arg| {
if let Expr::Starred(ast::ExprStarred { value, .. }) = arg {
Some(value.as_ref())
} else {
None
// 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;
}
}
})
.collect::<Vec<_>>();
Some(kwarg) => {
let [call_kwarg] = &call_kwargs[..] else {
return;
};

let call_kwargs: Vec<&Expr> = keywords.iter().map(|kw| &kw.value).collect::<Vec<_>>();
let Expr::Name(ast::ExprName { id, .. }) = call_kwarg else {
return;
};

let call_ordinary_args: Vec<&Expr> = args
.iter()
.filter(|arg| !matches!(arg, Expr::Starred(_)))
.collect::<Vec<_>>();

if let Some(parameters) = parameters.as_ref() {
if let Some(kwarg) = parameters.kwarg.as_ref() {
if call_kwargs.is_empty()
|| call_kwargs.iter().any(|kw| {
if let Expr::Name(ast::ExprName { id, .. }) = kw {
id.as_str() != kwarg.name.as_str()
} else {
true
}
})
{
return;
if id.as_str() != kwarg.name.as_str() {
return;
}
}
} else if !call_kwargs.is_empty() {
return;
}
if let Some(vararg) = parameters.vararg.as_ref() {
if call_starargs.is_empty()
|| call_starargs.iter().any(|arg| {
if let Expr::Name(ast::ExprName { id, .. }) = arg {
id.as_str() != vararg.name.as_str()
} else {
true
}
})
{
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;
}
}
} else if !call_starargs.is_empty() {
return;
}

let lambda_ordinary_params: Vec<&Parameter> = parameters
// Ex) `lambda x, y: func(x, y)`
let lambda_posargs: Vec<&Parameter> = parameters
.args
.iter()
.map(|ParameterWithDefault { parameter, .. }| parameter)
.collect::<Vec<_>>();

if call_ordinary_args.len() != lambda_ordinary_params.len() {
if call_posargs.len() != lambda_posargs.len() {
return;
}

let params_args = lambda_ordinary_params
.iter()
.zip(call_ordinary_args.iter())
.collect::<Vec<_>>();

for (param, arg) in params_args {
if let Expr::Name(ast::ExprName { id, .. }) = arg {
if id.as_str() != param.name.as_str() {
return;
}
} else {
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;
}
}
} else if !call_starargs.is_empty()
|| !keywords.is_empty()
|| !call_ordinary_args.is_empty()
{
return;
}
}

// The lambda is necessary if it uses its parameter in the function it is
// calling in the lambda's body
// e.g. lambda foo: (func1 if foo else func2)(foo)

let mut finder = NameFinder { names: vec![] };
// 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 expr_name in finder.names {
if let Some(binding_id) = checker.semantic().resolve_name(expr_name) {
let binding = checker.semantic().binding(binding_id);
if checker.semantic().is_current_scope(binding.scope) {
return;
}
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.

let binding = checker.semantic().binding(binding_id);
if checker.semantic().is_current_scope(binding.scope) {
return;
}
}
}

checker
.diagnostics
.push(Diagnostic::new(UnnecessaryLambda, lambda.range()));
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);
}
}
16 changes: 8 additions & 8 deletions ...es/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0108_unnecessary_lambda.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
unnecessary_lambda.py:1:5: PLW0108 Lambda may not be necessary
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 not be necessary
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]
Expand All @@ -17,7 +17,7 @@ unnecessary_lambda.py:2:5: PLW0108 Lambda may not be necessary
4 | _ = lambda *args: f(*args) # [unnecessary-lambda]
|

unnecessary_lambda.py:4:5: PLW0108 Lambda may not be necessary
unnecessary_lambda.py:4:5: PLW0108 Lambda may be unnecessary; consider inlining inner function
|
2 | _ = lambda x, y: min(x, y) # [unnecessary-lambda]
3 |
Expand All @@ -27,7 +27,7 @@ unnecessary_lambda.py:4:5: PLW0108 Lambda may not be necessary
6 | _ = lambda *args, **kwargs: f(*args, **kwargs) # [unnecessary-lambda]
|

unnecessary_lambda.py:5:5: PLW0108 Lambda may not be necessary
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]
Expand All @@ -36,7 +36,7 @@ unnecessary_lambda.py:5:5: PLW0108 Lambda may not be necessary
7 | _ = lambda x, y, z, *args, **kwargs: f(x, y, z, *args, **kwargs) # [unnecessary-lambda]
|

unnecessary_lambda.py:6:5: PLW0108 Lambda may not be necessary
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]
Expand All @@ -45,7 +45,7 @@ unnecessary_lambda.py:6:5: PLW0108 Lambda may not be necessary
7 | _ = lambda x, y, z, *args, **kwargs: f(x, y, z, *args, **kwargs) # [unnecessary-lambda]
|

unnecessary_lambda.py:7:5: PLW0108 Lambda may not be necessary
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]
Expand All @@ -55,7 +55,7 @@ unnecessary_lambda.py:7:5: PLW0108 Lambda may not be necessary
9 | _ = lambda x: f(lambda x: x)(x) # [unnecessary-lambda]
|

unnecessary_lambda.py:9:5: PLW0108 Lambda may not be necessary
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 |
Expand All @@ -64,7 +64,7 @@ unnecessary_lambda.py:9:5: PLW0108 Lambda may not be necessary
10 | _ = lambda x, y: f(lambda x, y: x + y)(x, y) # [unnecessary-lambda]
|

unnecessary_lambda.py:10:5: PLW0108 Lambda may not be necessary
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]
Expand Down