From 616245096a1b7bdad34944998132e2e1aa26705d Mon Sep 17 00:00:00 2001 From: Martin Imre Date: Mon, 5 Feb 2024 18:33:46 +0100 Subject: [PATCH 1/4] 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. --- tests/b909.py | 117 +++++++++++++++++++++++------------------- tests/test_bugbear.py | 36 ++++++++----- 2 files changed, 85 insertions(+), 68 deletions(-) diff --git a/tests/b909.py b/tests/b909.py index 6e11603..ad6e819 100644 --- a/tests/b909.py +++ b/tests/b909.py @@ -3,73 +3,82 @@ 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'}) -class A: - some_list: list + # no errors + elem.popitem() + elem.setdefault('foo', 1) + elem.update({'foo': 'bar'}) - def __init__(self, ls): - self.some_list = list(ls) +## sets +myset = { 1, 2, 3 } -a = A((1, 2, 3)) -for elem in a.some_list: - print(elem) - if elem % 2 == 0: - a.some_list.remove(elem) # should error +for elem 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) -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 - break + # no errors + del myset -mydicts = {'a': {'foo': 1, 'bar': 2}} +## members +class A: + some_list: list + def __init__(self, ls): + self.some_list = list(ls) -for mydict in mydicts: - if mydicts.get('a', ''): - print(mydict['foo']) # should not error - mydicts.popitem() # should error - +a = A((1, 2, 3)) +# ensure member accesses are handled +for elem in a.some_list: + a.some_list.remove(elem) + del a.some_list[2] diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py index 8aed9fc..ed0b650 100644 --- a/tests/test_bugbear.py +++ b/tests/test_bugbear.py @@ -976,20 +976,28 @@ def test_b909(self): 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(83, 4), + B909(84, 4), ] self.assertEqual(errors, self.errors(*expected)) From 877d17fa27477e6376e4b239657506d7071d0c5d Mon Sep 17 00:00:00 2001 From: Martin Imre Date: Mon, 5 Feb 2024 18:36:42 +0100 Subject: [PATCH 2/4] feat(B909): Add more container mutating functions --- bugbear.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/bugbear.py b/bugbear.py index 6a3c2be..e9b0c91 100644 --- a/bugbear.py +++ b/bugbear.py @@ -1609,6 +1609,13 @@ class B909Checker(ast.NodeVisitor): "insert", "pop", "popitem", + "setdefault", + "update", + "intersection_update", + "difference_update", + "symmetric_difference_update", + "add", + "discard", ) def __init__(self, name: str): From fde6f9eb6dbb24745ca173138476140347713c8a Mon Sep 17 00:00:00 2001 From: Martin Imre Date: Thu, 8 Feb 2024 20:07:28 +0100 Subject: [PATCH 3/4] 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". --- bugbear.py | 13 ++++++++++- tests/b909.py | 51 ++++++++++++++++++++++++++++++------------- tests/test_bugbear.py | 11 +++++++++- 3 files changed, 58 insertions(+), 17 deletions(-) diff --git a/bugbear.py b/bugbear.py index e9b0c91..1fc7e1d 100644 --- a/bugbear.py +++ b/bugbear.py @@ -1622,12 +1622,23 @@ def __init__(self, name: str): self.name = name self.mutations = [] + 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.append(node) + self.generic_visit(node) + + def visit_AugAssign(self, node: ast.AugAssign): + if _to_name_str(node.target) == self.name: + self.mutations.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) diff --git a/tests/b909.py b/tests/b909.py index ad6e819..06f35c6 100644 --- a/tests/b909.py +++ b/tests/b909.py @@ -3,7 +3,7 @@ B999 - on lines 11, 25, 26, 40, 46 """ -## lists +# lists some_list = [1, 2, 3] some_other_list = [1, 2, 3] @@ -19,11 +19,11 @@ 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: + if elem == 3: break # non-errors @@ -39,12 +39,12 @@ break -## dicts +# dicts mydicts = {'a': {'foo': 1, 'bar': 2}} for elem in mydicts: # errors - mydicts.popitem() + mydicts.popitem() mydicts.setdefault('foo', 1) mydicts.update({'foo': 'bar'}) @@ -53,32 +53,53 @@ elem.setdefault('foo', 1) elem.update({'foo': 'bar'}) -## sets +# sets -myset = { 1, 2, 3 } +myset = {1, 2, 3} for elem in myset: # errors - myset.update({4,5}) - myset.intersection_update({4,5}) - myset.difference_update({4,5}) - myset.symmetric_difference_update({4,5}) + 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 +# members class A: some_list: list + def __init__(self, ls): self.some_list = list(ls) + a = A((1, 2, 3)) # ensure member accesses are handled for elem in a.some_list: - a.some_list.remove(elem) - del a.some_list[2] + 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 diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py index ed0b650..fc3d7cb 100644 --- a/tests/test_bugbear.py +++ b/tests/test_bugbear.py @@ -996,8 +996,17 @@ def test_b909(self): B909(65, 4), B909(66, 4), B909(67, 4), - B909(83, 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), ] self.assertEqual(errors, self.errors(*expected)) From 2cfde403dd187ecee272f8cc59589fb3b43a54ff Mon Sep 17 00:00:00 2001 From: Martin Imre Date: Thu, 8 Feb 2024 20:19:16 +0100 Subject: [PATCH 4/4] feat(b909): Ignore mutations followed by unconfiditional break --- bugbear.py | 26 ++++++++++++++++++-------- tests/b909.py | 26 +++++++++++++++++++++++++- tests/test_bugbear.py | 2 +- 3 files changed, 44 insertions(+), 10 deletions(-) diff --git a/bugbear.py b/bugbear.py index 1fc7e1d..6663cc4 100644 --- a/bugbear.py +++ b/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 @@ -1583,7 +1583,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)) @@ -1620,17 +1622,18 @@ class B909Checker(ast.NodeVisitor): 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.append(node) + 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.append(node) + self.mutations[self._conditional_block].append(node) self.generic_visit(node) def visit_Delete(self, node: ast.Delete): @@ -1644,7 +1647,7 @@ def visit_Delete(self, node: ast.Delete): 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): @@ -1656,17 +1659,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 diff --git a/tests/b909.py b/tests/b909.py index 06f35c6..00ee343 100644 --- a/tests/b909.py +++ b/tests/b909.py @@ -57,7 +57,7 @@ myset = {1, 2, 3} -for elem in myset: +for _ in myset: # errors myset.update({4, 5}) myset.intersection_update({4, 5}) @@ -103,3 +103,27 @@ def __init__(self, ls): foo &= bar foo -= bar foo ^= bar + + +# more tests for unconditional breaks +for _ in foo: + foo.remove(1) + for _ in bar: + bar.remove(1) + break + 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 diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py index fc3d7cb..53864a6 100644 --- a/tests/test_bugbear.py +++ b/tests/test_bugbear.py @@ -974,7 +974,6 @@ 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(12, 4), B909(13, 4), @@ -1007,6 +1006,7 @@ def test_b909(self): B909(103, 4), B909(104, 4), B909(105, 4), + B909(125, 4), ] self.assertEqual(errors, self.errors(*expected))