From 43a8ce6c89b60de9e810e6452062f43c8d17c462 Mon Sep 17 00:00:00 2001 From: Florian Best Date: Wed, 25 Jan 2023 21:18:58 +0100 Subject: [PATCH] fix: avoid flagging unused loop variable (B007) with globals(), vars() or eval() (#2166) --- README.md | 2 +- .../test/fixtures/flake8_bugbear/B007.py | 11 ++ src/ast/helpers.rs | 20 +-- .../rules/unused_loop_control_variable.rs | 12 +- ...ake8_bugbear__tests__B007_B007.py.snap.new | 130 ++++++++++++++++++ src/violations.rs | 31 ++++- 6 files changed, 183 insertions(+), 23 deletions(-) create mode 100644 src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B007_B007.py.snap.new diff --git a/README.md b/README.md index df9e5e6269..d28d471129 100644 --- a/README.md +++ b/README.md @@ -853,7 +853,7 @@ For more, see [flake8-bugbear](https://pypi.org/project/flake8-bugbear/) on PyPI | B004 | unreliable-callable-check | Using `hasattr(x, '__call__')` to test if x is callable is unreliable. Use `callable(x)` for consistent results. | | | B005 | strip-with-multi-characters | Using `.strip()` with multi-character strings is misleading the reader | | | B006 | mutable-argument-default | Do not use mutable data structures for argument defaults | | -| B007 | unused-loop-control-variable | Loop control variable `{name}` not used within the loop body | 🛠 | +| B007 | unused-loop-control-variable | Loop control variable `{name}` not used within loop body | | | B008 | function-call-argument-default | Do not perform function call `{name}` in argument defaults | | | B009 | get-attr-with-constant | Do not call `getattr` with a constant attribute value. It is not any safer than normal property access. | 🛠 | | B010 | set-attr-with-constant | Do not call `setattr` with a constant attribute value. It is not any safer than normal property access. | 🛠 | diff --git a/resources/test/fixtures/flake8_bugbear/B007.py b/resources/test/fixtures/flake8_bugbear/B007.py index 569b5ef0d1..891e008c40 100644 --- a/resources/test/fixtures/flake8_bugbear/B007.py +++ b/resources/test/fixtures/flake8_bugbear/B007.py @@ -34,3 +34,14 @@ FMT = "{foo} {bar}" for foo, bar in [(1, 2)]: if foo: print(FMT.format(**locals())) + +for foo, bar in [(1, 2)]: + if foo: + print(FMT.format(**globals())) + +for foo, bar in [(1, 2)]: + if foo: + print(FMT.format(**vars())) + +for foo, bar in [(1, 2)]: + print(FMT.format(foo=foo, bar=eval('bar'))) diff --git a/src/ast/helpers.rs b/src/ast/helpers.rs index 2237205c76..86de087f28 100644 --- a/src/ast/helpers.rs +++ b/src/ast/helpers.rs @@ -4,8 +4,8 @@ use once_cell::sync::Lazy; use regex::Regex; use rustc_hash::{FxHashMap, FxHashSet}; use rustpython_ast::{ - Arguments, Constant, Excepthandler, ExcepthandlerKind, Expr, ExprContext, ExprKind, Keyword, - KeywordData, Located, Location, Stmt, StmtKind, + Arguments, Constant, Excepthandler, ExcepthandlerKind, Expr, ExprKind, Keyword, KeywordData, + Located, Location, Stmt, StmtKind, }; use rustpython_parser::lexer; use rustpython_parser::lexer::Tok; @@ -562,15 +562,17 @@ pub fn is_super_call_with_arguments(func: &Expr, args: &[Expr]) -> bool { } } -/// Return `true` if the body uses `locals()`. -pub fn uses_locals(body: &[Stmt]) -> bool { +/// Return `true` if the body uses `locals()`, `globals()`, `vars()`, `eval()`. +pub fn uses_magic_variable_access(checker: &Checker, body: &[Stmt]) -> bool { any_over_body(body, &|expr| { if let ExprKind::Call { func, .. } = &expr.node { - if let ExprKind::Name { id, ctx } = &func.node { - id == "locals" && matches!(ctx, ExprContext::Load) - } else { - false - } + checker.resolve_call_path(func).map_or(false, |call_path| { + call_path.as_slice() == ["", "locals"] + || call_path.as_slice() == ["", "globals"] + || call_path.as_slice() == ["", "vars"] + || call_path.as_slice() == ["", "eval"] + || call_path.as_slice() == ["", "exec"] + }) } else { false } diff --git a/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs b/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs index c1808847a1..2d5de155d7 100644 --- a/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs +++ b/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs @@ -57,10 +57,6 @@ where /// B007 pub fn unused_loop_control_variable(checker: &mut Checker, target: &Expr, body: &[Stmt]) { - if helpers::uses_locals(body) { - return; - } - let control_names = { let mut finder = NameFinder::new(); finder.visit_expr(target); @@ -86,11 +82,15 @@ pub fn unused_loop_control_variable(checker: &mut Checker, target: &Expr, body: continue; } + let safe = !helpers::uses_magic_variable_access(checker, body); let mut diagnostic = Diagnostic::new( - violations::UnusedLoopControlVariable(name.to_string()), + violations::UnusedLoopControlVariable { + name: name.to_string(), + safe, + }, Range::from_located(expr), ); - if checker.patch(diagnostic.kind.rule()) { + if safe && checker.patch(diagnostic.kind.rule()) { // Prefix the variable name with an underscore. diagnostic.amend(Fix::replacement( format!("_{name}"), diff --git a/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B007_B007.py.snap.new b/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B007_B007.py.snap.new new file mode 100644 index 0000000000..1fee3a28b4 --- /dev/null +++ b/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B007_B007.py.snap.new @@ -0,0 +1,130 @@ +--- +source: src/rules/flake8_bugbear/mod.rs +assertion_line: 52 +expression: diagnostics +--- +- kind: + UnusedLoopControlVariable: + name: i + safe: true + location: + row: 6 + column: 4 + end_location: + row: 6 + column: 5 + fix: + content: _i + location: + row: 6 + column: 4 + end_location: + row: 6 + column: 5 + parent: ~ +- kind: + UnusedLoopControlVariable: + name: k + safe: true + location: + row: 18 + column: 12 + end_location: + row: 18 + column: 13 + fix: + content: _k + location: + row: 18 + column: 12 + end_location: + row: 18 + column: 13 + parent: ~ +- kind: + UnusedLoopControlVariable: + name: i + safe: true + location: + row: 30 + column: 4 + end_location: + row: 30 + column: 5 + fix: + content: _i + location: + row: 30 + column: 4 + end_location: + row: 30 + column: 5 + parent: ~ +- kind: + UnusedLoopControlVariable: + name: k + safe: true + location: + row: 30 + column: 12 + end_location: + row: 30 + column: 13 + fix: + content: _k + location: + row: 30 + column: 12 + end_location: + row: 30 + column: 13 + parent: ~ +- kind: + UnusedLoopControlVariable: + name: bar + safe: false + location: + row: 34 + column: 9 + end_location: + row: 34 + column: 12 + fix: ~ + parent: ~ +- kind: + UnusedLoopControlVariable: + name: bar + safe: false + location: + row: 38 + column: 9 + end_location: + row: 38 + column: 12 + fix: ~ + parent: ~ +- kind: + UnusedLoopControlVariable: + name: bar + safe: false + location: + row: 42 + column: 9 + end_location: + row: 42 + column: 12 + fix: ~ + parent: ~ +- kind: + UnusedLoopControlVariable: + name: bar + safe: false + location: + row: 46 + column: 9 + end_location: + row: 46 + column: 12 + fix: ~ + parent: ~ + diff --git a/src/violations.rs b/src/violations.rs index ade6e627ab..7de190760c 100644 --- a/src/violations.rs +++ b/src/violations.rs @@ -1181,18 +1181,35 @@ impl Violation for MutableArgumentDefault { } define_violation!( - pub struct UnusedLoopControlVariable(pub String); + pub struct UnusedLoopControlVariable { + /// The name of the loop control variable. + pub name: String, + /// Whether the variable is safe to rename. A variable is unsafe to + /// rename if it _might_ be used by magic control flow (e.g., + /// `locals()`). + pub safe: bool, + } ); -impl AlwaysAutofixableViolation for UnusedLoopControlVariable { +impl Violation for UnusedLoopControlVariable { #[derive_message_formats] fn message(&self) -> String { - let UnusedLoopControlVariable(name) = self; - format!("Loop control variable `{name}` not used within the loop body") + let UnusedLoopControlVariable { name, safe } = self; + if *safe { + format!("Loop control variable `{name}` not used within loop body") + } else { + format!("Loop control variable `{name}` may not be used within loop body") + } } - fn autofix_title(&self) -> String { - let UnusedLoopControlVariable(name) = self; - format!("Rename unused `{name}` to `_{name}`") + fn autofix_title_formatter(&self) -> Option String> { + let UnusedLoopControlVariable { safe, .. } = self; + if *safe { + Some(|UnusedLoopControlVariable { name, .. }| { + format!("Rename unused `{name}` to `_{name}`") + }) + } else { + None + } } }