GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Optimize redundant bool check by moreal · Pull Request #7176 · RustPython/RustPython Conversation
|
📝 WalkthroughWalkthroughIntroduce targeted short-circuit jump threading in boolean-op compilation by adding Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
github-actions
Bot
commented
Feb 16, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin optimize-redundant-bool-check |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
0fe34f5 to
d28d1cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_opto support optional short-circuit target override - Added test case validating that
__bool__is called only once forTest() 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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
youknowone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| with assert_raises(TypeError): | ||
| bool(TestLenThrowError()) | ||
|
|
||
| # Issue #3567: nested BoolOps should not call __bool__ redundantly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is more like short-circuit than bool. we have syntax_short_circuit_bool.py for that
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
…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 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
…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 <[email protected]>
c1e7986 to
e68c018
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick comments (1)
extra_tests/snippets/syntax_short_circuit_bool.py (1)
34-36: Consider asserting the expression result for completeness.The
ExplodingBoolmechanism 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) == FalseThis 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).
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
|
As a note about d6e48ff, |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick comments (1)
extra_tests/snippets/syntax_short_circuit_bool.py (1)
34-36: Good fix — previousasserton a bare falsy expression has been resolved.The
== Falsecomparison correctly makes this assertion pass when the optimization works (single__bool__call), while theExplodingBoolinternal assert still guards against redundant invocations. Nice.One minor nit from flake8 (E712): prefer
is Falseover== 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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
youknowone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
* Add compile_bool_op_inner and optimize nested opposite-operator BoolOps 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 <[email protected]> * Add snapshot test for nested BoolOp bytecode Co-Authored-By: Claude Opus 4.6 <[email protected]> * Add runtime test for redundant __bool__ check (issue RustPython#3567) Co-Authored-By: Claude Opus 4.6 <[email protected]> * Apply clippy and rustfmt * Apply ruff format * Refactor compile_bool_op: extract emit_short_circuit_test and unify with 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 <[email protected]> * Relocate redundant __bool__ check test snippet * Update extra_tests/snippets/syntax_short_circuit_bool.py * Fix assertion in syntax_short_circuit_bool --------- Co-authored-by: Claude Opus 4.6 <[email protected]> Co-authored-by: Jeong, YunWon <[email protected]>
* Add compile_bool_op_inner and optimize nested opposite-operator BoolOps 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 <[email protected]> * Add snapshot test for nested BoolOp bytecode Co-Authored-By: Claude Opus 4.6 <[email protected]> * Add runtime test for redundant __bool__ check (issue #3567) Co-Authored-By: Claude Opus 4.6 <[email protected]> * Apply clippy and rustfmt * Apply ruff format * Refactor compile_bool_op: extract emit_short_circuit_test and unify with 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 <[email protected]> * Relocate redundant __bool__ check test snippet * Update extra_tests/snippets/syntax_short_circuit_bool.py * Fix assertion in syntax_short_circuit_bool --------- Co-authored-by: Claude Opus 4.6 <[email protected]> Co-authored-by: Jeong, YunWon <[email protected]>
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
Labels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Boolean expression performs redundant bool checkUh oh!
There was an error while loading. Please reload this page.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.