Skip to content

Commit 9cd97d4

Browse files
iritkatrieliritkatriel
committed
pythongh-124285: Fix bug where bool() is called multiple times for the same part of a boolean expression
1 parent aba42c0 commit 9cd97d4

9 files changed

Lines changed: 169 additions & 28 deletions

File tree

‎Doc/library/dis.rst‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1872,6 +1872,12 @@ but are replaced by real opcodes or removed before bytecode is generated.
18721872
Undirected relative jump instructions which are replaced by their
18731873
directed (forward/backward) counterparts by the assembler.
18741874

1875+
.. opcode:: JUMP_IF_TRUE
1876+
.. opcode:: JUMP_IF_FALSE
1877+
1878+
Conditional jumps which do not impact the stack. Replaced by the sequence
1879+
``COPY 1``, ``TO_BOOL``, ``POP_JUMP_IF_TRUE/FALSE``.
1880+
18751881
.. opcode:: LOAD_CLOSURE (i)
18761882

18771883
Pushes a reference to the cell contained in slot ``i`` of the "fast locals"

‎Include/internal/pycore_opcode_metadata.h‎

Lines changed: 24 additions & 8 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Include/opcode_ids.h‎

Lines changed: 9 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Lib/_opcode_metadata.py‎

Lines changed: 9 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Lib/test/test_compile.py‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1527,6 +1527,45 @@ async def name_4():
15271527
pass
15281528
[[]]
15291529

1530+
class TestBooleanExpression(unittest.TestCase):
1531+
class Value:
1532+
def __init__(self):
1533+
self.called = 0
1534+
1535+
def __bool__(self):
1536+
self.called += 1
1537+
return self.value
1538+
1539+
class Yes(Value):
1540+
value = True
1541+
1542+
class No(Value):
1543+
value = False
1544+
1545+
def test_short_circuit_and(self):
1546+
v = [self.Yes(), self.No(), self.Yes()]
1547+
res = v[0] and v[1] and v[0]
1548+
self.assertIs(res, v[1])
1549+
self.assertEqual([e.called for e in v], [1, 1, 0])
1550+
1551+
def test_short_circuit_or(self):
1552+
v = [self.No(), self.Yes(), self.No()]
1553+
res = v[0] or v[1] or v[0]
1554+
self.assertIs(res, v[1])
1555+
self.assertEqual([e.called for e in v], [1, 1, 0])
1556+
1557+
def test_compound(self):
1558+
# See gh-124285
1559+
v = [self.No(), self.Yes(), self.Yes(), self.Yes()]
1560+
res = v[0] and v[1] or v[2] or v[3]
1561+
self.assertIs(res, v[2])
1562+
self.assertEqual([e.called for e in v], [1, 0, 1, 0])
1563+
1564+
v = [self.No(), self.No(), self.Yes(), self.Yes(), self.No()]
1565+
res = v[0] or v[1] and v[2] or v[3] or v[4]
1566+
self.assertIs(res, v[3])
1567+
self.assertEqual([e.called for e in v], [1, 1, 0, 1, 0])
1568+
15301569
@requires_debug_ranges()
15311570
class TestSourcePositions(unittest.TestCase):
15321571
# 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‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Fix bug where ``bool(a)`` can be invoked more than once during the
2+
evaluation of a compound boolean expression.

‎Python/bytecodes.c‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2571,6 +2571,16 @@ dummy_func(
25712571
JUMP_BACKWARD_NO_INTERRUPT,
25722572
};
25732573

2574+
pseudo(JUMP_IF_FALSE, (cond -- cond)) = {
2575+
JUMP_FORWARD,
2576+
JUMP_BACKWARD,
2577+
};
2578+
2579+
pseudo(JUMP_IF_TRUE, (cond -- cond)) = {
2580+
JUMP_FORWARD,
2581+
JUMP_BACKWARD,
2582+
};
2583+
25742584
tier1 inst(ENTER_EXECUTOR, (--)) {
25752585
#ifdef _Py_TIER2
25762586
PyCodeObject *code = _PyFrame_GetCode(frame);

‎Python/codegen.c‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3140,17 +3140,15 @@ codegen_boolop(compiler *c, expr_ty e)
31403140
location loc = LOC(e);
31413141
assert(e->kind == BoolOp_kind);
31423142
if (e->v.BoolOp.op == And)
3143-
jumpi = POP_JUMP_IF_FALSE;
3143+
jumpi = JUMP_IF_FALSE;
31443144
else
3145-
jumpi = POP_JUMP_IF_TRUE;
3145+
jumpi = JUMP_IF_TRUE;
31463146
NEW_JUMP_TARGET_LABEL(c, end);
31473147
s = e->v.BoolOp.values;
31483148
n = asdl_seq_LEN(s) - 1;
31493149
assert(n >= 0);
31503150
for (i = 0; i < n; ++i) {
31513151
VISIT(c, expr, (expr_ty)asdl_seq_GET(s, i));
3152-
ADDOP_I(c, loc, COPY, 1);
3153-
ADDOP(c, loc, TO_BOOL);
31543152
ADDOP_JUMP(c, loc, jumpi, end);
31553153
ADDOP(c, loc, POP_TOP);
31563154
}

‎Python/flowgraph.c‎

Lines changed: 68 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1589,6 +1589,8 @@ basicblock_optimize_load_const(PyObject *const_cache, basicblock *bb, PyObject *
15891589
switch(nextop) {
15901590
case POP_JUMP_IF_FALSE:
15911591
case POP_JUMP_IF_TRUE:
1592+
case JUMP_IF_FALSE:
1593+
case JUMP_IF_TRUE:
15921594
{
15931595
/* Remove LOAD_CONST const; conditional jump */
15941596
PyObject* cnt = get_const_value(opcode, oparg, consts);
@@ -1600,8 +1602,11 @@ basicblock_optimize_load_const(PyObject *const_cache, basicblock *bb, PyObject *
16001602
if (is_true == -1) {
16011603
return ERROR;
16021604
}
1603-
INSTR_SET_OP0(inst, NOP);
1604-
int jump_if_true = nextop == POP_JUMP_IF_TRUE;
1605+
if (PyCompile_OpcodeStackEffect(nextop, 0) == -1) {
1606+
/* POP_JUMP_IF_FALSE or POP_JUMP_IF_TRUE */
1607+
INSTR_SET_OP0(inst, NOP);
1608+
}
1609+
int jump_if_true = (nextop == POP_JUMP_IF_TRUE || nextop == JUMP_IF_TRUE);
16051610
if (is_true == jump_if_true) {
16061611
bb->b_instr[i+1].i_opcode = JUMP;
16071612
}
@@ -1761,6 +1766,34 @@ optimize_basic_block(PyObject *const_cache, basicblock *bb, PyObject *consts)
17611766
i -= jump_thread(bb, inst, target, POP_JUMP_IF_TRUE);
17621767
}
17631768
break;
1769+
case JUMP_IF_FALSE:
1770+
switch (target->i_opcode) {
1771+
case JUMP:
1772+
case JUMP_IF_FALSE:
1773+
i -= jump_thread(bb, inst, target, JUMP_IF_FALSE);
1774+
continue;
1775+
case JUMP_IF_TRUE:
1776+
// No need to check for loops here, a block's b_next
1777+
// cannot point to itself.
1778+
assert(inst->i_target != inst->i_target->b_next);
1779+
inst->i_target = inst->i_target->b_next;
1780+
continue;
1781+
}
1782+
break;
1783+
case JUMP_IF_TRUE:
1784+
switch (target->i_opcode) {
1785+
case JUMP:
1786+
case JUMP_IF_TRUE:
1787+
i -= jump_thread(bb, inst, target, JUMP_IF_TRUE);
1788+
continue;
1789+
case JUMP_IF_FALSE:
1790+
// No need to check for loops here, a block's b_next
1791+
// cannot point to itself.
1792+
assert(inst->i_target != inst->i_target->b_next);
1793+
inst->i_target = inst->i_target->b_next;
1794+
continue;
1795+
}
1796+
break;
17641797
case JUMP:
17651798
case JUMP_NO_INTERRUPT:
17661799
switch (target->i_opcode) {
@@ -2367,6 +2400,37 @@ push_cold_blocks_to_end(cfg_builder *g) {
23672400
return SUCCESS;
23682401
}
23692402

2403+
static int
2404+
convert_pseudo_conditional_jumps(cfg_builder *g)
2405+
{
2406+
basicblock *entryblock = g->g_entryblock;
2407+
for (basicblock *b = entryblock; b != NULL; b = b->b_next) {
2408+
for (int i = 0; i < b->b_iused; i++) {
2409+
cfg_instr *instr = &b->b_instr[i];
2410+
if (instr->i_opcode == JUMP_IF_FALSE || instr->i_opcode == JUMP_IF_TRUE) {
2411+
assert(i == b->b_iused - 1);
2412+
instr->i_opcode = instr->i_opcode == JUMP_IF_FALSE ?
2413+
POP_JUMP_IF_FALSE : POP_JUMP_IF_TRUE;
2414+
cfg_instr copy = {
2415+
.i_opcode = COPY,
2416+
.i_oparg = 1,
2417+
.i_loc = instr->i_loc,
2418+
.i_target = NULL,
2419+
};
2420+
RETURN_IF_ERROR(basicblock_insert_instruction(b, i++, &copy));
2421+
cfg_instr to_bool = {
2422+
.i_opcode = TO_BOOL,
2423+
.i_oparg = 0,
2424+
.i_loc = instr->i_loc,
2425+
.i_target = NULL,
2426+
};
2427+
RETURN_IF_ERROR(basicblock_insert_instruction(b, i++, &to_bool));
2428+
}
2429+
}
2430+
}
2431+
return SUCCESS;
2432+
}
2433+
23702434
static int
23712435
convert_pseudo_ops(cfg_builder *g)
23722436
{
@@ -2826,6 +2890,8 @@ _PyCfg_OptimizedCfgToInstructionSequence(cfg_builder *g,
28262890
int *stackdepth, int *nlocalsplus,
28272891
_PyInstructionSequence *seq)
28282892
{
2893+
RETURN_IF_ERROR(convert_pseudo_conditional_jumps(g));
2894+
28292895
*stackdepth = calculate_stackdepth(g);
28302896
if (*stackdepth < 0) {
28312897
return ERROR;

0 commit comments

Comments
 (0)