Repository navigation
Behavior change for foo and 1 or 2: 3.12 newly converts foo to bool twice #124285
Description
Bug report
Bug description:
We noticed a behavior change between 3.11 and 3.12. The following code calls Foo.__bool__ once in 3.11 and twice in 3.12. Consequently for this contrived example, the expression evaluates to different results in 3.11 and 3.12.
class Foo:
def __init__(self):
self._a = True
def __bool__(self):
self._a = not self._a
print(f"Foo.__bool__ -> {self._a}")
return self._a
Foo() and "a string" or 42In Python 3.11:
>>> Foo() and "a string" or 42
Foo.__bool__ -> False
42
In Python 3.12 (and 3.13.0b2):
>>> Foo() and "a string" or 42
Foo.__bool__ -> False
Foo.__bool__ -> True
<__main__.Foo object at 0x7f0ae554c1a0>
Is this change intentional?
Note that I'm not necessarily asking to change this. We should arguably change the code to "a string" if Foo() else 42, which evaluates the same in 3.11 and 3.12.
CPython versions tested on:
3.12
Operating systems tested on:
Linux
Activity
ncoghlan commentedon Sep 20, 2024
The rationale for dropping those specialised jump opcodes was here: faster-cpython/ideas#567 (the CPython issue just links there for the details).
From a language specification point of view, I don't think anything has actually changed: while an implementation is permitted to assume that a non-pathological
__bool__won't change its result if the value hasn't been mutated, and that evaluatinga and bwon't mutate eitheraorb, and hence the optimisation to only callbool(a)andbool(b)once ina and b or cis valid, it isn't required to make those assumptions, and hence callingboolagain on the result ofa and bina and b or cis also valid.So the most I would suggest changing for 3.12 and 3.13 is adding a note to the language reference that whether
a.__bool__andb.__bool__are called once or twice ina and b or cis going to be implementation dependent.Presumably CPython's own behaviour may change again in the future if a nicer way to implement that previous optimisation is identified (the old behaviour is akin to compiling
a and b or cas(b or c) if a else crather than as(a and b) or c, sinceandandoronly check the truth value of the left hand operand).Edit: Checking the history of the peephole optimizer, I suspect if you went back to Python 2.3 (before ef0a82b landed), you'd also see the behaviour where the runtime evaluates
bool(a and b)when handlinga and b or c.Edit 2: I worked out a nicer way of expressing the old behaviour that doesn't rely on the
boolbuiltin (it may even be clean enough to make this worth considering as a potential AST level optimization).markshannon commentedon Sep 20, 2024
Thanks @ncoghlan for the analysis.
I think we should classify this as a bug. There is no reason for
bool(a and b)to be re-evaluated, since we know it frombool(a)andbool(b).@iritkatriel could we fix this by using pseudo instructions similar to the
JUMP_IF_[FALSE|TRUE]_OR_POPused in 3.11, and converting them after jump-threading?I note that in ef0a82b the instructions back then were
JUMP_IF_[FALSE|TRUE]not theJUMP_IF_[FALSE|TRUE]_OR_POPof 3.11.We could use the simpler instructions in codegen, as they are straightforward to convert later:
JUMP_IF_FALSE->COPY 1; TO_BOOL; POP_JUMP_IF_FALSE.iritkatriel commentedon Sep 22, 2024
The pseudo-instruction mechanism currently doesn't support mapping a pseudo-instruction to a sequence of instructions. We could teach the generator to do this, but then we will probably have the problem that this pseudo-instruction has irregular stack effect, which is what we were trying to get away from in the first place.
So I think probably not with
JUMP_IF_[FALSE|TRUE]_OR_POP, but maybe withJUMP_IF_[FALSE|TRUE]. Let me try to make that work.pythongh-124285: Fix bug where bool() is called multiple times for th…
gh-124285: Fix bug where bool() is called multiple times for the same…
iritkatriel commentedon Sep 25, 2024
I merged a fix, but I don't know about back porting it.
ncoghlan commentedon Sep 26, 2024
@JelleZijlstra asked on the 3.14 PR if the idea of doing this in the AST optimiser was worth considering further.
Duplicating the
cnode in the generated AST really isn't desirable if we can get the same performance benefit later in the pipeline without introducing any duplication in the generated code.I mainly suggested the AST option in case it was hard to restore the optimisation at the opcode generation stage, which doesn't appear to be a relevant concern (since @iritkatriel already restored it).
JelleZijlstra commentedon Sep 26, 2024
@iritkatriel I think we shouldn't backport this, both because the fix is invasive (new opcodes), and because the behavior change feels too big for a bugfix release.
ncoghlan commentedon Sep 27, 2024
This skip coincidentally came up in a recent forum thread.
The point was made that if compilers are allowed to make this assumption (and they are, since the reference interpreter did it for years, and only stopped due to a bug), then the
__bool__data model entry should mention that.Edit: given the bug fix is too invasive to reasonably backport, I think that means the remaining work here is choosing suitable docs wording for 3.12/3.13/3.14, deciding where it should live in the language reference, and then mentioning it in the docs for
__bool__(and maybe__len__?)pythongh-124285: document assumptions on __bool__/__len__ behaviour
skirpichev commentedon Sep 28, 2024
Here is an attempt to document things: #124723
PS: @Eclips4, probably we should clear 3.12-3.14 labels, as the fix is not going to be backported?