diff --git a/README.rst b/README.rst index 51cca44..8c4a76e 100644 --- a/README.rst +++ b/README.rst @@ -501,6 +501,8 @@ UNRELEASED ~~~~~~~~~~ * B019: also flag `async_lru.alru_cache` and check cache decorators on `async def` methods (#488) +* 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) * B902: don't raise a false positive on a metaclass defined with a dotted base such as `abc.ABCMeta` or `enum.EnumMeta` (#411) diff --git a/bugbear.py b/bugbear.py index 1b49311..70b395f 100644 --- a/bugbear.py +++ b/bugbear.py @@ -975,18 +975,28 @@ def check_for_b019(self, node: ast.FunctionDef | ast.AsyncFunctionDef) -> None: return def check_for_b020(self, node: ast.For) -> None: - targets = NameFinder() - targets.visit(node.target) - ctrl_names = set(targets.names) - - iterset = B020NameFinder() + iterset = B020AttributeFinder() iterset.visit(node.iter) iterset_names = set(iterset.names) - for name in sorted(ctrl_names): + # `for self.a in self.b` rebinds an attribute, not the name `self`, so + # comparing the bare base name reports every loop over a sibling + # attribute of the same object. Compare the whole dotted path instead. + candidates: dict[str, ast.expr] = dict(_dotted_targets(node.target)) + if candidates: + iterset_names |= iterset.paths + + # a name that only ever appears in load context is the *base* of an + # attribute or subscript target, not something the loop rebinds + targets = NameFinder() + targets.visit(node.target) + for name, names in targets.names.items(): + if any(isinstance(n.ctx, ast.Store) for n in names): + candidates[name] = names[0] + + for name in sorted(candidates): if name in iterset_names: - n = targets.names[name][0] - self.add_error("B020", n, name) + self.add_error("B020", candidates[name], name) def check_for_b023( # noqa: C901 self, @@ -2082,6 +2092,31 @@ def visit(self, node: ast.AST | list[ast.stmt]) -> ast.AST | list[ast.stmt] | No return node +def _dotted_name(node: ast.AST) -> str | None: + """Return `"self.a.b"` for an attribute chain rooted in a name, else None.""" + parts = [] + while isinstance(node, ast.Attribute): + parts.append(node.attr) + node = node.value + if not isinstance(node, ast.Name): + return None + parts.append(node.id) + return ".".join(reversed(parts)) + + +def _dotted_targets(target: ast.AST) -> Iterator[tuple[str, ast.Attribute]]: + """Yield the attribute paths a `for` statement rebinds on each iteration.""" + if isinstance(target, (ast.Tuple, ast.List)): + for element in target.elts: + yield from _dotted_targets(element) + elif isinstance(target, ast.Starred): + yield from _dotted_targets(target.value) + elif isinstance(target, ast.Attribute): + name = _dotted_name(target) + if name is not None: + yield name, target + + @attr.s class NameFinder(ast.NodeVisitor): """Finds a name within a tree of nodes. @@ -2233,6 +2268,31 @@ def visit_Lambda(self, node) -> None: self.names.pop(lambda_arg.arg, None) +@attr.s +class B020AttributeFinder(B020NameFinder): + """Dotted attribute paths, under the scope rules B020NameFinder uses for names. + + Collecting the paths with a plain `ast.walk` would ignore lexical scope: in + `for obj.value in [obj.value for obj in objects]` the two `obj` bindings are + different objects, and the comprehension-local one must not be matched + against the loop target. + """ + + paths: set[str] = attr.ib(factory=set) + + def visit_Attribute(self, node: ast.Attribute) -> None: + path = _dotted_name(node) + if path is not None: + self.paths.add(path) + self.generic_visit(node) + + def visit_Lambda(self, node: ast.Lambda) -> None: + super().visit_Lambda(node) + for lambda_arg in node.args.args: + prefix = f"{lambda_arg.arg}." + self.paths = {path for path in self.paths if not path.startswith(prefix)} + + B005_METHODS = {"lstrip", "rstrip", "strip"} # Note: these are also used by B039 diff --git a/tests/eval_files/b020.py b/tests/eval_files/b020.py index e6b39ea..7e5cac8 100644 --- a/tests/eval_files/b020.py +++ b/tests/eval_files/b020.py @@ -1,6 +1,6 @@ """ Should emit: -B020 - on lines 8, 21, and 36 +B020 - on lines 8, 21, 32, 36, 58, and 75 """ items = [1, 2, 3] @@ -38,3 +38,39 @@ for var in sorted(range(10), key=lambda var: var.real): print(var) + + +# `for self.a.b in self.c` rebinds an attribute, not the name `self`: two +# different attributes of the same object are two different bindings. +# https://github.com/PyCQA/flake8-bugbear/issues/248 +class AttributeTargets: + test_suite = [1, 2, 3] + + def ok_sibling_attributes(self): + for self.model_instance.value in self.test_suite: + print(self.model_instance.value) + + def ok_plain_attribute(self): + for self.value in self.test_suite: + print(self.value) + + def still_an_error(self): + for self.test_suite in self.test_suite: # B020: 12, "self.test_suite" + print(self.test_suite) + +# the `obj` a comprehension binds is not the `obj` the loop rebinds +def ok_comprehension_scope(obj, objects): + for obj.value in [obj.value for obj in objects]: + print(obj.value) + + +# nor is the `obj` a lambda binds +def ok_lambda_scope(obj, objects): + for obj.value in map(lambda obj: obj.value, objects): + print(obj.value) + + +# the same path on both sides is still an error +def still_an_error_at_module_level(obj): + for obj.value in obj.value: # B020: 8, "obj.value" + print(obj.value)