diff --git a/README.rst b/README.rst index bb92e02..5eeb3ae 100644 --- a/README.rst +++ b/README.rst @@ -389,6 +389,14 @@ The ``strict=`` argument was added in Python 3.13, so don't enable this flag for **B912**: ``map()`` without an explicit `strict=` parameter set. ``strict=True`` causes the resulting iterator to raise a ``ValueError`` if the arguments are exhausted at differing lengths. +.. _B913: + +**B913**: In a ``zip()`` loop with at least one retained target, one or more +trailing values are discarded by unused or repeated ``_`` targets. Those +iterables still affect how many times the loop runs. If this is intentional, use +descriptive variables; otherwise remove the matching arguments and targets. +Calls with an explicit ``strict=`` argument or starred unpacking are not checked. + .. _B950: **B950**: Line too long. This is a pragmatic equivalent of @@ -502,6 +510,7 @@ UNRELEASED * B018: handle also useless calls such as `isinstance(x, int)` without assigning or using the result * B031: don't count a store-context reference (e.g. an annotation target like `group: T`) as a use of the `groupby` generator (#465) * B902: don't raise a false positive on a metaclass defined with a dotted base such as `abc.ABCMeta` or `enum.EnumMeta` (#411) +* B913: Add an optional check for unused trailing ``_`` targets in ``zip()`` loops (#545) 25.11.29 ~~~~~~~~ diff --git a/bugbear.py b/bugbear.py index e7ae9c9..3e66e5b 100644 --- a/bugbear.py +++ b/bugbear.py @@ -613,6 +613,7 @@ def visit_For(self, node: ast.For) -> None: self.check_for_b023(node) self.check_for_b031(node) self.check_for_b909(node) + self.check_for_b913(node) self.generic_visit(node) def visit_AsyncFor(self, node: ast.AsyncFor) -> None: @@ -1650,6 +1651,43 @@ def check_for_b912(self, node: ast.Call) -> None: if not any(kw.arg == "strict" for kw in node.keywords): self.add_error("B912", node) + def check_for_b913(self, node: ast.For) -> None: + if not ( + isinstance(node.target, (ast.Tuple, ast.List)) + and isinstance(node.iter, ast.Call) + and isinstance(node.iter.func, ast.Name) + and node.iter.func.id == "zip" + ): + return + + targets = node.target.elts + arguments = node.iter.args + if ( + len(targets) != len(arguments) + or any(isinstance(target, ast.Starred) for target in targets) + or any(isinstance(argument, ast.Starred) for argument in arguments) + or node.iter.keywords + ): + return + + trailing_underscores = 0 + for target in reversed(targets): + if isinstance(target, ast.Name) and target.id == "_": + trailing_underscores += 1 + else: + break + if trailing_underscores == 0 or trailing_underscores == len(targets): + return + + if trailing_underscores == 1: + body_names = B913UsageFinder() + body_names.visit(node.body + node.orelse) + if "_" in body_names.names: + return + + first_discarded = targets[-trailing_underscores] + self.add_error("B913", first_discarded) + def check_for_b906(self, node: ast.FunctionDef) -> None: if not node.name.startswith("visit_"): return @@ -2106,6 +2144,55 @@ def visit(self, node): return node +class B913UsageFinder(NameFinder): + """Find loads of ``_`` that refer to a surrounding loop target.""" + + @staticmethod + def _binds_underscore(node: ast.expr) -> bool: + return "_" in names_from_assignments(node) + + def visit_Name(self, node: ast.Name) -> None: # noqa: B906 + if node.id == "_" and isinstance(node.ctx, ast.Load): + super().visit_Name(node) + + def _visit_comprehension( + self, + generators: list[ast.comprehension], + values: list[ast.expr], + ) -> None: + shadowed = False + for index, generator in enumerate(generators): + if index == 0 or not shadowed: + self.visit(generator.iter) + if not shadowed: + self.visit(generator.target) + if self._binds_underscore(generator.target): + shadowed = True + if not shadowed: + self.visit(generator.ifs) + if not shadowed: + self.visit(values) + + def visit_GeneratorExp(self, node: ast.GeneratorExp) -> None: + self._visit_comprehension(node.generators, [node.elt]) + + def visit_ListComp(self, node: ast.ListComp) -> None: + self._visit_comprehension(node.generators, [node.elt]) + + def visit_SetComp(self, node: ast.SetComp) -> None: + self._visit_comprehension(node.generators, [node.elt]) + + def visit_DictComp(self, node: ast.DictComp) -> None: + self._visit_comprehension(node.generators, [node.key, node.value]) + + def visit_AugAssign(self, node: ast.AugAssign) -> None: + self.visit(node.value) + if isinstance(node.target, ast.Name) and node.target.id == "_": + self.names.setdefault("_", []).append(node.target) + else: + self.visit(node.target) + + @attr.s class NamedExprFinder(ast.NodeVisitor): """Finds names defined through an ast.NamedExpr. @@ -2626,6 +2713,13 @@ def __call__(self, lineno: int, col: int, vars: tuple[object, ...] = ()) -> erro message="B911 `itertools.batched()` without an explicit `strict=` parameter." ), "B912": Error(message="B912 `map()` without an explicit `strict=` parameter."), + "B913": Error( + message=( + "B913 Trailing `zip()` values are discarded by unused `_` targets. " + "If those iterables intentionally control loop length, use named " + "variables; otherwise remove the arguments and targets." + ) + ), "B950": Error(message="B950 line too long ({} > {} characters)"), } @@ -2642,5 +2736,6 @@ def __call__(self, lineno: int, col: int, vars: tuple[object, ...] = ()) -> erro "B910", "B911", "B912", + "B913", "B950", ] diff --git a/tests/eval_files/b913.py b/tests/eval_files/b913.py new file mode 100644 index 0000000..872d7a2 --- /dev/null +++ b/tests/eval_files/b913.py @@ -0,0 +1,76 @@ +# OPTIONS: select=["B913"] + +# Errors: direct trailing underscore targets whose values are unused. +for left, right, _ in zip(xs, ys, limits): # B913: 17 + consume(left, right) + +for [left, right, _] in zip(xs, ys, limits): # B913: 18 + consume(left, right) + +for left, right, _, _ in zip(xs, ys, limits, extras): # B913: 17 + consume(left, right) + +for left, _ in zip(xs, limits): # B913: 10 + consume(left) + +# Stores, nested bindings, and repeated targets do not use every zip value. +for left, right, _ in zip(xs, ys, limits): # B913: 17 + consume(left, right) + _ = object() + +for left, right, _ in zip(xs, ys, limits): # B913: 17 + consume(left, right, [item for _ in xs]) + +for left, right, _, _ in zip(xs, ys, limits, extras): # B913: 17 + consume(left, right, _) + +# No errors: the discarded value is used or its iteration semantics are explicit. +for left, right, _ in zip(xs, ys, limits): + consume(left, right, _) + +for left, right, _ in zip(xs, ys, limits): + consume(left, right) +else: + consume(_) + +for left, right, _ in zip(xs, ys, limits, strict=True): + consume(left, right) + +# No errors: the target-to-argument mapping is not direct and statically known. +for left, *_rest, _ in zip(xs, ys, limits): + consume(left) + +for left, right, _ in zip(xs, *iterables): + consume(left, right) + +for left, right, (value, _) in zip(xs, ys, pairs): + consume(left, right, value) + +for left, _, right in zip(xs, limits, ys): + consume(left, right) + +for _, _ in zip(xs, ys): + pass + +for left, right, _ in builtins.zip(xs, ys, limits): + consume(left, right) + +for left, right, _ in zip(xs, ys): + consume(left, right) + +# No errors: these reads refer to the zip target rather than a nested binding. +for left, right, _ in zip(xs, ys, limits): + consume(left, right, [consume(_) for item in items]) + +for left, right, _ in zip(xs, ys, limits): + consume(left, right, [item for item in _]) + +for left, right, _ in zip(xs, ys, limits): + consume(left, right, [item for _.value in items]) + +for left, right, _ in zip(xs, ys, limits): + consume(left, right, [item for target[_] in items]) + +for left, right, _ in zip(xs, ys, limits): + _ += 1 + consume(left, right)