B044: add check for using str.find()/rfind() result as a boolean (#170)

Author: ChrisJr404

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
 ~~~~~~~~

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=(

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