diff --git a/README.rst b/README.rst index 5f62d4f..373f67c 100644 --- a/README.rst +++ b/README.rst @@ -306,6 +306,15 @@ If you define `__str__/__reduce__` in super classes this check is unable to dete **B043**: Do not call ``delattr(x, 'attr')``, instead use ``del x.attr``. There is no additional safety in using ``delattr`` if you know the attribute name ahead of time. +.. _B044: + +**B044**: Do not use the result of ``str.find()`` or ``str.rfind()`` directly as a boolean. +They return ``-1`` when the substring is missing, which is truthy, and ``0`` when it is found at the +start of the string, which is falsy, so ``if s.find(x):`` reads backwards from what it does. Compare +the returned index explicitly instead, e.g. ``if s.find(x) != -1:`` or ``if s.find(x) == 0:``. +This is a name-based check, so it can fire on unrelated objects that also define a ``find`` method +(such as BeautifulSoup); add a ``# noqa: B044`` there if needed. + Opinionated warnings ~~~~~~~~~~~~~~~~~~~~ @@ -508,6 +517,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) +* B044: Add new check for using the result of `str.find()`/`str.rfind()` directly as a boolean (#170) 25.11.29 ~~~~~~~~ diff --git a/bugbear.py b/bugbear.py index 33880ed..3b422d3 100644 --- a/bugbear.py +++ b/bugbear.py @@ -604,6 +604,7 @@ def visit_Call(self, node: ast.Call) -> None: self.check_for_b910(node) self.check_for_b911(node) self.check_for_b912(node) + self.check_for_b044(node) # no need for copying, if used in nested calls it will be set to None current_b040_caught_exception = self.b040_caught_exception @@ -1416,6 +1417,39 @@ def check_for_b031(self, loop_node: ast.For) -> None: # noqa: C901 if num_usages > 1: self.add_error("B031", node, node.id) + def check_for_b044(self, node: ast.Call) -> None: + # `str.find()`/`rfind()` return -1 when the substring is missing, which + # is truthy, and 0 when it is found at the start, which is falsy. Testing + # the result directly therefore inverts the intended logic, so require an + # explicit comparison against the returned index instead. + if not ( + isinstance(node.func, ast.Attribute) + and node.func.attr in ("find", "rfind") + and node.args + ): + return + if self._is_used_as_boolean(node): + self.add_error("B044", node) + + def _is_used_as_boolean(self, node: ast.expr) -> bool: + # node is the node currently being visited, so it sits on top of the + # stack. Walk the ancestors, stepping through `not`/`and`/`or` wrappers + # that keep testing the value's truthiness, and report if we reach a + # place that uses it as a condition. + child: ast.AST = node + for parent in reversed(self.node_stack[:-1]): + if isinstance(parent, ast.BoolOp): + child = parent + elif isinstance(parent, ast.UnaryOp) and isinstance(parent.op, ast.Not): + child = parent + elif isinstance(parent, (ast.If, ast.IfExp, ast.While, ast.Assert)): + return parent.test is child + elif isinstance(parent, ast.comprehension): + return child in parent.ifs + else: + return False + return False + def _get_names_from_tuple(self, node: ast.Tuple) -> Iterator[str]: for dim in node.elts: if isinstance(dim, ast.Name): @@ -2797,6 +2831,14 @@ def __call__(self, lineno: int, col: int, vars: tuple[object, ...] = ()) -> erro "it is not any safer than normal property access." ) ), + "B044": Error( + message=( + "B044 Using the result of `.find()`/`.rfind()` as a boolean is " + "misleading: it returns -1 (truthy) when the substring is missing and " + "0 (falsy) when it is found at the start. Compare the returned index " + "explicitly instead." + ) + ), # Warnings disabled by default. "B901": Error( message=( diff --git a/tests/eval_files/b044.py b/tests/eval_files/b044.py new file mode 100644 index 0000000..3998643 --- /dev/null +++ b/tests/eval_files/b044.py @@ -0,0 +1,62 @@ +haystack = "hello world" +needle = "world" + + +# Bad: the index is used directly as a boolean. +if haystack.find(needle): # B044: 3 + pass + +if not haystack.find(needle): # B044: 7 + pass + +while haystack.find(needle): # B044: 6 + pass + +assert haystack.find(needle) # B044: 7 + +found = "yes" if haystack.find(needle) else "no" # B044: 17 + +if haystack.find(needle) and needle: # B044: 3 + pass + +if needle or haystack.find(needle): # B044: 13 + pass + +if not (haystack.find(needle) or needle): # B044: 8 + pass + +matches = [c for c in haystack if haystack.find(c)] # B044: 34 + +if haystack.rfind(needle): # B044: 3 + pass + +if b"data".find(b"a"): # B044: 3 + pass + + +# OK: the returned index is compared explicitly. +if haystack.find(needle) == 0: + pass + +if haystack.find(needle) != -1: + pass + +if haystack.find(needle) >= 0: + pass + +index = haystack.find(needle) +if index: + pass + + +def get_index(): + return haystack.find(needle) + + +haystack.find(needle) +print(haystack.find(needle)) + +# OK: `.index()` raises instead of returning -1, and unrelated `.find()` users +# such as BeautifulSoup are not our concern here, but a plain attribute is. +if haystack.index(needle): + pass