B909 improvements (#460)

* refactor(B909): Clean up test for B909

This splits the convoluted test files into clearer categories of what is
being tested.
Previously I had the test file contain semantically correct code, that
would also run just fine, except for the errors.
I scratched that idea now to aid the readability and understandability
of the test file.

* feat(B909): Add more container mutating functions

* feat(b909): Add more cases to detect for b909

This commit takes care of detecting mutations stemming from
AugAssign and Assign.
Also removes incorrect detection of "del foo".

* feat(b909): Ignore mutations followed by unconfiditional break

Author: mimre25

bugbear.py

@@ -8,7 +8,7 @@
 import re
 import sys
 import warnings
-from collections import namedtuple
+from collections import defaultdict, namedtuple
 from contextlib import suppress
 from functools import lru_cache, partial
 from keyword import iskeyword
@@ -1585,7 +1585,9 @@ def check_for_b909(self, node: ast.For):
             return
         checker = B909Checker(name)
         checker.visit(node.body)
-        for mutation in checker.mutations:
+        for mutation in itertools.chain.from_iterable(
+            m for m in checker.mutations.values()
+        ):
             self.errors.append(B909(mutation.lineno, mutation.col_offset))
 
 
@@ -1611,24 +1613,43 @@ class B909Checker(ast.NodeVisitor):
         "insert",
         "pop",
         "popitem",
+        "setdefault",
+        "update",
+        "intersection_update",
+        "difference_update",
+        "symmetric_difference_update",
+        "add",
+        "discard",
     )
 
     def __init__(self, name: str):
         self.name = name
-        self.mutations = []
+        self.mutations = defaultdict(list)
+        self._conditional_block = 0
+
+    def visit_Assign(self, node: ast.Assign):
+        for target in node.targets:
+            if isinstance(target, ast.Subscript) and _to_name_str(target.value):
+                self.mutations[self._conditional_block].append(node)
+        self.generic_visit(node)
+
+    def visit_AugAssign(self, node: ast.AugAssign):
+        if _to_name_str(node.target) == self.name:
+            self.mutations[self._conditional_block].append(node)
+        self.generic_visit(node)
 
     def visit_Delete(self, node: ast.Delete):
         for target in node.targets:
             if isinstance(target, ast.Subscript):
                 name = _to_name_str(target.value)
             elif isinstance(target, (ast.Attribute, ast.Name)):
-                name = _to_name_str(target)
+                name = ""  # ignore "del foo"
             else:
                 name = ""  # fallback
                 self.generic_visit(target)
 
             if name == self.name:
-                self.mutations.append(node)
+                self.mutations[self._conditional_block].append(node)
 
     def visit_Call(self, node: ast.Call):
         if isinstance(node.func, ast.Attribute):
@@ -1640,17 +1661,24 @@ def visit_Call(self, node: ast.Call):
                 function_object == self.name
                 and function_name in self.MUTATING_FUNCTIONS
             ):
-                self.mutations.append(node)
+                self.mutations[self._conditional_block].append(node)
 
         self.generic_visit(node)
 
+    def visit_If(self, node: ast.If):
+        self._conditional_block += 1
+        self.visit(node.body)
+        self._conditional_block += 1
+
     def visit(self, node):
         """Like super-visit but supports iteration over lists."""
         if not isinstance(node, list):
             return super().visit(node)
 
         for elem in node:
-            super().visit(elem)
+            if isinstance(elem, ast.Break):
+                self.mutations[self._conditional_block].clear()
+            self.visit(elem)
         return node
 
 

tests/b909.py

@@ -3,30 +3,74 @@
 B999 - on lines 11, 25, 26, 40, 46
 """
 
-
-some_list = [1, 2, 3]
-for elem in some_list:
-    print(elem)
-    if elem % 2 == 0:
-        some_list.remove(elem)  # should error
+# lists
 
 some_list = [1, 2, 3]
 some_other_list = [1, 2, 3]
 for elem in some_list:
-    print(elem)
-    if elem % 2 == 0:
-        some_other_list.remove(elem)  # should not error
-        del some_other_list
+    # errors
+    some_list.remove(elem)
+    del some_list[2]
+    some_list.append(elem)
+    some_list.sort()
+    some_list.reverse()
+    some_list.clear()
+    some_list.extend([1, 2])
+    some_list.insert(1, 1)
+    some_list.pop(1)
+    some_list.pop()
+
+    # conditional break should error
+    if elem == 2:
+        some_list.remove(elem)
+        if elem == 3:
+            break
+
+    # non-errors
+    some_other_list.remove(elem)
+    del some_list
+    del some_other_list
+    found_idx = some_list.index(elem)
+    some_list = 3
+
+    # unconditional break should not error
+    if elem == 2:
+        some_list.remove(elem)
+        break
 
 
-some_list = [1, 2, 3]
-for elem in some_list:
-    print(elem)
-    if elem % 2 == 0:
-        del some_list[2]  # should error
-        del some_list
+# dicts
+mydicts = {'a': {'foo': 1, 'bar': 2}}
+
+for elem in mydicts:
+    # errors
+    mydicts.popitem()
+    mydicts.setdefault('foo', 1)
+    mydicts.update({'foo': 'bar'})
+
+    # no errors
+    elem.popitem()
+    elem.setdefault('foo', 1)
+    elem.update({'foo': 'bar'})
+
+# sets
+
+myset = {1, 2, 3}
+
+for _ in myset:
+    # errors
+    myset.update({4, 5})
+    myset.intersection_update({4, 5})
+    myset.difference_update({4, 5})
+    myset.symmetric_difference_update({4, 5})
+    myset.add(4)
+    myset.discard(3)
+
+    # no errors
+    del myset
 
 
+# members
 class A:
     some_list: list
 
@@ -35,41 +79,51 @@ def __init__(self, ls):
 
 
 a = A((1, 2, 3))
+# ensure member accesses are handled
 for elem in a.some_list:
-    print(elem)
-    if elem % 2 == 0:
-        a.some_list.remove(elem)  # should error
-
-a = A((1, 2, 3))
-for elem in a.some_list:
-    print(elem)
-    if elem % 2 == 0:
-        del a.some_list[2]  # should error
-
-
-
-some_list = [1, 2, 3]
-for elem in some_list:
-    print(elem)
-    if elem == 2:
-        found_idx = some_list.index(elem)  # should not error
-        some_list.append(elem)  # should error
-        some_list.sort()  # should error
-        some_list.reverse()  # should error
-        some_list.clear()  # should error
-        some_list.extend([1,2])  # should error
-        some_list.insert(1, 1)  # should error
-        some_list.pop(1) # should error
-        some_list.pop() # should error
-        some_list = 3 # should error
+    a.some_list.remove(elem)
+    del a.some_list[2]
+
+
+# Augassign
+
+foo = [1, 2, 3]
+bar = [4, 5, 6]
+for _ in foo:
+    foo *= 2
+    foo += bar
+    foo[1] = 9 #todo
+    foo[1:2] = bar
+    foo[1:2:3] = bar
+
+foo = {1,2,3}
+bar = {4,5,6}
+for _ in foo:
+    foo |= bar
+    foo &= bar
+    foo -= bar
+    foo ^= bar
+
+
+# more tests for unconditional breaks
+for _ in foo:
+    foo.remove(1)
+    for _ in bar:
+        bar.remove(1)
         break
-
-
-
-mydicts = {'a': {'foo': 1, 'bar': 2}}
-
-for mydict in mydicts:
-    if mydicts.get('a', ''):
-        print(mydict['foo'])  # should not error
-        mydicts.popitem() # should error
-        
+    break
+
+# should not error
+for _ in foo:
+    foo.remove(1)
+    for _ in bar:
+        ...
+    break
+
+# should error (?)
+for _ in foo:
+    foo.remove(1)
+    if bar:
+        bar.remove(1)
+        break
+    break

tests/test_bugbear.py

@@ -974,22 +974,39 @@ def test_b909(self):
         mock_options = Namespace(select=[], extend_select=["B909"])
         bbc = BugBearChecker(filename=str(filename), options=mock_options)
         errors = list(bbc.run())
-        print(errors)
         expected = [
-            B909(11, 8),
-            B909(26, 8),
-            B909(27, 8),
-            B909(41, 8),
-            B909(47, 8),
-            B909(56, 8),
-            B909(57, 8),
-            B909(58, 8),
-            B909(59, 8),
-            B909(60, 8),
-            B909(61, 8),
-            B909(62, 8),
-            B909(63, 8),
-            B909(74, 8),
+            B909(12, 4),
+            B909(13, 4),
+            B909(14, 4),
+            B909(15, 4),
+            B909(16, 4),
+            B909(17, 4),
+            B909(18, 4),
+            B909(19, 4),
+            B909(20, 4),
+            B909(21, 4),
+            B909(25, 8),
+            B909(47, 4),
+            B909(48, 4),
+            B909(49, 4),
+            B909(62, 4),
+            B909(63, 4),
+            B909(64, 4),
+            B909(65, 4),
+            B909(66, 4),
+            B909(67, 4),
+            B909(84, 4),
+            B909(85, 4),
+            B909(93, 4),
+            B909(94, 4),
+            B909(95, 4),
+            B909(96, 4),
+            B909(97, 4),
+            B909(102, 4),
+            B909(103, 4),
+            B909(104, 4),
+            B909(105, 4),
+            B909(125, 4),
         ]
         self.assertEqual(errors, self.errors(*expected))