Avoid infinite renames for unused-loop-control-variable (#2581)

astral-sh · Feb 5, 2023 · 84be1df · 84be1df
1 parent 6b3ae1a
commit 84be1df
Show file tree

Hide file tree

Showing 3 changed files with 90 additions and 35 deletions.
diff --git a/resources/test/fixtures/flake8_bugbear/B007.py b/resources/test/fixtures/flake8_bugbear/B007.py
@@ -80,3 +80,9 @@ def f():
 
     bar = 1
     print(bar)
+
+
+# Unfixable due to trailing underscore (`_line_` wouldn't be considered an ignorable
+# variable name).
+for line_ in range(self.header_lines):
+     fp.readline()
diff --git a/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs b/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs
@@ -44,7 +44,9 @@ define_violation!(
     pub struct UnusedLoopControlVariable {
         /// The name of the loop control variable.
         pub name: String,
-        /// Whether the variable is certain to be unused, or merely suspect.
+        /// The name to which the variable should be renamed, if it can be safely renamed.
+        pub rename: Option<String>,
+        /// Whether the variable is certain to be unused in the loop body, 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,
@@ -55,7 +57,9 @@ impl Violation for UnusedLoopControlVariable {
 
     #[derive_message_formats]
     fn message(&self) -> String {
-        let UnusedLoopControlVariable { name, certainty } = self;
+        let UnusedLoopControlVariable {
+            name, certainty, ..
+        } = self;
         if matches!(certainty, Certainty::Certain) {
             format!("Loop control variable `{name}` not used within loop body")
         } else {
@@ -64,10 +68,13 @@ impl Violation for UnusedLoopControlVariable {
     }
 
     fn autofix_title_formatter(&self) -> Option<fn(&Self) -> String> {
-        let UnusedLoopControlVariable { certainty, .. } = self;
-        if matches!(certainty, Certainty::Certain) {
-            Some(|UnusedLoopControlVariable { name, .. }| {
-                format!("Rename unused `{name}` to `_{name}`")
+        let UnusedLoopControlVariable {
+            certainty, rename, ..
+        } = self;
+        if matches!(certainty, Certainty::Certain) && rename.is_some() {
+            Some(|UnusedLoopControlVariable { name, rename, .. }| {
+                let rename = rename.as_ref().unwrap();
+                format!("Rename unused `{name}` to `_{rename}`")
             })
         } else {
             None
@@ -133,50 +140,67 @@ pub fn unused_loop_control_variable(
             continue;
         }
 
+        // Avoid fixing any variables that _may_ be used, but undetectably so.
         let certainty = if helpers::uses_magic_variable_access(checker, body) {
             Certainty::Uncertain
         } else {
             Certainty::Certain
         };
+
+        // Attempt to rename the variable by prepending an underscore, but avoid applying the fix
+        // if doing so wouldn't actually cause us to ignore the violation in the next pass.
+        let rename = format!("_{name}");
+        let rename = if checker
+            .settings
+            .dummy_variable_rgx
+            .is_match(rename.as_str())
+        {
+            Some(rename)
+        } else {
+            None
+        };
+
         let mut diagnostic = Diagnostic::new(
             UnusedLoopControlVariable {
                 name: name.to_string(),
+                rename: rename.clone(),
                 certainty,
             },
             Range::from_located(expr),
         );
-        if matches!(certainty, Certainty::Certain) && checker.patch(diagnostic.kind.rule()) {
-            // Find the `BindingKind::LoopVar` corresponding to the name.
-            let scope = checker.current_scope();
-            if let Some(binding) = iter::once(scope.bindings.get(name))
-                .flatten()
-                .chain(
-                    iter::once(scope.rebounds.get(name))
-                        .flatten()
-                        .into_iter()
-                        .flatten(),
-                )
-                .find_map(|index| {
-                    let binding = &checker.bindings[*index];
-                    if let Some(source) = &binding.source {
-                        if source == &RefEquality(stmt) {
-                            Some(binding)
+        if let Some(rename) = rename {
+            if matches!(certainty, Certainty::Certain) && checker.patch(diagnostic.kind.rule()) {
+                // Find the `BindingKind::LoopVar` corresponding to the name.
+                let scope = checker.current_scope();
+                if let Some(binding) = iter::once(scope.bindings.get(name))
+                    .flatten()
+                    .chain(
+                        iter::once(scope.rebounds.get(name))
+                            .flatten()
+                            .into_iter()
+                            .flatten(),
+                    )
+                    .find_map(|index| {
+                        let binding = &checker.bindings[*index];
+                        if let Some(source) = &binding.source {
+                            if source == &RefEquality(stmt) {
+                                Some(binding)
+                            } else {
+                                None
+                            }
                         } else {
                             None
                         }
-                    } else {
-                        None
-                    }
-                })
-            {
-                if matches!(binding.kind, BindingKind::LoopVar) {
-                    if !binding.used() {
-                        // Prefix the variable name with an underscore.
-                        diagnostic.amend(Fix::replacement(
-                            format!("_{name}"),
-                            expr.location,
-                            expr.end_location.unwrap(),
-                        ));
+                    })
+                {
+                    if matches!(binding.kind, BindingKind::LoopVar) {
+                        if !binding.used() {
+                            diagnostic.amend(Fix::replacement(
+                                rename,
+                                expr.location,
+                                expr.end_location.unwrap(),
+                            ));
+                        }
                     }
                 }
             }

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
@@ -5,6 +5,7 @@ expression: diagnostics
 - kind:
     UnusedLoopControlVariable:
       name: i
+      rename: _i
       certainty: Certain
   location:
     row: 6
@@ -17,6 +18,7 @@ expression: diagnostics
 - kind:
     UnusedLoopControlVariable:
       name: k
+      rename: _k
       certainty: Certain
   location:
     row: 18
@@ -37,6 +39,7 @@ expression: diagnostics
 - kind:
     UnusedLoopControlVariable:
       name: i
+      rename: _i
       certainty: Certain
   location:
     row: 30
@@ -49,6 +52,7 @@ expression: diagnostics
 - kind:
     UnusedLoopControlVariable:
       name: k
+      rename: _k
       certainty: Certain
   location:
     row: 30
@@ -69,6 +73,7 @@ expression: diagnostics
 - kind:
     UnusedLoopControlVariable:
       name: bar
+      rename: _bar
       certainty: Uncertain
   location:
     row: 34
@@ -81,6 +86,7 @@ expression: diagnostics
 - kind:
     UnusedLoopControlVariable:
       name: bar
+      rename: _bar
       certainty: Uncertain
   location:
     row: 38
@@ -93,6 +99,7 @@ expression: diagnostics
 - kind:
     UnusedLoopControlVariable:
       name: bar
+      rename: _bar
       certainty: Uncertain
   location:
     row: 42
@@ -105,6 +112,7 @@ expression: diagnostics
 - kind:
     UnusedLoopControlVariable:
       name: bar
+      rename: _bar
       certainty: Uncertain
   location:
     row: 46
@@ -117,6 +125,7 @@ expression: diagnostics
 - kind:
     UnusedLoopControlVariable:
       name: bar
+      rename: _bar
       certainty: Certain
   location:
     row: 52
@@ -137,6 +146,7 @@ expression: diagnostics
 - kind:
     UnusedLoopControlVariable:
       name: bar
+      rename: _bar
       certainty: Certain
   location:
     row: 59
@@ -149,6 +159,7 @@ expression: diagnostics
 - kind:
     UnusedLoopControlVariable:
       name: bar
+      rename: _bar
       certainty: Certain
   location:
     row: 68
@@ -169,6 +180,7 @@ expression: diagnostics
 - kind:
     UnusedLoopControlVariable:
       name: bar
+      rename: _bar
       certainty: Certain
   location:
     row: 77
@@ -186,4 +198,17 @@ expression: diagnostics
       row: 77
       column: 16
   parent: ~
+- kind:
+    UnusedLoopControlVariable:
+      name: line_
+      rename: ~
+      certainty: Certain
+  location:
+    row: 87
+    column: 4
+  end_location:
+    row: 87
+    column: 9
+  fix: ~
+  parent: ~