From a0746251215bb31aece6795cf174ae6fed7b311f Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 2 Feb 2023 20:59:47 -0500 Subject: [PATCH] Avoid renaming unused loop variables with deferred usages (#2509) --- .../test/fixtures/flake8_bugbear/B007.py | 27 +++++- src/checkers/ast.rs | 28 +++++- .../rules/unused_loop_control_variable.rs | 75 +++++++++++----- ...__flake8_bugbear__tests__B007_B007.py.snap | 90 +++++++++++-------- 4 files changed, 161 insertions(+), 59 deletions(-) diff --git a/resources/test/fixtures/flake8_bugbear/B007.py b/resources/test/fixtures/flake8_bugbear/B007.py index 891e008c40..f249aced9b 100644 --- a/resources/test/fixtures/flake8_bugbear/B007.py +++ b/resources/test/fixtures/flake8_bugbear/B007.py @@ -44,4 +44,29 @@ for foo, bar in [(1, 2)]: print(FMT.format(**vars())) for foo, bar in [(1, 2)]: - print(FMT.format(foo=foo, bar=eval('bar'))) + print(FMT.format(foo=foo, bar=eval("bar"))) + + +def f(): + # Fixable. + for foo, bar, baz in (["1", "2", "3"],): + if foo or baz: + break + + +def f(): + # Unfixable due to usage of `bar` outside of loop. + for foo, bar, baz in (["1", "2", "3"],): + if foo or baz: + break + + print(bar) + + +def f(): + # Fixable, but not fixed in practice, since we can't tell if `bar` is used. + for foo, bar, baz in (["1", "2", "3"],): + if foo or baz: + break + + bar = 1 diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index fd27f768d3..8ec017e199 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -84,6 +84,7 @@ pub struct Checker<'a> { deferred_type_definitions: Vec<(&'a Expr, bool, DeferralContext<'a>)>, deferred_functions: Vec<(&'a Stmt, DeferralContext<'a>, VisibleScope)>, deferred_lambdas: Vec<(&'a Expr, DeferralContext<'a>)>, + deferred_for_loops: Vec<(&'a Stmt, DeferralContext<'a>)>, deferred_assignments: Vec>, // Body iteration; used to peek at siblings. body: &'a [Stmt], @@ -145,6 +146,7 @@ impl<'a> Checker<'a> { deferred_type_definitions: vec![], deferred_functions: vec![], deferred_lambdas: vec![], + deferred_for_loops: vec![], deferred_assignments: vec![], // Body iteration. body: &[], @@ -1557,7 +1559,8 @@ where .rules .enabled(&Rule::UnusedLoopControlVariable) { - flake8_bugbear::rules::unused_loop_control_variable(self, target, body); + self.deferred_for_loops + .push((stmt, (self.scope_stack.clone(), self.parents.clone()))); } if self .settings @@ -4421,6 +4424,28 @@ impl<'a> Checker<'a> { } } + fn check_deferred_for_loops(&mut self) { + self.deferred_for_loops.reverse(); + while let Some((stmt, (scopes, parents))) = self.deferred_for_loops.pop() { + self.scope_stack = scopes.clone(); + self.parents = parents.clone(); + + if let StmtKind::For { target, body, .. } | StmtKind::AsyncFor { target, body, .. } = + &stmt.node + { + if self + .settings + .rules + .enabled(&Rule::UnusedLoopControlVariable) + { + flake8_bugbear::rules::unused_loop_control_variable(self, stmt, target, body); + } + } else { + unreachable!("Expected ExprKind::Lambda"); + } + } + } + fn check_dead_scopes(&mut self) { if !(self.settings.rules.enabled(&Rule::UnusedImport) || self.settings.rules.enabled(&Rule::ImportStarUsage) @@ -5201,6 +5226,7 @@ pub fn check_ast( checker.check_deferred_type_definitions(); let mut allocator = vec![]; checker.check_deferred_string_type_definitions(&mut allocator); + checker.check_deferred_for_loops(); // Check docstrings. checker.check_definitions(); 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 f53758f437..10ba6eb48f 100644 --- a/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs +++ b/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs @@ -18,7 +18,13 @@ //! method() //! ``` -use crate::ast::types::Range; +use rustc_hash::FxHashMap; +use rustpython_ast::{Expr, ExprKind, Stmt}; +use serde::{Deserialize, Serialize}; + +use ruff_macros::derive_message_formats; + +use crate::ast::types::{BindingKind, Range, RefEquality}; use crate::ast::visitor::Visitor; use crate::ast::{helpers, visitor}; use crate::checkers::ast::Checker; @@ -26,18 +32,21 @@ use crate::define_violation; use crate::fix::Fix; use crate::registry::Diagnostic; use crate::violation::{AutofixKind, Availability, Violation}; -use ruff_macros::derive_message_formats; -use rustc_hash::FxHashMap; -use rustpython_ast::{Expr, ExprKind, Stmt}; + +#[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub enum Certainty { + Certain, + Uncertain, +} define_violation!( 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, + /// Whether the variable is certain to be unused, or merely suspect. + /// A variable _may_ be used, but undetectably so, if the loop incorporates + /// by magic control flow (e.g., `locals()`). + pub certainty: Certainty, } ); impl Violation for UnusedLoopControlVariable { @@ -45,8 +54,8 @@ impl Violation for UnusedLoopControlVariable { #[derive_message_formats] fn message(&self) -> String { - let UnusedLoopControlVariable { name, safe } = self; - if *safe { + let UnusedLoopControlVariable { name, certainty } = self; + if matches!(certainty, Certainty::Certain) { format!("Loop control variable `{name}` not used within loop body") } else { format!("Loop control variable `{name}` may not be used within loop body") @@ -54,8 +63,8 @@ impl Violation for UnusedLoopControlVariable { } fn autofix_title_formatter(&self) -> Option String> { - let UnusedLoopControlVariable { safe, .. } = self; - if *safe { + let UnusedLoopControlVariable { certainty, .. } = self; + if matches!(certainty, Certainty::Certain) { Some(|UnusedLoopControlVariable { name, .. }| { format!("Rename unused `{name}` to `_{name}`") }) @@ -92,7 +101,12 @@ where } /// B007 -pub fn unused_loop_control_variable(checker: &mut Checker, target: &Expr, body: &[Stmt]) { +pub fn unused_loop_control_variable( + checker: &mut Checker, + stmt: &Stmt, + target: &Expr, + body: &[Stmt], +) { let control_names = { let mut finder = NameFinder::new(); finder.visit_expr(target); @@ -118,21 +132,38 @@ pub fn unused_loop_control_variable(checker: &mut Checker, target: &Expr, body: continue; } - let safe = !helpers::uses_magic_variable_access(checker, body); + let certainty = if helpers::uses_magic_variable_access(checker, body) { + Certainty::Uncertain + } else { + Certainty::Certain + }; let mut diagnostic = Diagnostic::new( UnusedLoopControlVariable { name: name.to_string(), - safe, + certainty, }, Range::from_located(expr), ); - if safe && checker.patch(diagnostic.kind.rule()) { - // Prefix the variable name with an underscore. - diagnostic.amend(Fix::replacement( - format!("_{name}"), - expr.location, - expr.end_location.unwrap(), - )); + if matches!(certainty, Certainty::Certain) && checker.patch(diagnostic.kind.rule()) { + // Guard against cases in which the loop variable is used later on in the scope. + // TODO(charlie): This avoids fixing cases in which the loop variable is overridden (but + // unused) later on in the scope. + if let Some(binding) = checker.find_binding(name) { + if matches!(binding.kind, BindingKind::LoopVar) { + if !binding.used() { + if let Some(source) = &binding.source { + if source == &RefEquality(stmt) { + // Prefix the variable name with an underscore. + diagnostic.amend(Fix::replacement( + format!("_{name}"), + expr.location, + expr.end_location.unwrap(), + )); + } + } + } + } + } } checker.diagnostics.push(diagnostic); } diff --git a/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B007_B007.py.snap b/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B007_B007.py.snap index f02aacee27..6ff18da3e3 100644 --- a/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B007_B007.py.snap +++ b/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B007_B007.py.snap @@ -5,67 +5,43 @@ expression: diagnostics - kind: UnusedLoopControlVariable: name: i - safe: true + certainty: Certain 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 + fix: ~ parent: ~ - kind: UnusedLoopControlVariable: name: k - safe: true + certainty: Certain 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 + fix: ~ parent: ~ - kind: UnusedLoopControlVariable: name: i - safe: true + certainty: Certain 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 + fix: ~ parent: ~ - kind: UnusedLoopControlVariable: name: k - safe: true + certainty: Certain location: row: 30 column: 12 @@ -85,7 +61,7 @@ expression: diagnostics - kind: UnusedLoopControlVariable: name: bar - safe: false + certainty: Uncertain location: row: 34 column: 9 @@ -97,7 +73,7 @@ expression: diagnostics - kind: UnusedLoopControlVariable: name: bar - safe: false + certainty: Uncertain location: row: 38 column: 9 @@ -109,7 +85,7 @@ expression: diagnostics - kind: UnusedLoopControlVariable: name: bar - safe: false + certainty: Uncertain location: row: 42 column: 9 @@ -121,7 +97,7 @@ expression: diagnostics - kind: UnusedLoopControlVariable: name: bar - safe: false + certainty: Uncertain location: row: 46 column: 9 @@ -130,4 +106,48 @@ expression: diagnostics column: 12 fix: ~ parent: ~ +- kind: + UnusedLoopControlVariable: + name: bar + certainty: Certain + location: + row: 52 + column: 13 + end_location: + row: 52 + column: 16 + fix: + content: + - _bar + location: + row: 52 + column: 13 + end_location: + row: 52 + column: 16 + parent: ~ +- kind: + UnusedLoopControlVariable: + name: bar + certainty: Certain + location: + row: 59 + column: 13 + end_location: + row: 59 + column: 16 + fix: ~ + parent: ~ +- kind: + UnusedLoopControlVariable: + name: bar + certainty: Certain + location: + row: 68 + column: 13 + end_location: + row: 68 + column: 16 + fix: ~ + parent: ~