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 0000000000..8f7dc91396 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_lambda.py @@ -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) 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 0000000000..d1ccf9f055 --- /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/mod.rs b/crates/ruff_linter/src/checkers/ast/analyze/mod.rs index ed623b1abf..dd9ec2fbfd 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/mod.rs @@ -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; diff --git a/crates/ruff_linter/src/checkers/ast/deferred.rs b/crates/ruff_linter/src/checkers/ast/deferred.rs index c29f61354e..8296215452 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 0cd527f761..2ff7bad73c 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; @@ -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 = 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); diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 5546ce4a1f..cc8b718471 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -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), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index d1807b2695..f6fcb09177 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -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( diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 30ede8337a..54731009ee 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -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; 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 0000000000..82e5c8decd --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_lambda.rs @@ -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::>(); + + // 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; + } + } + 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::>(); + 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); + } +} 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 0000000000..c4acb1801d --- /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 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 + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 2bf5cbe8b1..14f2362217 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3057,6 +3057,8 @@ "PLW", "PLW0", "PLW01", + "PLW010", + "PLW0108", "PLW012", "PLW0120", "PLW0127",