Add B913 for unused trailing zip values - #564
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new optional flake8-bugbear diagnostic (B913) to detect for ... in zip(...) loops where one or more trailing zip() values are discarded via _ targets, plus accompanying documentation and eval coverage. This fits the codebase’s pattern of providing conservative, opt-in correctness/style checks in the B9xx range.
Changes:
- Implement
B913inbugbear.py, including_-usage detection that avoids counting stores and comprehension-local bindings. - Document
B913inREADME.rstand add it to the UNRELEASED notes. - Add
tests/eval_files/b913.pycovering positive and “no error” cases (strict=, starred, arity, nested targets, loop-length semantics, etc.).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/eval_files/b913.py | Adds eval cases for B913 hits and non-hits across a range of zip/target patterns. |
| README.rst | Documents the new B913 rule and notes it in the changelog. |
| bugbear.py | Implements B913 detection logic, usage finder, and registers the new error code/message. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 | ||
|
|
cooperlees
left a comment
cooperlees
left a comment
There was a problem hiding this comment.
Thanks for the PR. Can you address copilots findings please.
| for left, right, (value, _) in zip(xs, ys, pairs): | ||
| consume(left, right, value) | ||
|
|
||
| for left, _, right in zip(xs, limits, ys): |
There was a problem hiding this comment.
Why is this not an error? Doesn't seem meaningfully different from the one above where limits is last.
cooperlees
left a comment
cooperlees
left a comment
There was a problem hiding this comment.
LGTM now, I do agree with Jelle tho, but seems on purpose that it's not, but does confuse me. What are we missing?
Summary
B913diagnostic for directzip()loops with unused or repeated trailing_targets_loads from stores and comprehension-local bindingsstrict=, nested-target, and loop-length casesCloses #545.
Behavior and safety
B913remains disabled by default. It only checks directzip()calls with a statically known one-to-one top-level target mapping. Calls with keywords (includingstrict=), starred targets or arguments, arity mismatches, non-trailing or nested_targets, and all-_targets are skipped.Removing a
zip()iterable can change the loop length. The diagnostic therefore recommends keeping a descriptive variable when that iterable intentionally controls iteration, and only suggests removal otherwise. Complex nested rebinding is treated conservatively as usage, favoring missed diagnostics over false positives.Validation
python -m coverage run -m pytest tests/test_bugbear.py: 81 passed, 1 skippedpython -m coverage report -m: 98% forbugbear.pypython -m mypy bugbear.py: successpre-commit run --files bugbear.py README.rst tests/eval_files/b913.py: isort, Black, flake8, and rstcheck passedgit diff --check: passedAI assistance
This pull request was fully AI-assisted and automatically prepared under the GitHub account owner's authorization. Separate AI review passes checked diagnostic correctness, prior review concerns, tests, and security boundaries; their actionable findings were addressed before publication. No issue or pull-request claim comment was posted.