Optimize redundant bool check (#7176)

moreal · claude · youknowone · web-flow · commit d07d52224ead · 2026-02-18T00:45:17.000+09:00
* 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 <noreply@anthropic.com> * Add snapshot test for nested BoolOp bytecode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add runtime test for redundant __bool__ check (issue #3567) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
diff --git a/crates/codegen/src/compile.rs b/crates/codegen/src/compile.rs
@@ -6611,33 +6611,45 @@ impl Compiler {
     /// Compile a boolean operation as an expression.
     /// This means, that the last value remains on the stack.
     fn compile_bool_op(&mut self, op: &ast::BoolOp, values: &[ast::Expr]) -> CompileResult<()> {
-        let after_block = self.new_block();
+        self.compile_bool_op_with_target(op, values, None)
+    }
 
+    /// Compile a boolean operation as an expression, with an optional
+    /// short-circuit target override. When `short_circuit_target` is `Some`,
+    /// the short-circuit jumps go to that block instead of the default
+    /// `after_block`, enabling jump threading to avoid redundant `__bool__` calls.
+    fn compile_bool_op_with_target(
+        &mut self,
+        op: &ast::BoolOp,
+        values: &[ast::Expr],
+        short_circuit_target: Option<BlockIdx>,
+    ) -> CompileResult<()> {
+        let after_block = self.new_block();
         let (last_value, values) = values.split_last().unwrap();
+        let jump_target = short_circuit_target.unwrap_or(after_block);
 
         for value in values {
-            self.compile_expression(value)?;
-
-            emit!(self, Instruction::Copy { index: 1_u32 });
-            match op {
-                ast::BoolOp::And => {
-                    emit!(
-                        self,
-                        Instruction::PopJumpIfFalse {
-                            target: after_block,
-                        }
-                    );
-                }
-                ast::BoolOp::Or => {
-                    emit!(
-                        self,
-                        Instruction::PopJumpIfTrue {
-                            target: after_block,
-                        }
-                    );
-                }
+            // Optimization: when a non-last value is a BoolOp with the opposite
+            // operator, redirect its short-circuit exits to skip the outer's
+            // redundant __bool__ test (jump threading).
+            if short_circuit_target.is_none()
+                && let ast::Expr::BoolOp(ast::ExprBoolOp {
+                    op: inner_op,
+                    values: inner_values,
+                    ..
+                }) = value
+                && inner_op != op
+            {
+                let pop_block = self.new_block();
+                self.compile_bool_op_with_target(inner_op, inner_values, Some(pop_block))?;
+                self.emit_short_circuit_test(op, after_block);
+                self.switch_to_block(pop_block);
+                emit!(self, Instruction::PopTop);
+                continue;
             }
 
+            self.compile_expression(value)?;
+            self.emit_short_circuit_test(op, jump_target);
             emit!(self, Instruction::PopTop);
         }
 
@@ -6647,6 +6659,20 @@ impl Compiler {
         Ok(())
     }
 
+    /// Emit `Copy 1` + conditional jump for short-circuit evaluation.
+    /// For `And`, emits `PopJumpIfFalse`; for `Or`, emits `PopJumpIfTrue`.
+    fn emit_short_circuit_test(&mut self, op: &ast::BoolOp, target: BlockIdx) {
+        emit!(self, Instruction::Copy { index: 1_u32 });
+        match op {
+            ast::BoolOp::And => {
+                emit!(self, Instruction::PopJumpIfFalse { target });
+            }
+            ast::BoolOp::Or => {
+                emit!(self, Instruction::PopJumpIfTrue { target });
+            }
+        }
+    }
+
     fn compile_dict(&mut self, items: &[ast::DictItem]) -> CompileResult<()> {
         let has_unpacking = items.iter().any(|item| item.key.is_none());
 
@@ -9006,6 +9032,15 @@ if (True and False) or (False and True):
         ));
     }
 
+    #[test]
+    fn test_nested_bool_op() {
+        assert_dis_snapshot!(compile_exec(
+            "\
+x = Test() and False or False
+"
+        ));
+    }
+
     #[test]
     fn test_nested_double_async_with() {
         assert_dis_snapshot!(compile_exec(
diff --git a/crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_bool_op.snap b/crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_bool_op.snap
diff --git a/extra_tests/snippets/syntax_short_circuit_bool.py b/extra_tests/snippets/syntax_short_circuit_bool.py
@@ -31,3 +31,6 @@ def __bool__(self):
 
 # if ExplodingBool(False) and False and True and False:
 #     pass
+
+# Issue #3567: nested BoolOps should not call __bool__ redundantly
+assert (ExplodingBool(False) and False or False) == False
