From 1a6812bc3d3ac2724fc6800fc68c232936d7e361 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] [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 | 16 +- 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, 361 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..64df411004bac2 --- /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) \ No newline at end of file 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..884f7cad229cbb 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); } @@ -1981,7 +1992,6 @@ pub(crate) fn check_ast( let allocator = typed_arena::Arena::new(); checker.visit_deferred_string_type_definitions(&allocator); 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..26d24a99f13ab8 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::Unspecified, 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",