Remove discard, remove, and pop allowance for loop-iterator-mutation

astral-sh · Jul 17, 2024 · ded5f3c · ded5f3c
1 parent 9a2dafb
commit ded5f3c
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 18 deletions.
diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B909.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B909.py
@@ -152,7 +152,7 @@ def __init__(self, ls):
     else:
         break
 
-# should not error
+# should error
 for elem in some_list:
     del some_list[elem]
     some_list[elem] = 1

diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/loop_iterator_mutation.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/loop_iterator_mutation.rs
@@ -186,7 +186,7 @@ impl<'a> LoopMutationsVisitor<'a> {
     }
 
     /// Handle, e.g., `items.append(1)`.
-    fn handle_call(&mut self, func: &Expr, arguments: &Arguments) {
+    fn handle_call(&mut self, func: &Expr) {
         if let Expr::Attribute(ExprAttribute {
             range,
             value,
@@ -197,17 +197,6 @@ impl<'a> LoopMutationsVisitor<'a> {
             if is_mutating_function(attr.as_str()) {
                 // Find, e.g., `items.remove(1)`.
                 if ComparableExpr::from(self.iter) == ComparableExpr::from(value) {
-                    // But allow, e.g., `for item in items: items.remove(item)`.
-                    if matches!(attr.as_str(), "remove" | "discard" | "pop") {
-                        if arguments.len() == 1 {
-                            if let [arg] = &*arguments.args {
-                                if ComparableExpr::from(self.target) == ComparableExpr::from(arg) {
-                                    return;
-                                }
-                            }
-                        }
-                    }
-
                     self.add_mutation(*range);
                 }
             }
@@ -283,11 +272,8 @@ impl<'a> Visitor<'a> for LoopMutationsVisitor<'a> {
 
     fn visit_expr(&mut self, expr: &'a Expr) {
         // Ex) `items.append(1)`
-        if let Expr::Call(ExprCall {
-            func, arguments, ..
-        }) = expr
-        {
-            self.handle_call(func, arguments);
+        if let Expr::Call(ExprCall { func, .. }) = expr {
+            self.handle_call(func);
         }
 
         visitor::walk_expr(self, expr);

diff --git a/...les/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B909_B909.py.snap b/...les/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B909_B909.py.snap
@@ -339,3 +339,20 @@ B909.py:150:8: B909 Mutation to loop iterable `some_list` during iteration
 151 |         pass
 152 |     else:
     |
+
+B909.py:159:5: B909 Mutation to loop iterable `some_list` during iteration
+    |
+157 |     del some_list[elem]
+158 |     some_list[elem] = 1
+159 |     some_list.remove(elem)
+    |     ^^^^^^^^^^^^^^^^ B909
+160 |     some_list.discard(elem)
+    |
+
+B909.py:160:5: B909 Mutation to loop iterable `some_list` during iteration
+    |
+158 |     some_list[elem] = 1
+159 |     some_list.remove(elem)
+160 |     some_list.discard(elem)
+    |     ^^^^^^^^^^^^^^^^^ B909
+    |