B044: add check for using str.find()/rfind() result as a boolean - #571
B044: add check for using str.find()/rfind() result as a boolean#571ChrisJr404 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new flake8-bugbear check (B044) to flag cases where the return value of .find() / .rfind() is used in a boolean context, which can invert intended logic due to -1 being truthy and 0 being falsy.
Changes:
- Implemented the B044 check in
bugbear.pyand registered an associated error message. - Added a new eval test file to exercise expected B044 detections.
- Documented B044 in the README and added it to the UNRELEASED changelog entries.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
bugbear.py |
Adds B044 detection logic and a new error message entry. |
tests/eval_files/b044.py |
Introduces eval-based test cases for B044. |
README.rst |
Documents B044 behavior and adds an UNRELEASED changelog bullet. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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 |
There was a problem hiding this comment.
This is scope creep - we can do in dedicated PR if you want or if you're happy to do here let me know.
| # 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 |
There was a problem hiding this comment.
Nice find copilot. Let's label this or fix it if we can, but I don't think we can right?
|
Looks good to me, my only worry is the noisyness thinking about this again. So if anyone else looks at this should we B9XX this to be optional (aka no one really turns these on) or release as B044 and if many people complain about having to suppress we can revert it to a 9XX check? |
Closes #170.
str.find()andstr.rfind()return -1 when the substring is missing and 0 when it's found at the start, so using the result directly as a boolean silently inverts the logic. This adds B044 to flag that when the returned value is used in a boolean context (anif/elif/while/asserttest, a conditional expression, anot/and/oroperand, or a comprehension condition) and leaves explicit comparisons likes.find(x) != -1ors.find(x) == 0alone.Since it's name based it can also fire on unrelated objects that define a
findmethod (BeautifulSoup came to mind), so I called out the# noqa: B044escape hatch in the README. I went back and forth on making it opinionated, but it felt narrow enough to be on by default like you mentioned in the issue. Happy to move it to B9xx if you'd rather.Tests cover the cases from the issue plus the ones that should stay quiet, and the README and changelog are updated.