diff --git a/README.md b/README.md index d2237a2463..7ed82e1526 100644 --- a/README.md +++ b/README.md @@ -575,6 +575,7 @@ For more, see [flake8-bugbear](https://pypi.org/project/flake8-bugbear/22.10.27/ | B020 | LoopVariableOverridesIterator | Loop control variable `...` overrides iterable it iterates | | | B021 | FStringDocstring | f-string used as docstring. This will be interpreted by python as a joined string rather than a docstring. | | | B022 | UselessContextlibSuppress | No arguments passed to `contextlib.suppress`. No exceptions will be suppressed and therefore this context manager is redundant | | +| B023 | FunctionUsesLoopVariable | Function definition does not bind loop variable `...` | | | B024 | AbstractBaseClassWithoutAbstractMethod | `...` is an abstract base class, but it has no abstract methods | | | B025 | DuplicateTryBlockException | try-except block with duplicate exception `Exception` | | | B026 | StarArgUnpackingAfterKeywordArg | Star-arg unpacking after a keyword argument is strongly discouraged | | @@ -820,6 +821,8 @@ including: - [`pydocstyle`](https://pypi.org/project/pydocstyle/) - [`pep8-naming`](https://pypi.org/project/pep8-naming/) - [`yesqa`](https://github.com/asottile/yesqa) +- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) +- [`flake8-comprehensions`](https://pypi.org/project/flake8-comprehensions/) - [`flake8-docstrings`](https://pypi.org/project/flake8-docstrings/) - [`flake8-builtins`](https://pypi.org/project/flake8-builtins/) - [`flake8-super`](https://pypi.org/project/flake8-super/) @@ -827,9 +830,7 @@ including: - [`flake8-print`](https://pypi.org/project/flake8-print/) - [`flake8-quotes`](https://pypi.org/project/flake8-quotes/) - [`flake8-annotations`](https://pypi.org/project/flake8-annotations/) -- [`flake8-comprehensions`](https://pypi.org/project/flake8-comprehensions/) - [`flake8-bandit`](https://pypi.org/project/flake8-bandit/) (6/40) -- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (27/32) - [`flake8-2020`](https://pypi.org/project/flake8-2020/) - [`flake8-blind-except`](https://pypi.org/project/flake8-blind-except/) - [`flake8-boolean-trap`](https://pypi.org/project/flake8-boolean-trap/) @@ -851,6 +852,8 @@ Today, Ruff can be used to replace Flake8 when used with any of the following pl - [`pydocstyle`](https://pypi.org/project/pydocstyle/) - [`pep8-naming`](https://pypi.org/project/pep8-naming/) +- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) +- [`flake8-comprehensions`](https://pypi.org/project/flake8-comprehensions/) - [`flake8-docstrings`](https://pypi.org/project/flake8-docstrings/) - [`flake8-builtins`](https://pypi.org/project/flake8-builtins/) - [`flake8-super`](https://pypi.org/project/flake8-super/) @@ -859,8 +862,6 @@ Today, Ruff can be used to replace Flake8 when used with any of the following pl - [`flake8-quotes`](https://pypi.org/project/flake8-quotes/) - [`flake8-annotations`](https://pypi.org/project/flake8-annotations/) - [`flake8-bandit`](https://pypi.org/project/flake8-bandit/) (6/40) -- [`flake8-comprehensions`](https://pypi.org/project/flake8-comprehensions/) -- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (27/32) - [`flake8-2020`](https://pypi.org/project/flake8-2020/) - [`flake8-blind-except`](https://pypi.org/project/flake8-blind-except/) - [`flake8-boolean-trap`](https://pypi.org/project/flake8-boolean-trap/) diff --git a/resources/test/fixtures/B023.py b/resources/test/fixtures/B023.py new file mode 100644 index 0000000000..542e46edc9 --- /dev/null +++ b/resources/test/fixtures/B023.py @@ -0,0 +1,78 @@ +""" +Should emit: +B023 - on lines 12, 13, 16, 28, 29, 30, 31, 40, 42, 50, 51, 52, 53, 61, 68. +""" + +functions = [] +z = 0 + +for x in range(3): + y = x + 1 + # Subject to late-binding problems + functions.append(lambda: x) + functions.append(lambda: y) # not just the loop var + + def f_bad_1(): + return x + + # Actually OK + functions.append(lambda x: x * 2) + functions.append(lambda x=x: x) + functions.append(lambda: z) # OK because not assigned in the loop + + def f_ok_1(x): + return x * 2 + + +def check_inside_functions_too(): + ls = [lambda: x for x in range(2)] + st = {lambda: x for x in range(2)} + gn = (lambda: x for x in range(2)) + dt = {x: lambda: x for x in range(2)} + + +async def pointless_async_iterable(): + yield 1 + + +async def container_for_problems(): + async for x in pointless_async_iterable(): + functions.append(lambda: x) + + [lambda: x async for x in pointless_async_iterable()] + + +a = 10 +b = 0 +while True: + a = a_ = a - 1 + b += 1 + functions.append(lambda: a) + functions.append(lambda: a_) + functions.append(lambda: b) + functions.append(lambda: c) # not a name error because of late binding! + c: bool = a > 3 + if not c: + break + +# Nested loops should not duplicate reports +for j in range(2): + for k in range(3): + lambda: j * k + + +for j, k, l in [(1, 2, 3)]: + + def f(): + j = None # OK because it's an assignment + [l for k in range(2)] # error for l, not for k + + assert a and functions + + a.attribute = 1 # modifying an attribute doesn't make it a loop variable + functions[0] = lambda: None # same for an element + +for var in range(2): + + def explicit_capture(captured=var): + return captured diff --git a/ruff_dev/src/generate_check_code_prefix.rs b/ruff_dev/src/generate_check_code_prefix.rs index d52d803b0b..719e49130b 100644 --- a/ruff_dev/src/generate_check_code_prefix.rs +++ b/ruff_dev/src/generate_check_code_prefix.rs @@ -117,7 +117,8 @@ pub fn main(cli: &Cli) -> Result<()> { // Construct the output contents. let mut output = String::new(); - output.push_str("//! File automatically generated by examples/generate_check_code_prefix.rs."); + output + .push_str("//! File automatically generated by `examples/generate_check_code_prefix.rs`."); output.push('\n'); output.push('\n'); output.push_str("use serde::{{Serialize, Deserialize}};"); diff --git a/src/ast/helpers.rs b/src/ast/helpers.rs index 1266f9895c..f91ffbe1f6 100644 --- a/src/ast/helpers.rs +++ b/src/ast/helpers.rs @@ -1,7 +1,9 @@ use once_cell::sync::Lazy; use regex::Regex; use rustc_hash::{FxHashMap, FxHashSet}; -use rustpython_ast::{Excepthandler, ExcepthandlerKind, Expr, ExprKind, Location, Stmt, StmtKind}; +use rustpython_ast::{ + Arguments, Excepthandler, ExcepthandlerKind, Expr, ExprKind, Location, Stmt, StmtKind, +}; use crate::ast::types::Range; use crate::SourceCodeLocator; @@ -213,6 +215,27 @@ pub fn extract_handler_names(handlers: &[Excepthandler]) -> Vec> { handler_names } +/// Return the set of all bound argument names. +pub fn collect_arg_names<'a>(arguments: &'a Arguments) -> FxHashSet<&'a str> { + let mut arg_names: FxHashSet<&'a str> = FxHashSet::default(); + for arg in &arguments.posonlyargs { + arg_names.insert(arg.node.arg.as_str()); + } + for arg in &arguments.args { + arg_names.insert(arg.node.arg.as_str()); + } + if let Some(arg) = &arguments.vararg { + arg_names.insert(arg.node.arg.as_str()); + } + for arg in &arguments.kwonlyargs { + arg_names.insert(arg.node.arg.as_str()); + } + if let Some(arg) = &arguments.kwarg { + arg_names.insert(arg.node.arg.as_str()); + } + arg_names +} + /// Returns `true` if a call is an argumented `super` invocation. pub fn is_super_call_with_arguments(func: &Expr, args: &[Expr]) -> bool { // Check: is this a `super` call? diff --git a/src/ast/types.rs b/src/ast/types.rs index ba1a9479e8..ff2191f065 100644 --- a/src/ast/types.rs +++ b/src/ast/types.rs @@ -1,7 +1,7 @@ use std::sync::atomic::{AtomicUsize, Ordering}; use rustc_hash::FxHashMap; -use rustpython_ast::{Expr, Keyword}; +use rustpython_ast::{Expr, Keyword, Stmt}; use rustpython_parser::ast::{Located, Location}; fn id() -> usize { @@ -9,6 +9,12 @@ fn id() -> usize { COUNTER.fetch_add(1, Ordering::Relaxed) } +#[derive(Clone)] +pub enum Node<'a> { + Stmt(&'a Stmt), + Expr(&'a Expr), +} + #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord)] pub struct Range { pub location: Location, diff --git a/src/check_ast.rs b/src/check_ast.rs index 3a0c85936a..2edc0e7a88 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -19,8 +19,8 @@ use crate::ast::helpers::{ use crate::ast::operations::extract_all_names; use crate::ast::relocate::relocate_expr; use crate::ast::types::{ - Binding, BindingContext, BindingKind, ClassScope, FunctionScope, ImportKind, Range, Scope, - ScopeKind, + Binding, BindingContext, BindingKind, ClassScope, FunctionScope, ImportKind, Node, Range, + Scope, ScopeKind, }; use crate::ast::visitor::{walk_excepthandler, walk_withitem, Visitor}; use crate::ast::{helpers, operations, visitor}; @@ -83,6 +83,8 @@ pub struct Checker<'a> { futures_allowed: bool, annotations_future_enabled: bool, except_handlers: Vec>>, + // Check-specific state. + pub(crate) seen_b023: Vec<&'a Expr>, } impl<'a> Checker<'a> { @@ -127,6 +129,8 @@ impl<'a> Checker<'a> { futures_allowed: true, annotations_future_enabled: false, except_handlers: vec![], + // Check-specific state. + seen_b023: vec![], } } @@ -951,8 +955,16 @@ where flake8_bugbear::plugins::assert_raises_exception(self, stmt, items); } } + StmtKind::While { .. } => { + if self.settings.enabled.contains(&CheckCode::B023) { + flake8_bugbear::plugins::function_uses_loop_variable(self, &Node::Stmt(stmt)); + } + } StmtKind::For { target, body, iter, .. + } + | StmtKind::AsyncFor { + target, body, iter, .. } => { if self.settings.enabled.contains(&CheckCode::B007) { flake8_bugbear::plugins::unused_loop_control_variable(self, target, body); @@ -960,6 +972,9 @@ where if self.settings.enabled.contains(&CheckCode::B020) { flake8_bugbear::plugins::loop_variable_overrides_iterator(self, target, iter); } + if self.settings.enabled.contains(&CheckCode::B023) { + flake8_bugbear::plugins::function_uses_loop_variable(self, &Node::Stmt(stmt)); + } } StmtKind::Try { handlers, .. } => { if self.settings.enabled.contains(&CheckCode::F707) { @@ -1810,9 +1825,15 @@ where self.add_check(check); }; } + if self.settings.enabled.contains(&CheckCode::B023) { + flake8_bugbear::plugins::function_uses_loop_variable(self, &Node::Expr(expr)); + } self.push_scope(Scope::new(ScopeKind::Generator)); } ExprKind::GeneratorExp { .. } | ExprKind::DictComp { .. } => { + if self.settings.enabled.contains(&CheckCode::B023) { + flake8_bugbear::plugins::function_uses_loop_variable(self, &Node::Expr(expr)); + } self.push_scope(Scope::new(ScopeKind::Generator)); } _ => {} diff --git a/src/checks.rs b/src/checks.rs index ca53297a55..251a5539fb 100644 --- a/src/checks.rs +++ b/src/checks.rs @@ -104,6 +104,7 @@ pub enum CheckCode { B020, B021, B022, + B023, B024, B025, B026, @@ -427,32 +428,33 @@ pub enum CheckKind { // flake8-blind-except BlindExcept, // flake8-bugbear - UnaryPrefixIncrement, - AssignmentToOsEnviron, - UnreliableCallableCheck, - StripWithMultiCharacters, - MutableArgumentDefault, - UnusedLoopControlVariable(String), - FunctionCallArgumentDefault(Option), - GetAttrWithConstant, - SetAttrWithConstant, - DoNotAssertFalse, - JumpStatementInFinally(String), - RedundantTupleInExceptionHandler(String), - DuplicateHandlerException(Vec), - UselessComparison, - CannotRaiseLiteral, - NoAssertRaisesException, - UselessExpression, - CachedInstanceMethod, - LoopVariableOverridesIterator(String), - FStringDocstring, - UselessContextlibSuppress, AbstractBaseClassWithoutAbstractMethod(String), + AssignmentToOsEnviron, + CachedInstanceMethod, + CannotRaiseLiteral, + DoNotAssertFalse, + DuplicateHandlerException(Vec), DuplicateTryBlockException(String), - StarArgUnpackingAfterKeywordArg, EmptyMethodWithoutAbstractDecorator(String), + FStringDocstring, + FunctionCallArgumentDefault(Option), + FunctionUsesLoopVariable(String), + GetAttrWithConstant, + JumpStatementInFinally(String), + LoopVariableOverridesIterator(String), + MutableArgumentDefault, + NoAssertRaisesException, RaiseWithoutFromInsideExcept, + RedundantTupleInExceptionHandler(String), + SetAttrWithConstant, + StarArgUnpackingAfterKeywordArg, + StripWithMultiCharacters, + UnaryPrefixIncrement, + UnreliableCallableCheck, + UnusedLoopControlVariable(String), + UselessComparison, + UselessContextlibSuppress, + UselessExpression, // flake8-comprehensions UnnecessaryGeneratorList, UnnecessaryGeneratorSet, @@ -716,6 +718,7 @@ impl CheckCode { CheckCode::B020 => CheckKind::LoopVariableOverridesIterator("...".to_string()), CheckCode::B021 => CheckKind::FStringDocstring, CheckCode::B022 => CheckKind::UselessContextlibSuppress, + CheckCode::B023 => CheckKind::FunctionUsesLoopVariable("...".to_string()), CheckCode::B024 => CheckKind::AbstractBaseClassWithoutAbstractMethod("...".to_string()), CheckCode::B025 => CheckKind::DuplicateTryBlockException("Exception".to_string()), CheckCode::B026 => CheckKind::StarArgUnpackingAfterKeywordArg, @@ -981,6 +984,7 @@ impl CheckCode { CheckCode::B020 => CheckCategory::Flake8Bugbear, CheckCode::B021 => CheckCategory::Flake8Bugbear, CheckCode::B022 => CheckCategory::Flake8Bugbear, + CheckCode::B023 => CheckCategory::Flake8Bugbear, CheckCode::B024 => CheckCategory::Flake8Bugbear, CheckCode::B025 => CheckCategory::Flake8Bugbear, CheckCode::B026 => CheckCategory::Flake8Bugbear, @@ -1184,32 +1188,33 @@ impl CheckKind { CheckKind::BuiltinArgumentShadowing(_) => &CheckCode::A002, CheckKind::BuiltinAttributeShadowing(_) => &CheckCode::A003, // flake8-bugbear - CheckKind::UnaryPrefixIncrement => &CheckCode::B002, - CheckKind::AssignmentToOsEnviron => &CheckCode::B003, - CheckKind::UnreliableCallableCheck => &CheckCode::B004, - CheckKind::StripWithMultiCharacters => &CheckCode::B005, - CheckKind::MutableArgumentDefault => &CheckCode::B006, - CheckKind::UnusedLoopControlVariable(_) => &CheckCode::B007, - CheckKind::FunctionCallArgumentDefault(_) => &CheckCode::B008, - CheckKind::GetAttrWithConstant => &CheckCode::B009, - CheckKind::SetAttrWithConstant => &CheckCode::B010, - CheckKind::DoNotAssertFalse => &CheckCode::B011, - CheckKind::JumpStatementInFinally(_) => &CheckCode::B012, - CheckKind::RedundantTupleInExceptionHandler(_) => &CheckCode::B013, - CheckKind::DuplicateHandlerException(_) => &CheckCode::B014, - CheckKind::UselessComparison => &CheckCode::B015, - CheckKind::CannotRaiseLiteral => &CheckCode::B016, - CheckKind::NoAssertRaisesException => &CheckCode::B017, - CheckKind::UselessExpression => &CheckCode::B018, - CheckKind::CachedInstanceMethod => &CheckCode::B019, - CheckKind::LoopVariableOverridesIterator(_) => &CheckCode::B020, - CheckKind::FStringDocstring => &CheckCode::B021, - CheckKind::UselessContextlibSuppress => &CheckCode::B022, CheckKind::AbstractBaseClassWithoutAbstractMethod(_) => &CheckCode::B024, + CheckKind::AssignmentToOsEnviron => &CheckCode::B003, + CheckKind::CachedInstanceMethod => &CheckCode::B019, + CheckKind::CannotRaiseLiteral => &CheckCode::B016, + CheckKind::DoNotAssertFalse => &CheckCode::B011, + CheckKind::DuplicateHandlerException(_) => &CheckCode::B014, CheckKind::DuplicateTryBlockException(_) => &CheckCode::B025, - CheckKind::StarArgUnpackingAfterKeywordArg => &CheckCode::B026, CheckKind::EmptyMethodWithoutAbstractDecorator(_) => &CheckCode::B027, + CheckKind::FStringDocstring => &CheckCode::B021, + CheckKind::FunctionCallArgumentDefault(_) => &CheckCode::B008, + CheckKind::FunctionUsesLoopVariable(_) => &CheckCode::B023, + CheckKind::GetAttrWithConstant => &CheckCode::B009, + CheckKind::JumpStatementInFinally(_) => &CheckCode::B012, + CheckKind::LoopVariableOverridesIterator(_) => &CheckCode::B020, + CheckKind::MutableArgumentDefault => &CheckCode::B006, + CheckKind::NoAssertRaisesException => &CheckCode::B017, CheckKind::RaiseWithoutFromInsideExcept => &CheckCode::B904, + CheckKind::RedundantTupleInExceptionHandler(_) => &CheckCode::B013, + CheckKind::SetAttrWithConstant => &CheckCode::B010, + CheckKind::StarArgUnpackingAfterKeywordArg => &CheckCode::B026, + CheckKind::StripWithMultiCharacters => &CheckCode::B005, + CheckKind::UnaryPrefixIncrement => &CheckCode::B002, + CheckKind::UnreliableCallableCheck => &CheckCode::B004, + CheckKind::UnusedLoopControlVariable(_) => &CheckCode::B007, + CheckKind::UselessComparison => &CheckCode::B015, + CheckKind::UselessContextlibSuppress => &CheckCode::B022, + CheckKind::UselessExpression => &CheckCode::B018, // flake8-blind-except CheckKind::BlindExcept => &CheckCode::BLE001, // flake8-comprehensions @@ -1554,6 +1559,9 @@ impl CheckKind { "Do not perform function call in argument defaults".to_string() } } + CheckKind::FunctionUsesLoopVariable(name) => { + format!("Function definition does not bind loop variable `{name}`") + } CheckKind::GetAttrWithConstant => "Do not call `getattr` with a constant attribute \ value. It is not any safer than normal property \ access." diff --git a/src/checks_gen.rs b/src/checks_gen.rs index 03888022d7..b333442ff4 100644 --- a/src/checks_gen.rs +++ b/src/checks_gen.rs @@ -59,6 +59,7 @@ pub enum CheckCodePrefix { B020, B021, B022, + B023, B024, B025, B026, @@ -186,6 +187,12 @@ pub enum CheckCodePrefix { F406, F407, F5, + F52, + F521, + F522, + F523, + F524, + F525, F54, F541, F6, @@ -415,6 +422,7 @@ impl CheckCodePrefix { CheckCode::B020, CheckCode::B021, CheckCode::B022, + CheckCode::B023, CheckCode::B024, CheckCode::B025, CheckCode::B026, @@ -443,6 +451,7 @@ impl CheckCodePrefix { CheckCode::B020, CheckCode::B021, CheckCode::B022, + CheckCode::B023, CheckCode::B024, CheckCode::B025, CheckCode::B026, @@ -492,6 +501,7 @@ impl CheckCodePrefix { CheckCode::B020, CheckCode::B021, CheckCode::B022, + CheckCode::B023, CheckCode::B024, CheckCode::B025, CheckCode::B026, @@ -500,6 +510,7 @@ impl CheckCodePrefix { CheckCodePrefix::B020 => vec![CheckCode::B020], CheckCodePrefix::B021 => vec![CheckCode::B021], CheckCodePrefix::B022 => vec![CheckCode::B022], + CheckCodePrefix::B023 => vec![CheckCode::B023], CheckCodePrefix::B024 => vec![CheckCode::B024], CheckCodePrefix::B025 => vec![CheckCode::B025], CheckCodePrefix::B026 => vec![CheckCode::B026], @@ -899,7 +910,26 @@ impl CheckCodePrefix { CheckCodePrefix::F405 => vec![CheckCode::F405], CheckCodePrefix::F406 => vec![CheckCode::F406], CheckCodePrefix::F407 => vec![CheckCode::F407], - CheckCodePrefix::F5 => vec![CheckCode::F541], + CheckCodePrefix::F5 => vec![ + CheckCode::F521, + CheckCode::F522, + CheckCode::F523, + CheckCode::F524, + CheckCode::F525, + CheckCode::F541, + ], + CheckCodePrefix::F52 => vec![ + CheckCode::F521, + CheckCode::F522, + CheckCode::F523, + CheckCode::F524, + CheckCode::F525, + ], + CheckCodePrefix::F521 => vec![CheckCode::F521], + CheckCodePrefix::F522 => vec![CheckCode::F522], + CheckCodePrefix::F523 => vec![CheckCode::F523], + CheckCodePrefix::F524 => vec![CheckCode::F524], + CheckCodePrefix::F525 => vec![CheckCode::F525], CheckCodePrefix::F54 => vec![CheckCode::F541], CheckCodePrefix::F541 => vec![CheckCode::F541], CheckCodePrefix::F6 => vec![ @@ -1293,6 +1323,7 @@ impl CheckCodePrefix { CheckCodePrefix::B020 => PrefixSpecificity::Explicit, CheckCodePrefix::B021 => PrefixSpecificity::Explicit, CheckCodePrefix::B022 => PrefixSpecificity::Explicit, + CheckCodePrefix::B023 => PrefixSpecificity::Explicit, CheckCodePrefix::B024 => PrefixSpecificity::Explicit, CheckCodePrefix::B025 => PrefixSpecificity::Explicit, CheckCodePrefix::B026 => PrefixSpecificity::Explicit, @@ -1420,6 +1451,12 @@ impl CheckCodePrefix { CheckCodePrefix::F406 => PrefixSpecificity::Explicit, CheckCodePrefix::F407 => PrefixSpecificity::Explicit, CheckCodePrefix::F5 => PrefixSpecificity::Hundreds, + CheckCodePrefix::F52 => PrefixSpecificity::Tens, + CheckCodePrefix::F521 => PrefixSpecificity::Explicit, + CheckCodePrefix::F522 => PrefixSpecificity::Explicit, + CheckCodePrefix::F523 => PrefixSpecificity::Explicit, + CheckCodePrefix::F524 => PrefixSpecificity::Explicit, + CheckCodePrefix::F525 => PrefixSpecificity::Explicit, CheckCodePrefix::F54 => PrefixSpecificity::Tens, CheckCodePrefix::F541 => PrefixSpecificity::Explicit, CheckCodePrefix::F6 => PrefixSpecificity::Hundreds, diff --git a/src/flake8_annotations/plugins.rs b/src/flake8_annotations/plugins.rs index e51bbde074..ea627d5073 100644 --- a/src/flake8_annotations/plugins.rs +++ b/src/flake8_annotations/plugins.rs @@ -21,7 +21,7 @@ where fn visit_stmt(&mut self, stmt: &'b Stmt) { match &stmt.node { StmtKind::FunctionDef { .. } | StmtKind::AsyncFunctionDef { .. } => { - // No recurse. + // Don't recurse. } StmtKind::Return { value } => self.returns.push(value.as_ref().map(|expr| &**expr)), _ => visitor::walk_stmt(self, stmt), diff --git a/src/flake8_bugbear/plugins/function_uses_loop_variable.rs b/src/flake8_bugbear/plugins/function_uses_loop_variable.rs new file mode 100644 index 0000000000..4627790af6 --- /dev/null +++ b/src/flake8_bugbear/plugins/function_uses_loop_variable.rs @@ -0,0 +1,218 @@ +use rustc_hash::FxHashSet; +use rustpython_ast::{Comprehension, Expr, ExprContext, ExprKind, Stmt, StmtKind}; + +use crate::ast::helpers::collect_arg_names; +use crate::ast::types::{Node, Range}; +use crate::ast::visitor; +use crate::ast::visitor::Visitor; +use crate::check_ast::Checker; +use crate::checks::{Check, CheckKind}; + +#[derive(Default)] +struct LoadedNamesVisitor<'a> { + names: Vec<(&'a str, &'a Expr)>, +} + +/// `Visitor` to collect all used identifiers in a statement. +impl<'a, 'b> Visitor<'b> for LoadedNamesVisitor<'a> +where + 'b: 'a, +{ + fn visit_expr(&mut self, expr: &'b Expr) { + match &expr.node { + ExprKind::Name { id, ctx } if matches!(ctx, ExprContext::Load) => { + self.names.push((id, expr)); + } + _ => visitor::walk_expr(self, expr), + } + } +} + +#[derive(Default)] +struct SuspiciousVariablesVisitor<'a> { + names: Vec<(&'a str, &'a Expr)>, +} + +/// `Visitor` to collect all suspicious variables (those referenced in +/// functions, but not bound as arguments). +impl<'a, 'b> Visitor<'b> for SuspiciousVariablesVisitor<'a> +where + 'b: 'a, +{ + fn visit_stmt(&mut self, stmt: &'b Stmt) { + match &stmt.node { + StmtKind::FunctionDef { args, body, .. } + | StmtKind::AsyncFunctionDef { args, body, .. } => { + // Collect all loaded variable names. + let mut visitor = LoadedNamesVisitor::default(); + for stmt in body { + visitor.visit_stmt(stmt); + } + + // Collect all argument names. + let arg_names = collect_arg_names(args); + + // Treat any non-arguments as "suspicious". + self.names.extend( + visitor + .names + .into_iter() + .filter(|(id, _)| !arg_names.contains(id)), + ); + } + _ => visitor::walk_stmt(self, stmt), + } + } + + fn visit_expr(&mut self, expr: &'b Expr) { + match &expr.node { + ExprKind::Lambda { args, body } => { + // Collect all loaded variable names. + let mut visitor = LoadedNamesVisitor::default(); + visitor.visit_expr(body); + + // Collect all argument names. + let arg_names = collect_arg_names(args); + + // Treat any non-arguments as "suspicious". + self.names.extend( + visitor + .names + .into_iter() + .filter(|(id, _)| !arg_names.contains(id)), + ); + } + _ => visitor::walk_expr(self, expr), + } + } +} + +#[derive(Default)] +struct NamesFromAssignmentsVisitor<'a> { + names: FxHashSet<&'a str>, +} + +/// `Visitor` to collect all names used in an assignment expression. +impl<'a, 'b> Visitor<'b> for NamesFromAssignmentsVisitor<'a> +where + 'b: 'a, +{ + fn visit_expr(&mut self, expr: &'b Expr) { + match &expr.node { + ExprKind::Name { id, .. } => { + self.names.insert(id.as_str()); + } + ExprKind::Starred { value, .. } => { + self.visit_expr(value); + } + ExprKind::List { elts, .. } | ExprKind::Tuple { elts, .. } => { + for expr in elts { + self.visit_expr(expr); + } + } + _ => {} + } + } +} + +#[derive(Default)] +struct AssignedNamesVisitor<'a> { + names: FxHashSet<&'a str>, +} + +/// `Visitor` to collect all used identifiers in a statement. +impl<'a, 'b> Visitor<'b> for AssignedNamesVisitor<'a> +where + 'b: 'a, +{ + fn visit_stmt(&mut self, stmt: &'b Stmt) { + if matches!( + &stmt.node, + StmtKind::FunctionDef { .. } | StmtKind::AsyncFunctionDef { .. } + ) { + // Don't recurse. + return; + } + + match &stmt.node { + StmtKind::Assign { targets, .. } => { + let mut visitor = NamesFromAssignmentsVisitor::default(); + for expr in targets { + visitor.visit_expr(expr); + } + self.names.extend(visitor.names); + } + StmtKind::AugAssign { target, .. } + | StmtKind::AnnAssign { target, .. } + | StmtKind::For { target, .. } + | StmtKind::AsyncFor { target, .. } => { + let mut visitor = NamesFromAssignmentsVisitor::default(); + visitor.visit_expr(target); + self.names.extend(visitor.names); + } + _ => {} + } + + visitor::walk_stmt(self, stmt); + } + + fn visit_expr(&mut self, expr: &'b Expr) { + if matches!(&expr.node, ExprKind::Lambda { .. }) { + // Don't recurse. + return; + } + + visitor::walk_expr(self, expr); + } + + fn visit_comprehension(&mut self, comprehension: &'b Comprehension) { + let mut visitor = NamesFromAssignmentsVisitor::default(); + visitor.visit_expr(&comprehension.target); + self.names.extend(visitor.names); + + visitor::walk_comprehension(self, comprehension); + } +} + +/// B023 +pub fn function_uses_loop_variable<'a, 'b>(checker: &'a mut Checker<'b>, node: &Node<'b>) +where + 'b: 'a, +{ + // 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::<'b>::default(); + match node { + Node::Stmt(stmt) => visitor.visit_stmt(stmt), + Node::Expr(expr) => visitor.visit_expr(expr), + } + visitor.names + }; + + if !suspicious_variables.is_empty() { + // Identify any variables that are assigned in the loop (ignoring functions). + let reassigned_in_loop = { + let mut visitor = AssignedNamesVisitor::<'b>::default(); + match node { + Node::Stmt(stmt) => visitor.visit_stmt(stmt), + Node::Expr(expr) => visitor.visit_expr(expr), + } + visitor.names + }; + + // If a variable was used in a function or lambda body, and assigned in the + // loop, flag it. + for (name, expr) in suspicious_variables { + if reassigned_in_loop.contains(name) { + if !checker.seen_b023.contains(&expr) { + checker.seen_b023.push(expr); + checker.add_check(Check::new( + CheckKind::FunctionUsesLoopVariable(name.to_string()), + Range::from_located(expr), + )); + } + } + } + } +} diff --git a/src/flake8_bugbear/plugins/mod.rs b/src/flake8_bugbear/plugins/mod.rs index 0f8d49b1d9..5ca3142e5a 100644 --- a/src/flake8_bugbear/plugins/mod.rs +++ b/src/flake8_bugbear/plugins/mod.rs @@ -7,6 +7,7 @@ pub use cannot_raise_literal::cannot_raise_literal; pub use duplicate_exceptions::duplicate_exceptions; pub use f_string_docstring::f_string_docstring; pub use function_call_argument_default::function_call_argument_default; +pub use function_uses_loop_variable::function_uses_loop_variable; pub use getattr_with_constant::getattr_with_constant; pub use jump_statement_in_finally::jump_statement_in_finally; pub use loop_variable_overrides_iterator::loop_variable_overrides_iterator; @@ -32,6 +33,7 @@ mod cannot_raise_literal; mod duplicate_exceptions; mod f_string_docstring; mod function_call_argument_default; +mod function_uses_loop_variable; mod getattr_with_constant; mod jump_statement_in_finally; mod loop_variable_overrides_iterator; diff --git a/src/linter.rs b/src/linter.rs index d06569e2b6..512d06cc0c 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -413,6 +413,7 @@ mod tests { #[test_case(CheckCode::B020, Path::new("B020.py"); "B020")] #[test_case(CheckCode::B021, Path::new("B021.py"); "B021")] #[test_case(CheckCode::B022, Path::new("B022.py"); "B022")] + #[test_case(CheckCode::B023, Path::new("B023.py"); "B023")] #[test_case(CheckCode::B024, Path::new("B024.py"); "B024")] #[test_case(CheckCode::B025, Path::new("B025.py"); "B025")] #[test_case(CheckCode::B026, Path::new("B026.py"); "B026")] diff --git a/src/snapshots/ruff__linter__tests__B023_B023.py.snap b/src/snapshots/ruff__linter__tests__B023_B023.py.snap new file mode 100644 index 0000000000..407de8272f --- /dev/null +++ b/src/snapshots/ruff__linter__tests__B023_B023.py.snap @@ -0,0 +1,149 @@ +--- +source: src/linter.rs +expression: checks +--- +- kind: + FunctionUsesLoopVariable: x + location: + row: 12 + column: 29 + end_location: + row: 12 + column: 30 + fix: ~ +- kind: + FunctionUsesLoopVariable: y + location: + row: 13 + column: 29 + end_location: + row: 13 + column: 30 + fix: ~ +- kind: + FunctionUsesLoopVariable: x + location: + row: 16 + column: 15 + end_location: + row: 16 + column: 16 + fix: ~ +- kind: + FunctionUsesLoopVariable: x + location: + row: 28 + column: 18 + end_location: + row: 28 + column: 19 + fix: ~ +- kind: + FunctionUsesLoopVariable: x + location: + row: 29 + column: 18 + end_location: + row: 29 + column: 19 + fix: ~ +- kind: + FunctionUsesLoopVariable: x + location: + row: 30 + column: 18 + end_location: + row: 30 + column: 19 + fix: ~ +- kind: + FunctionUsesLoopVariable: x + location: + row: 31 + column: 21 + end_location: + row: 31 + column: 22 + fix: ~ +- kind: + FunctionUsesLoopVariable: x + location: + row: 40 + column: 33 + end_location: + row: 40 + column: 34 + fix: ~ +- kind: + FunctionUsesLoopVariable: x + location: + row: 42 + column: 13 + end_location: + row: 42 + column: 14 + fix: ~ +- kind: + FunctionUsesLoopVariable: a + location: + row: 50 + column: 29 + end_location: + row: 50 + column: 30 + fix: ~ +- kind: + FunctionUsesLoopVariable: a_ + location: + row: 51 + column: 29 + end_location: + row: 51 + column: 31 + fix: ~ +- kind: + FunctionUsesLoopVariable: b + location: + row: 52 + column: 29 + end_location: + row: 52 + column: 30 + fix: ~ +- kind: + FunctionUsesLoopVariable: c + location: + row: 53 + column: 29 + end_location: + row: 53 + column: 30 + fix: ~ +- kind: + FunctionUsesLoopVariable: j + location: + row: 61 + column: 16 + end_location: + row: 61 + column: 17 + fix: ~ +- kind: + FunctionUsesLoopVariable: k + location: + row: 61 + column: 20 + end_location: + row: 61 + column: 21 + fix: ~ +- kind: + FunctionUsesLoopVariable: l + location: + row: 68 + column: 9 + end_location: + row: 68 + column: 10 + fix: ~ +