pythongh-124285: Fix bug where bool() is called multiple times for th…

…e same part of a boolean expression

Doc/library/dis.rst

@@ -1872,6 +1872,12 @@ but are replaced by real opcodes or removed before bytecode is generated.
    Undirected relative jump instructions which are replaced by their
    directed (forward/backward) counterparts by the assembler.
 
+.. opcode:: JUMP_IF_TRUE
+.. opcode:: JUMP_IF_FALSE
+
+   Conditional jumps which do not impact the stack. Replaced by the sequence
+   ``COPY 1``, ``TO_BOOL``, ``POP_JUMP_IF_TRUE/FALSE``.
+
 .. opcode:: LOAD_CLOSURE (i)
 
    Pushes a reference to the cell contained in slot ``i`` of the "fast locals"

Lib/test/test_compile.py

@@ -1527,6 +1527,45 @@ async def name_4():
                     pass
             [[]]
 
+class TestBooleanExpression(unittest.TestCase):
+    class Value:
+        def __init__(self):
+            self.called = 0
+
+        def __bool__(self):
+            self.called += 1
+            return self.value
+
+    class Yes(Value):
+        value = True
+
+    class No(Value):
+        value = False
+
+    def test_short_circuit_and(self):
+        v = [self.Yes(), self.No(), self.Yes()]
+        res = v[0] and v[1] and v[0]
+        self.assertIs(res, v[1])
+        self.assertEqual([e.called for e in v], [1, 1, 0])
+
+    def test_short_circuit_or(self):
+        v = [self.No(), self.Yes(), self.No()]
+        res = v[0] or v[1] or v[0]
+        self.assertIs(res, v[1])
+        self.assertEqual([e.called for e in v], [1, 1, 0])
+
+    def test_compound(self):
+        # See gh-124285
+        v = [self.No(), self.Yes(), self.Yes(), self.Yes()]
+        res = v[0] and v[1] or v[2] or v[3]
+        self.assertIs(res, v[2])
+        self.assertEqual([e.called for e in v], [1, 0, 1, 0])
+
+        v = [self.No(), self.No(), self.Yes(), self.Yes(), self.No()]
+        res = v[0] or v[1] and v[2] or v[3] or v[4]
+        self.assertIs(res, v[3])
+        self.assertEqual([e.called for e in v], [1, 1, 0, 1, 0])
+
 @requires_debug_ranges()
 class TestSourcePositions(unittest.TestCase):
     # Ensure that compiled code snippets have correct line and column numbers

Misc/NEWS.d/next/Core and Builtins/2024-09-23-23-06-19.gh-issue-124285.mahGTg.rst

@@ -0,0 +1,2 @@
+Fix bug where ``bool(a)`` can be invoked more than once during the
+evaluation of a compound boolean expression.

Python/bytecodes.c

@@ -2571,6 +2571,16 @@ dummy_func(
             JUMP_BACKWARD_NO_INTERRUPT,
         };
 
+        pseudo(JUMP_IF_FALSE, (cond -- cond)) = {
+            JUMP_FORWARD,
+            JUMP_BACKWARD,
+        };
+
+        pseudo(JUMP_IF_TRUE, (cond -- cond)) = {
+            JUMP_FORWARD,
+            JUMP_BACKWARD,
+        };
+
         tier1 inst(ENTER_EXECUTOR, (--)) {
             #ifdef _Py_TIER2
             PyCodeObject *code = _PyFrame_GetCode(frame);

Python/codegen.c

@@ -3140,17 +3140,15 @@ codegen_boolop(compiler *c, expr_ty e)
     location loc = LOC(e);
     assert(e->kind == BoolOp_kind);
     if (e->v.BoolOp.op == And)
-        jumpi = POP_JUMP_IF_FALSE;
+        jumpi = JUMP_IF_FALSE;
     else
-        jumpi = POP_JUMP_IF_TRUE;
+        jumpi = JUMP_IF_TRUE;
     NEW_JUMP_TARGET_LABEL(c, end);
     s = e->v.BoolOp.values;
     n = asdl_seq_LEN(s) - 1;
     assert(n >= 0);
     for (i = 0; i < n; ++i) {
         VISIT(c, expr, (expr_ty)asdl_seq_GET(s, i));
-        ADDOP_I(c, loc, COPY, 1);
-        ADDOP(c, loc, TO_BOOL);
         ADDOP_JUMP(c, loc, jumpi, end);
         ADDOP(c, loc, POP_TOP);
     }

Python/flowgraph.c

@@ -1589,6 +1589,8 @@ basicblock_optimize_load_const(PyObject *const_cache, basicblock *bb, PyObject *
         switch(nextop) {
             case POP_JUMP_IF_FALSE:
             case POP_JUMP_IF_TRUE:
+            case JUMP_IF_FALSE:
+            case JUMP_IF_TRUE:
             {
                 /* Remove LOAD_CONST const; conditional jump */
                 PyObject* cnt = get_const_value(opcode, oparg, consts);
@@ -1600,8 +1602,11 @@ basicblock_optimize_load_const(PyObject *const_cache, basicblock *bb, PyObject *
                 if (is_true == -1) {
                     return ERROR;
                 }
-                INSTR_SET_OP0(inst, NOP);
-                int jump_if_true = nextop == POP_JUMP_IF_TRUE;
+                if (PyCompile_OpcodeStackEffect(nextop, 0) == -1) {
+                    /* POP_JUMP_IF_FALSE or POP_JUMP_IF_TRUE */
+                    INSTR_SET_OP0(inst, NOP);
+                }
+                int jump_if_true = (nextop == POP_JUMP_IF_TRUE || nextop == JUMP_IF_TRUE);
                 if (is_true == jump_if_true) {
                     bb->b_instr[i+1].i_opcode = JUMP;
                 }
@@ -1761,6 +1766,34 @@ optimize_basic_block(PyObject *const_cache, basicblock *bb, PyObject *consts)
                         i -= jump_thread(bb, inst, target, POP_JUMP_IF_TRUE);
                 }
                 break;
+            case JUMP_IF_FALSE:
+                switch (target->i_opcode) {
+                    case JUMP:
+                    case JUMP_IF_FALSE:
+                        i -= jump_thread(bb, inst, target, JUMP_IF_FALSE);
+                        continue;
+                    case JUMP_IF_TRUE:
+                        // No need to check for loops here, a block's b_next
+                        // cannot point to itself.
+                        assert(inst->i_target != inst->i_target->b_next);
+                        inst->i_target = inst->i_target->b_next;
+                        continue;
+                }
+                break;
+            case JUMP_IF_TRUE:
+                switch (target->i_opcode) {
+                    case JUMP:
+                    case JUMP_IF_TRUE:
+                        i -= jump_thread(bb, inst, target, JUMP_IF_TRUE);
+                        continue;
+                    case JUMP_IF_FALSE:
+                        // No need to check for loops here, a block's b_next
+                        // cannot point to itself.
+                        assert(inst->i_target != inst->i_target->b_next);
+                        inst->i_target = inst->i_target->b_next;
+                        continue;
+                }
+                break;
             case JUMP:
             case JUMP_NO_INTERRUPT:
                 switch (target->i_opcode) {
@@ -2367,6 +2400,37 @@ push_cold_blocks_to_end(cfg_builder *g) {
     return SUCCESS;
 }
 
+static int
+convert_pseudo_conditional_jumps(cfg_builder *g)
+{
+    basicblock *entryblock = g->g_entryblock;
+    for (basicblock *b = entryblock; b != NULL; b = b->b_next) {
+        for (int i = 0; i < b->b_iused; i++) {
+            cfg_instr *instr = &b->b_instr[i];
+            if (instr->i_opcode == JUMP_IF_FALSE || instr->i_opcode == JUMP_IF_TRUE) {
+                assert(i == b->b_iused - 1);
+                instr->i_opcode = instr->i_opcode == JUMP_IF_FALSE ?
+                                          POP_JUMP_IF_FALSE : POP_JUMP_IF_TRUE;
+                cfg_instr copy = {
+                            .i_opcode = COPY,
+                            .i_oparg = 1,
+                            .i_loc = instr->i_loc,
+                            .i_target = NULL,
+                };
+                RETURN_IF_ERROR(basicblock_insert_instruction(b, i++, &copy));
+                cfg_instr to_bool = {
+                            .i_opcode = TO_BOOL,
+                            .i_oparg = 0,
+                            .i_loc = instr->i_loc,
+                            .i_target = NULL,
+                };
+                RETURN_IF_ERROR(basicblock_insert_instruction(b, i++, &to_bool));
+            }
+        }
+    }
+    return SUCCESS;
+}
+
 static int
 convert_pseudo_ops(cfg_builder *g)
 {
@@ -2826,6 +2890,8 @@ _PyCfg_OptimizedCfgToInstructionSequence(cfg_builder *g,
                                      int *stackdepth, int *nlocalsplus,
                                      _PyInstructionSequence *seq)
 {
+    RETURN_IF_ERROR(convert_pseudo_conditional_jumps(g));
+
     *stackdepth = calculate_stackdepth(g);
     if (*stackdepth < 0) {
         return ERROR;