From 6bf907ced61739dcb298ffc1abf3d3e9a112e41a Mon Sep 17 00:00:00 2001 From: Martin Imre Date: Wed, 14 Feb 2024 04:13:50 +0100 Subject: [PATCH] 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 --- bugbear.py | 42 +++++++++-- tests/b909.py | 158 ++++++++++++++++++++++++++++-------------- tests/test_bugbear.py | 47 +++++++++---- 3 files changed, 173 insertions(+), 74 deletions(-) diff --git a/bugbear.py b/bugbear.py index 05693b5..9767ff4 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 @@ -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 diff --git a/tests/b909.py b/tests/b909.py index 6e11603..00ee343 100644 --- a/tests/b909.py +++ b/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 diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py index 8aed9fc..53864a6 100644 --- a/tests/test_bugbear.py +++ b/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))