From 125378ad3a8f28d1f3b798dc907124bb51f37373 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Schreiner?= Date: Fri, 13 Oct 2023 21:02:02 +0200 Subject: [PATCH 1/3] [pylint] Implement unnecessary-lambda (W0108) --- .../fixtures/pylint/unnecessary_lambda.py | 55 +++++ .../src/checkers/ast/analyze/lambda.rs | 21 ++ .../src/checkers/ast/analyze/mod.rs | 2 + crates/ruff_linter/src/checkers/ast/mod.rs | 17 +- crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 1 + .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + .../rules/pylint/rules/unnecessary_lambda.rs | 188 ++++++++++++++++++ ..._tests__PLW0108_unnecessary_lambda.py.snap | 76 +++++++ ruff.schema.json | 2 + 10 files changed, 362 insertions(+), 3 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_lambda.py create mode 100644 crates/ruff_linter/src/checkers/ast/analyze/lambda.rs create mode 100644 crates/ruff_linter/src/rules/pylint/rules/unnecessary_lambda.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0108_unnecessary_lambda.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_lambda.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_lambda.py new file mode 100644 index 00000000000000..862c502ff15025 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_lambda.py @@ -0,0 +1,55 @@ +_ = 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) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/lambda.rs b/crates/ruff_linter/src/checkers/ast/analyze/lambda.rs new file mode 100644 index 00000000000000..1dc2787ad5731f --- /dev/null +++ b/crates/ruff_linter/src/checkers/ast/analyze/lambda.rs @@ -0,0 +1,21 @@ +use crate::checkers::ast::Checker; +use crate::codes::Rule; +use crate::rules::pylint; +use ruff_python_ast::{self as ast, Expr}; + +pub(crate) fn lambda(expr: &Expr, checker: &mut Checker) { + match expr { + Expr::Lambda( + lambda @ ast::ExprLambda { + parameters: _, + body: _, + range: _, + }, + ) => { + if checker.enabled(Rule::UnnecessaryLambda) { + pylint::rules::unnecessary_lambda(checker, lambda); + } + } + _ => unreachable!("Expected Expr::Lambda"), + } +} diff --git a/crates/ruff_linter/src/checkers/ast/analyze/mod.rs b/crates/ruff_linter/src/checkers/ast/analyze/mod.rs index ed623b1abfdc6a..961c4b0eef9343 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/mod.rs @@ -5,6 +5,7 @@ pub(super) use deferred_scopes::deferred_scopes; pub(super) use definitions::definitions; pub(super) use except_handler::except_handler; pub(super) use expression::expression; +pub(super) use lambda::lambda; pub(super) use module::module; pub(super) use parameter::parameter; pub(super) use parameters::parameters; @@ -19,6 +20,7 @@ mod deferred_scopes; mod definitions; mod except_handler; mod expression; +mod lambda; mod module; mod parameter; mod parameters; diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index 26944ce0f0fc41..91b5f6523da7ba 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -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; @@ -1844,6 +1844,7 @@ impl<'a> Checker<'a> { fn visit_deferred_lambdas(&mut self) { let snapshot = self.semantic.snapshot(); + let mut visited_lambdas: Vec<(&Expr, Snapshot)> = vec![]; while !self.deferred.lambdas.is_empty() { let lambdas = std::mem::take(&mut self.deferred.lambdas); for (expr, snapshot) in lambdas { @@ -1859,11 +1860,21 @@ impl<'a> Checker<'a> { self.visit_parameters(parameters); } self.visit_expr(body); + + visited_lambdas.push((expr, snapshot)); } else { unreachable!("Expected Expr::Lambda"); } } } + + // all deferred lambdas must be visited before we can analyze them, because they might + // be nested + for (expr, snapshot) in visited_lambdas { + self.semantic.restore(snapshot); + analyze::lambda(expr, self); + } + self.semantic.restore(snapshot); } @@ -1980,8 +1991,8 @@ pub(crate) fn check_ast( checker.visit_deferred_type_param_definitions(); let allocator = typed_arena::Arena::new(); checker.visit_deferred_string_type_definitions(&allocator); - checker.visit_exports(); + checker.visit_exports(); // Check docstrings, bindings, and unresolved references. analyze::deferred_for_loops(&mut checker); analyze::definitions(&mut checker); diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index f0b9a808040516..50067f1f86855c 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -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::Preview, rules::pylint::rules::UnnecessaryLambda), (Pylint, "W0120") => (RuleGroup::Unspecified, rules::pylint::rules::UselessElseOnLoop), (Pylint, "W0127") => (RuleGroup::Unspecified, rules::pylint::rules::SelfAssigningVariable), (Pylint, "W0129") => (RuleGroup::Unspecified, rules::pylint::rules::AssertOnStringLiteral), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index aee24e4e620374..7a079b10f3aef7 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -134,6 +134,7 @@ mod tests { )] #[test_case(Rule::BadDunderMethodName, Path::new("bad_dunder_method_name.py"))] #[test_case(Rule::NoSelfUse, Path::new("no_self_use.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( diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 7643b55b52140b..ef218718700cb6 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -51,6 +51,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 useless_else_on_loop::*; pub(crate) use useless_import_alias::*; pub(crate) use useless_return::*; @@ -110,6 +111,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 useless_else_on_loop; mod useless_import_alias; mod useless_return; diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_lambda.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_lambda.rs new file mode 100644 index 00000000000000..23faf06b0cf549 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_lambda.rs @@ -0,0 +1,188 @@ +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_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. +/// +/// ## 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. +/// +/// ## 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 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); + } +} + +/// PLW0108 +pub(crate) fn unnecessary_lambda(checker: &mut Checker, lambda: &ExprLambda) { + let ExprLambda { + parameters, + 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| { + 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() { + return; + } + } + + 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::>(); + + let call_kwargs: Vec<&Expr> = keywords.iter().map(|kw| &kw.value).collect::>(); + + let call_ordinary_args: Vec<&Expr> = args + .iter() + .filter(|arg| !matches!(arg, Expr::Starred(_))) + .collect::>(); + + 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; + } + } 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; + } + } else if !call_starargs.is_empty() { + return; + } + + let lambda_ordinary_params: Vec<&Parameter> = parameters + .args + .iter() + .map(|ParameterWithDefault { parameter, .. }| parameter) + .collect::>(); + + if call_ordinary_args.len() != lambda_ordinary_params.len() { + return; + } + + let params_args = lambda_ordinary_params + .iter() + .zip(call_ordinary_args.iter()) + .collect::>(); + + for (param, arg) in params_args { + if let Expr::Name(ast::ExprName { id, .. }) = arg { + if id.as_str() != param.name.as_str() { + return; + } + } else { + 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![] }; + finder.visit_expr(func); + + 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; + } + } + } + + checker + .diagnostics + .push(Diagnostic::new(UnnecessaryLambda, lambda.range())); + } +} diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0108_unnecessary_lambda.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0108_unnecessary_lambda.py.snap new file mode 100644 index 00000000000000..1bb0bc2e163a38 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0108_unnecessary_lambda.py.snap @@ -0,0 +1,76 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +unnecessary_lambda.py:1:5: PLW0108 Lambda may not be necessary + | +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 + | +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 not be necessary + | +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 not be necessary + | +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 not be necessary + | +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 not be necessary + | +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 not be necessary + | + 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 not be necessary + | + 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 + | + + diff --git a/ruff.schema.json b/ruff.schema.json index df646ec363f9fd..bfa5453def158c 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2980,6 +2980,8 @@ "PLW", "PLW0", "PLW01", + "PLW010", + "PLW0108", "PLW012", "PLW0120", "PLW0127", From 6666b59936d8592048c54414da0509b765509bbe Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 20 Oct 2023 12:57:09 -0400 Subject: [PATCH 2/3] Tweaks --- .../fixtures/pylint/unnecessary_lambda.py | 4 + .../rules/pylint/rules/unnecessary_lambda.rs | 249 ++++++++++-------- ..._tests__PLW0108_unnecessary_lambda.py.snap | 16 +- 3 files changed, 147 insertions(+), 122 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_lambda.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_lambda.py index 862c502ff15025..8f7dc913967f3f 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_lambda.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_lambda.py @@ -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) diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_lambda.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_lambda.rs index 23faf06b0cf549..82e5c8decd46b1 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_lambda.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_lambda.rs @@ -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 @@ -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") } } @@ -60,10 +41,26 @@ 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() @@ -71,118 +68,142 @@ pub(crate) fn unnecessary_lambda(checker: &mut Checker, lambda: &ExprLambda) { }) { 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::>(); - 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::>(); + + // 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::>(); + + // Ex) `lambda **kwargs: func(**kwargs)` + match parameters.kwarg.as_ref() { + None => { + if !call_kwargs.is_empty() { + return; + } } - }) - .collect::>(); + Some(kwarg) => { + let [call_kwarg] = &call_kwargs[..] else { + return; + }; - let call_kwargs: Vec<&Expr> = keywords.iter().map(|kw| &kw.value).collect::>(); + 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::>(); - - 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::>(); - - 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::>(); - - 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) { + 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); } } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0108_unnecessary_lambda.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0108_unnecessary_lambda.py.snap index 1bb0bc2e163a38..c4acb1801dfeb8 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0108_unnecessary_lambda.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0108_unnecessary_lambda.py.snap @@ -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] @@ -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 | @@ -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] @@ -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] @@ -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] @@ -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 | @@ -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] From aa270ca0a8ba5e4b635aee107a082a6cc5ae93a1 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 20 Oct 2023 13:14:50 -0400 Subject: [PATCH 3/3] Move around deferred logic --- .../checkers/ast/analyze/deferred_lambdas.rs | 23 ++++++++++++++++ .../src/checkers/ast/analyze/lambda.rs | 21 --------------- .../src/checkers/ast/analyze/mod.rs | 4 +-- .../ruff_linter/src/checkers/ast/deferred.rs | 2 +- crates/ruff_linter/src/checkers/ast/mod.rs | 27 ++++++++----------- 5 files changed, 37 insertions(+), 40 deletions(-) create mode 100644 crates/ruff_linter/src/checkers/ast/analyze/deferred_lambdas.rs delete mode 100644 crates/ruff_linter/src/checkers/ast/analyze/lambda.rs diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_lambdas.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_lambdas.rs new file mode 100644 index 00000000000000..d1ccf9f055a2a1 --- /dev/null +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_lambdas.rs @@ -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); + } + } + } +} diff --git a/crates/ruff_linter/src/checkers/ast/analyze/lambda.rs b/crates/ruff_linter/src/checkers/ast/analyze/lambda.rs deleted file mode 100644 index 1dc2787ad5731f..00000000000000 --- a/crates/ruff_linter/src/checkers/ast/analyze/lambda.rs +++ /dev/null @@ -1,21 +0,0 @@ -use crate::checkers::ast::Checker; -use crate::codes::Rule; -use crate::rules::pylint; -use ruff_python_ast::{self as ast, Expr}; - -pub(crate) fn lambda(expr: &Expr, checker: &mut Checker) { - match expr { - Expr::Lambda( - lambda @ ast::ExprLambda { - parameters: _, - body: _, - range: _, - }, - ) => { - if checker.enabled(Rule::UnnecessaryLambda) { - pylint::rules::unnecessary_lambda(checker, lambda); - } - } - _ => unreachable!("Expected Expr::Lambda"), - } -} diff --git a/crates/ruff_linter/src/checkers/ast/analyze/mod.rs b/crates/ruff_linter/src/checkers/ast/analyze/mod.rs index 961c4b0eef9343..dd9ec2fbfd2ce1 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/mod.rs @@ -1,11 +1,11 @@ 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; pub(super) use expression::expression; -pub(super) use lambda::lambda; pub(super) use module::module; pub(super) use parameter::parameter; pub(super) use parameters::parameters; @@ -16,11 +16,11 @@ 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; mod expression; -mod lambda; mod module; mod parameter; mod parameters; diff --git a/crates/ruff_linter/src/checkers/ast/deferred.rs b/crates/ruff_linter/src/checkers/ast/deferred.rs index c29f61354ef7c2..829621545241e0 100644 --- a/crates/ruff_linter/src/checkers/ast/deferred.rs +++ b/crates/ruff_linter/src/checkers/ast/deferred.rs @@ -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, - pub(crate) lambdas: Vec<(&'a Expr, Snapshot)>, + pub(crate) lambdas: Vec, pub(crate) for_loops: Vec, } diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index 726256cbd94010..2ff7bad73c09eb 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -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,37 +1850,31 @@ impl<'a> Checker<'a> { fn visit_deferred_lambdas(&mut self) { let snapshot = self.semantic.snapshot(); - let mut visited_lambdas: Vec<(&Expr, Snapshot)> = vec![]; + let mut deferred: Vec = 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); } self.visit_expr(body); - - visited_lambdas.push((expr, snapshot)); } else { unreachable!("Expected Expr::Lambda"); } - } - } - // all deferred lambdas must be visited before we can analyze them, because they might - // be nested - for (expr, snapshot) in visited_lambdas { - self.semantic.restore(snapshot); - analyze::lambda(expr, self); + deferred.push(snapshot); + } } - + // Reset the deferred lambdas, so we can analyze them later on. + self.deferred.lambdas = deferred; self.semantic.restore(snapshot); } @@ -1997,9 +1991,10 @@ pub(crate) fn check_ast( checker.visit_deferred_type_param_definitions(); let allocator = typed_arena::Arena::new(); checker.visit_deferred_string_type_definitions(&allocator); - 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);