Avoid renaming unused loop variables with deferred usages (#2509)

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

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<DeferralContext<'a>>,
     // 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();

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;
+#[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize)]
-use rustpython_ast::{Expr, ExprKind, Stmt};
+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
+        /// Whether the variable is certain to be unused, or merely suspect.
-        /// rename if it _might_ be used by magic control flow (e.g.,
+        /// A variable _may_ be used, but undetectably so, if the loop incorporates
-        /// `locals()`).
+        /// by magic control flow (e.g., `locals()`).
-        pub safe: bool,
+        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;
+        let UnusedLoopControlVariable { name, certainty } = self;
-        if *safe {
+        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<fn(&Self) -> String> {
-        let UnusedLoopControlVariable { safe, .. } = self;
+        let UnusedLoopControlVariable { certainty, .. } = self;
-        if *safe {
+        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()) {
+        if matches!(certainty, Certainty::Certain) && checker.patch(diagnostic.kind.rule()) {
-            // Prefix the variable name with an underscore.
+            // Guard against cases in which the loop variable is used later on in the scope.
-            diagnostic.amend(Fix::replacement(
+            // TODO(charlie): This avoids fixing cases in which the loop variable is overridden (but
-                format!("_{name}"),
+            // unused) later on in the scope.
-                expr.location,
+            if let Some(binding) = checker.find_binding(name) {
-                expr.end_location.unwrap(),
+                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);
     }

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:
+  fix: ~
-    content:
-      - _i
-    location:
-      row: 6
-      column: 4
-    end_location:
-      row: 6
-      column: 5
   parent: ~
 - kind:
     UnusedLoopControlVariable:
       name: k
-      safe: true
+      certainty: Certain
   location:
     row: 18
     column: 12
   end_location:
     row: 18
     column: 13
-  fix:
+  fix: ~
-    content:
-      - _k
-    location:
-      row: 18
-      column: 12
-    end_location:
-      row: 18
-      column: 13
   parent: ~
 - kind:
     UnusedLoopControlVariable:
       name: i
-      safe: true
+      certainty: Certain
   location:
     row: 30
     column: 4
   end_location:
     row: 30
     column: 5
-  fix:
+  fix: ~
-    content:
-      - _i
-    location:
-      row: 30
-      column: 4
-    end_location:
-      row: 30
-      column: 5
   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: ~