Optimize redundant bool check by moreal · Pull Request #7176 · RustPython/RustPython

moreal · 2026-02-16T18:37:18Z

Summary by CodeRabbit

Bug Fixes
- Improved short-circuit evaluation for nested boolean expressions so inner truth checks are skipped when an outer expression short-circuits, preventing redundant evaluations and ensuring correct control flow across mixed boolean operators.
Tests
- Added tests covering nested boolean short-circuit scenarios, including cases that verify inner bool checks are not invoked when outer short-circuiting applies.

coderabbitai · 2026-02-16T18:37:28Z

📝 Walkthrough

Walkthrough

Introduce targeted short-circuit jump threading in boolean-op compilation by adding compile_bool_op_with_target and emit_short_circuit_test, enabling nested BoolOps to share short-circuit targets and reducing redundant __bool__ evaluations; add a test for nested short-circuit behavior. (≤50 words)

Changes

Cohort / File(s)	Summary
Boolean operation compilation `crates/codegen/src/compile.rs`	Added `compile_bool_op_with_target(op, values, short_circuit_target)` and `emit_short_circuit_test(op, target)`; `compile_bool_op` now delegates to the new helper. Implements jump-threading for nested `BoolOp`s, emits intermediate pop/jump tests, and documents the short-circuit threading rationale.
Test snippet `extra_tests/snippets/syntax_short_circuit_bool.py`	Added assertion and comment referencing Issue `#3567` to validate nested short-circuit boolean expression (`ExplodingBool(False) and False or False`) does not invoke redundant `__bool__` checks.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐇 I hop through branches, keen and spry,

Threading jumps so checks run nigh,
One peep, one blink — no needless call,
My carrot logic trims them all! 🥕

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Optimize redundant bool check' clearly summarizes the main change: optimizing redundant bool invocations in nested boolean expressions through compiler improvements.
Linked Issues check	✅ Passed	The PR addresses issue `#3567` by modifying the compiler to optimize nested BoolOp patterns with opposite operators, implementing the short-circuit target override mechanism to eliminate redundant bool calls as required.
Out of Scope Changes check	✅ Passed	All changes are directly related to the PR objectives: compiler modifications in compile.rs for the optimization and test additions in syntax_short_circuit_bool.py to validate the fix; no unrelated changes detected.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2026-02-16T18:37:50Z

Code has been automatically formatted

The code in this PR has been formatted using:

cargo fmt --all
Please pull the latest changes before pushing again:

git pull origin optimize-redundant-bool-check

Copilot

Pull request overview

This pull request implements an optimization for nested boolean operations to avoid redundant __bool__ calls, matching CPython's behavior. The optimization uses jump threading to allow inner boolean operations with opposite operators to short-circuit directly to a cleanup block, bypassing the outer operation's boolean conversion test.

Changes:

Added jump threading optimization for nested BoolOps with opposite operators (e.g., (x and y) or z)
Refactored compile_bool_op to support optional short-circuit target override
Added test case validating that __bool__ is called only once for Test() and False or False

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
`extra_tests/snippets/builtin_bool.py`	Added test case to verify `__bool__` is called only once in nested BoolOps
`crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_bool_op.snap`	Snapshot of optimized bytecode for the new test case
`crates/codegen/src/compile.rs`	Implemented jump threading optimization and refactored BoolOp compilation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

youknowone

👍

youknowone · 2026-02-17T00:59:09Z

extra_tests/snippets/builtin_bool.py

 with assert_raises(TypeError):
    bool(TestLenThrowError())
+
+# Issue #3567: nested BoolOps should not call __bool__ redundantly


this is more like short-circuit than bool. we have syntax_short_circuit_bool.py for that

I didn't know existence of the file. I moved and simplified the testcase in c1e7986 commit.

# Issue #3567: nested BoolOps should not call __bool__ redundantly ExplodingBool(False) and False or False

…ps to avoid redundant __bool__ calls When a nested BoolOp has the opposite operator (e.g., `And` inside `Or`), the inner BoolOp's short-circuit exits are redirected to skip the outer BoolOp's redundant truth test. This avoids calling `__bool__()` twice on the same value (e.g., `Test() and False or False` previously called `Test().__bool__()` twice instead of once). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…ith compile_bool_op_inner Reduce code duplication by: - Extracting the repeated Copy + conditional jump pattern into emit_short_circuit_test - Merging compile_bool_op and compile_bool_op_inner into a single compile_bool_op_with_target with an optional short_circuit_target parameter - Keeping compile_bool_op as a thin wrapper for the public interface Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

coderabbitai

🧹 Nitpick comments (1)

extra_tests/snippets/syntax_short_circuit_bool.py (1)
34-36: Consider asserting the expression result for completeness.

The ExplodingBool mechanism correctly validates that __bool__ is invoked only once. However, asserting the expected result value would also guard against changes in evaluation semantics:
-ExplodingBool(False) and False or False
+assert (ExplodingBool(False) and False or False) == False
This matches the style of line 27 (assert ExplodingBool(True) or False) and makes the test self-documenting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@extra_tests/snippets/syntax_short_circuit_bool.py` around lines 34 - 36, Add
an assertion that verifies the boolean expression's result (like the existing
`assert ExplodingBool(True) or False` at line 27) so the test both checks
`ExplodingBool.__bool__` is called only once and documents expected semantics;
specifically, assert that the expression `ExplodingBool(False) and False or
False` evaluates to False (use the same `ExplodingBool` symbol and the same
assert style as the earlier test).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@extra_tests/snippets/syntax_short_circuit_bool.py`:
- Around line 34-36: Add an assertion that verifies the boolean expression's
result (like the existing `assert ExplodingBool(True) or False` at line 27) so
the test both checks `ExplodingBool.__bool__` is called only once and documents
expected semantics; specifically, assert that the expression
`ExplodingBool(False) and False or False` evaluates to False (use the same
`ExplodingBool` symbol and the same assert style as the earlier test).

extra_tests/snippets/syntax_short_circuit_bool.py

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@extra_tests/snippets/syntax_short_circuit_bool.py`:
- Around line 34-36: The assert currently forces an AssertionError because the
expression "ExplodingBool(False) and False or False" is falsy; change the test
to not assert on the falsy expression—either remove the leading "assert" so the
expression is merely evaluated (matching the pattern used elsewhere) or flip it
to a truthy case (e.g., use ExplodingBool(True) and True or True) so the test
can exercise redundant __bool__ avoidance; locate the expression using the
ExplodingBool symbol and update accordingly.

extra_tests/snippets/syntax_short_circuit_bool.py

moreal · 2026-02-17T12:46:12Z

As a note about d6e48ff, assert not (ExplodingBool(False) and False or False) doesn't raise AssertionError in the current main branch. So I used == comparison which raises AssertionError. assert (ExplodingBool(False) and False or False) == False

coderabbitai

🧹 Nitpick comments (1)

extra_tests/snippets/syntax_short_circuit_bool.py (1)
34-36: Good fix — previous assert on a bare falsy expression has been resolved.

The == False comparison correctly makes this assertion pass when the optimization works (single __bool__ call), while the ExplodingBool internal assert still guards against redundant invocations. Nice.

One minor nit from flake8 (E712): prefer is False over == False.
Suggested tweak
-assert (ExplodingBool(False) and False or False) == False
+assert (ExplodingBool(False) and False or False) is False
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@extra_tests/snippets/syntax_short_circuit_bool.py` around lines 34 - 36,
Replace the equality comparison with an identity check to satisfy flake8 E712:
in the test assertion using ExplodingBool (the expression "(ExplodingBool(False)
and False or False) == False") change the comparator to "is False" so the
assertion becomes "(ExplodingBool(False) and False or False) is False",
preserving the single-__bool__ behavior while addressing the lint warning.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@extra_tests/snippets/syntax_short_circuit_bool.py`:
- Around line 34-36: Replace the equality comparison with an identity check to
satisfy flake8 E712: in the test assertion using ExplodingBool (the expression
"(ExplodingBool(False) and False or False) == False") change the comparator to
"is False" so the assertion becomes "(ExplodingBool(False) and False or False)
is False", preserving the single-__bool__ behavior while addressing the lint
warning.

youknowone

👍

moreal self-assigned this Feb 16, 2026

moreal force-pushed the optimize-redundant-bool-check branch 5 times, most recently from 0fe34f5 to d28d1cb Compare February 16, 2026 20:02

moreal requested a review from Copilot February 16, 2026 21:04

Copilot started reviewing on behalf of moreal February 16, 2026 21:04 View session

Copilot AI reviewed Feb 16, 2026

View reviewed changes

youknowone requested changes Feb 17, 2026

View reviewed changes

moreal marked this pull request as ready for review February 17, 2026 06:36

moreal requested a review from youknowone February 17, 2026 06:37

moreal and others added 7 commits February 17, 2026 21:23

Add snapshot test for nested BoolOp bytecode

9a2d780

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Add runtime test for redundant __bool__ check (issue RustPython#3567)

d8ccc98

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Apply clippy and rustfmt

66cab2c

Apply ruff format

a500d54

Relocate redundant __bool__ check test snippet

e68c018

moreal force-pushed the optimize-redundant-bool-check branch from c1e7986 to e68c018 Compare February 17, 2026 12:23

coderabbitai bot reviewed Feb 17, 2026

View reviewed changes

youknowone reviewed Feb 17, 2026

View reviewed changes

extra_tests/snippets/syntax_short_circuit_bool.py Outdated Show resolved Hide resolved

Update extra_tests/snippets/syntax_short_circuit_bool.py

dbf95a3

coderabbitai bot reviewed Feb 17, 2026

View reviewed changes

extra_tests/snippets/syntax_short_circuit_bool.py Outdated Show resolved Hide resolved

Fix assertion in syntax_short_circuit_bool

d6e48ff

coderabbitai bot reviewed Feb 17, 2026

View reviewed changes

moreal requested a review from youknowone February 17, 2026 13:26

youknowone approved these changes Feb 17, 2026

View reviewed changes

youknowone merged commit d07d522 into RustPython:main Feb 17, 2026
13 checks passed

Conversation

moreal commented Feb 16, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

github-actions bot commented Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

youknowone Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

moreal Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

moreal commented Feb 17, 2026

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

moreal commented Feb 16, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Feb 16, 2026 •

edited

Loading

github-actions bot commented Feb 16, 2026 •

edited

Loading