Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,8 @@ UNRELEASED
* B020: don't flag `for self.a in self.b`: rebinding an attribute is not rebinding the
base name, so two different attributes of the same object are two bindings (#248)
* 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)
* B031: don't count a store-context reference (e.g. an annotation target like `group: T`) as a use of the `groupby` generator,
and don't treat references in mutually exclusive ``if``/``elif``/``else`` branches as multiple uses (#465)
* B902: don't raise a false positive on a metaclass defined with a dotted base such as `abc.ABCMeta` or `enum.EnumMeta` (#411)

25.11.29
Expand Down
93 changes: 64 additions & 29 deletions bugbear.py
Original file line number Diff line number Diff line change
Expand Up @@ -374,11 +374,6 @@ def children_in_scope(node: ast.AST) -> Iterator[ast.AST]:
yield from children_in_scope(child)


def walk_list(nodes: Sequence[ast.AST]) -> Iterator[ast.AST]:
for node in nodes:
yield from ast.walk(node)


def _typesafe_issubclass(cls: type, class_or_tuple: type | tuple[type, ...]) -> bool:
try:
return issubclass(cls, class_or_tuple)
Expand Down Expand Up @@ -1367,6 +1362,69 @@ def check_for_b026(self, call: ast.Call) -> None:
):
self.add_error("B026", starred)

def _check_b031_group_usages(
self,
nodes: Sequence[ast.AST],
group_name: str,
num_usages: int = 0,
repeated: bool = False,
) -> int:
for node in nodes:
num_usages = self._check_b031_group_usage(
node, group_name, num_usages, repeated
)
return num_usages

def _check_b031_group_usage(
self,
node: ast.AST,
group_name: str,
num_usages: int,
repeated: bool,
) -> int:
if isinstance(node, ast.Name):
if node.id == group_name and isinstance(node.ctx, ast.Load):
num_usages += 1
if repeated or num_usages > 1:
self.add_error("B031", node, node.id)
return num_usages

if isinstance(node, ast.If):
num_usages = self._check_b031_group_usage(
node.test, group_name, num_usages, repeated
)
# Only one branch can execute, so keep the largest path count
# instead of adding usages from mutually exclusive branches.
return max(
self._check_b031_group_usages(
node.body, group_name, num_usages, repeated
),
self._check_b031_group_usages(
node.orelse, group_name, num_usages, repeated
),
)

if isinstance(node, ast.For):
num_usages = self._check_b031_group_usage(
node.target, group_name, num_usages, repeated
)
num_usages = self._check_b031_group_usage(
node.iter, group_name, num_usages, repeated
)
# Any body reference may run once per nested loop iteration.
num_usages = self._check_b031_group_usages(
node.body, group_name, num_usages, True
)
return self._check_b031_group_usages(
node.orelse, group_name, num_usages, repeated
)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I added failing fixtures for conditional branches inside both while and async for, then updated the traversal to treat those loop bodies as repeated. The full Python 3.13/3.14 test runs and all pre-commit checks pass in 98fec80.


for child in ast.iter_child_nodes(node):
num_usages = self._check_b031_group_usage(
child, group_name, num_usages, repeated
)
return num_usages

def check_for_b031(self, loop_node: ast.For) -> None: # noqa: C901
"""Check that `itertools.groupby` isn't iterated over more than once.

Expand All @@ -1391,30 +1449,7 @@ def check_for_b031(self, loop_node: ast.For) -> None: # noqa: C901
# Ignore any `groupby()` invocation that isn't unpacked
return

num_usages = 0
for node in walk_list(loop_node.body): # type: ignore[assignment]
# Handled nested loops
if isinstance(node, ast.For):
for nested_node in walk_list(node.body):
assert nested_node != node
if (
isinstance(nested_node, ast.Name)
and nested_node.id == group_name
and isinstance(nested_node.ctx, ast.Load)
):
self.add_error("B031", nested_node, nested_node.id)

# Handle multiple uses. Count only loads: a store-context
# reference, such as an annotation target (`group: T`), is
# not a read of the generator (#465).
if (
isinstance(node, ast.Name)
and node.id == group_name
and isinstance(node.ctx, ast.Load)
):
num_usages += 1
if num_usages > 1:
self.add_error("B031", node, node.id)
self._check_b031_group_usages(loop_node.body, group_name)

def _get_names_from_tuple(self, node: ast.Tuple) -> Iterator[str]:
for dim in node.elts:
Expand Down
38 changes: 38 additions & 0 deletions tests/eval_files/b031.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,41 @@ def collect_shop_items(shopper, items):
for _section, section_items in groupby(items, key=lambda p: p[1]):
section_items: list
collect_shop_items("Jane", section_items)


# Mutually exclusive branches cannot consume the group more than once (#465)
for _section, section_items in groupby(items, key=lambda p: p[1]):
if _section == "greens":
collect_shop_items("Jane", section_items)
else:
collect_shop_items("Joe", section_items)

# Each arm of an if/elif/else chain is also mutually exclusive
for _section, section_items in groupby(items, key=lambda p: p[1]):
if _section == "greens":
collect_shop_items("Jane", section_items)
elif _section == "meats & fish":
collect_shop_items("Joe", section_items)
else:
collect_shop_items("Sarah", section_items)

# Repeated uses on the same path must still warn
for _section, section_items in groupby(items, key=lambda p: p[1]):
if _section == "greens":
collect_shop_items("Jane", section_items)
collect_shop_items("Joe", section_items) # B031: 34, "section_items"
else:
collect_shop_items("Sarah", section_items)

# A use after a conditional can follow a use inside either branch
for _section, section_items in groupby(items, key=lambda p: p[1]):
if _section == "greens":
collect_shop_items("Jane", section_items)
collect_shop_items("Joe", section_items) # B031: 30, "section_items"

# A use in the condition happens before either branch
for _section, section_items in groupby(items, key=lambda p: p[1]):
if list(section_items):
collect_shop_items("Jane", section_items) # B031: 35, "section_items"
else:
collect_shop_items("Joe", section_items) # B031: 34, "section_items"