diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B023.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B023.py index 339c972452..6bf9633701 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B023.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B023.py @@ -183,3 +183,53 @@ for val in range(3): funcs.append(make_func()) + + +# Test cases for issue #15716 - false positives with lambda parameters +for _ in range(3): + [x for x in []] + def func(): + lambda x: x # Should not trigger B023 - x is a lambda parameter + + +# Test case from issue comment - pandas apply (should NOT trigger B023 - apply is safe like map) +import pandas as pd + +data = pd.DataFrame() +data.loc[0, "hex"] = "72756666" +data.loc[1, "hex"] = "75767576" + +def modifier(value): + return chr(int(value, 16)) + + +for _i in range(0, 4): + data[f"v{_i}"] = data["hex"].apply( + lambda x: modifier(x[2 * _i : 2 * _i + 2]), # Should NOT trigger B023 - apply is safe + ) + + +# Test case from issue comment - nested function (should NOT trigger B023 - value is function parameter) +for _ in range(2): + + def add_one(): + def _add_one_inner(value): + return value + 1 # Should NOT trigger B023 - value is function parameter + + return _add_one_inner + + for value in range(5): + result = add_one()(value) + print(result) + + +# nested function that captures loop variable (SHOULD trigger B023) +lst = [] +for value in range(2): + def add_one(): + def _add_one_inner(): + return value + 1 # Should trigger B023 - value is loop variable, not bound + + return _add_one_inner + + lst.append(add_one()) diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/function_uses_loop_variable.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/function_uses_loop_variable.rs index f19a43e633..7ce58290e1 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/function_uses_loop_variable.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/function_uses_loop_variable.rs @@ -3,6 +3,7 @@ use ruff_python_ast::types::Node; use ruff_python_ast::visitor; use ruff_python_ast::visitor::Visitor; use ruff_python_ast::{self as ast, Comprehension, Expr, ExprContext, Stmt}; +use ruff_python_semantic::Modules; use ruff_text_size::Ranged; use crate::Violation; @@ -63,6 +64,14 @@ struct LoadedNamesVisitor<'a> { /// `Visitor` to collect all used identifiers in a statement. impl<'a> Visitor<'a> for LoadedNamesVisitor<'a> { + fn visit_stmt(&mut self, stmt: &'a Stmt) { + // Skip nested function definitions - they are handled separately by `SuspiciousVariablesVisitor` + if stmt.is_function_def_stmt() { + return; + } + visitor::walk_stmt(self, stmt); + } + fn visit_expr(&mut self, expr: &'a Expr) { match expr { Expr::Name(name) => match &name.ctx { @@ -70,15 +79,17 @@ impl<'a> Visitor<'a> for LoadedNamesVisitor<'a> { ExprContext::Store => self.stored.push(name), _ => {} }, + Expr::Lambda(ast::ExprLambda { parameters: _, .. }) => {} _ => visitor::walk_expr(self, expr), } } } -#[derive(Default)] struct SuspiciousVariablesVisitor<'a> { names: Vec<&'a ast::ExprName>, safe_functions: Vec<&'a Expr>, + pandas_imported: bool, + outer_parameters: Vec<&'a ast::Parameters>, } /// `Visitor` to collect all suspicious variables (those referenced in @@ -89,7 +100,7 @@ impl<'a> Visitor<'a> for SuspiciousVariablesVisitor<'a> { Stmt::FunctionDef(ast::StmtFunctionDef { parameters, body, .. }) => { - // Collect all loaded variable names. + // Collect all loaded variable names and lambda parameters. let mut visitor = LoadedNamesVisitor::default(); visitor.visit_body(body); @@ -100,13 +111,27 @@ impl<'a> Visitor<'a> for SuspiciousVariablesVisitor<'a> { return false; } + // Check if variable is bound in current function parameters if parameters.includes(&loaded.id) { return false; } + // Check if variable is bound in outer function parameters + if self + .outer_parameters + .iter() + .any(|params| params.includes(&loaded.id)) + { + return false; + } true })); + // Recursively visit nested functions with updated parameter stack + self.outer_parameters.push(parameters); + visitor::walk_body(self, body); + self.outer_parameters.pop(); + return; } Stmt::Return(ast::StmtReturn { @@ -153,6 +178,15 @@ impl<'a> Visitor<'a> for SuspiciousVariablesVisitor<'a> { } } } + } else if attr == "apply" { + // If pandas is imported, apply is safe like map + if self.pandas_imported { + for arg in &*arguments.args { + if arg.is_lambda_expr() { + self.safe_functions.push(arg); + } + } + } } } _ => {} @@ -173,10 +207,16 @@ impl<'a> Visitor<'a> for SuspiciousVariablesVisitor<'a> { node_index: _, }) => { if !self.safe_functions.contains(&expr) { - // Collect all loaded variable names. + // Collect all loaded variable names from the lambda body. let mut visitor = LoadedNamesVisitor::default(); visitor.visit_expr(body); + // Collect lambda parameter names + let lambda_param_names: Vec<&str> = parameters + .as_ref() + .map(|params| params.iter().map(|param| param.name().as_str()).collect()) + .unwrap_or_default(); + // Treat any non-arguments as "suspicious". self.names .extend(visitor.loaded.into_iter().filter(|loaded| { @@ -184,10 +224,8 @@ impl<'a> Visitor<'a> for SuspiciousVariablesVisitor<'a> { return false; } - if parameters - .as_ref() - .is_some_and(|parameters| parameters.includes(&loaded.id)) - { + // Check if the variable is a lambda parameter + if lambda_param_names.contains(&loaded.id.as_str()) { return false; } @@ -285,7 +323,12 @@ pub(crate) fn function_uses_loop_variable(checker: &Checker, node: &Node) { // Identify any "suspicious" variables. These are defined as variables that are // referenced in a function or lambda body, but aren't bound as arguments. let suspicious_variables = { - let mut visitor = SuspiciousVariablesVisitor::default(); + let mut visitor = SuspiciousVariablesVisitor { + names: Vec::new(), + safe_functions: Vec::new(), + pandas_imported: checker.semantic().seen_module(Modules::PANDAS), + outer_parameters: Vec::new(), + }; match node { Node::Stmt(stmt) => visitor.visit_stmt(stmt), Node::Expr(expr) => visitor.visit_expr(expr), diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B023_B023.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B023_B023.py.snap index 034c5f4f03..0bfaf57dc4 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B023_B023.py.snap +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B023_B023.py.snap @@ -244,3 +244,14 @@ B023 Function definition does not bind loop variable `i` 174 | return [lambda: i for i in range(3)] # error | ^ | + +B023 Function definition does not bind loop variable `value` + --> B023.py:231:20 + | +229 | def add_one(): +230 | def _add_one_inner(): +231 | return value + 1 # Should trigger B023 - value is loop variable, not bound + | ^^^^^ +232 | +233 | return _add_one_inner + |