Short-circuit comparison chains in rewritten asserts

[SirHegel]

Closes #14819.

The problem

Python evaluates a comparison chain lazily: in a < b < c, c is never evaluated when a < b is false. visit_Compare walked the comparators in a loop, assigning every result, and only combined them with an and afterwards — by which point everything had already run.

def test_raises_the_wrong_error():
    assert 1 < 0 < 1 / 0        # rewritten: ZeroDivisionError, not AssertionError

def test_calls_what_it_should_not():
    calls = []
    def boom():
        calls.append("boom")
        return 5
    try:
        assert 1 < 0 < boom()
    except AssertionError:
        pass
    assert calls == []          # rewritten: calls == ["boom"]

Both pass under --assert=plain and fail when rewritten.

The change

Each comparison after the first is nested inside an if on the previous result — what visit_BoolOp has done for and/or since #57. For assert 1 < 0 < boom():

 @py_assert0 = 1
 @py_assert4 = 0
+@py_assert3 = @py_assert7 = None
 @py_assert2 = @py_assert0 < @py_assert4
-@py_assert7 = boom()
-@py_assert3 = @py_assert4 < @py_assert7
+if @py_assert2:
+    @py_assert7 = boom()
+    @py_assert3 = @py_assert4 < @py_assert7
 if not (@py_assert2 and @py_assert3):

The and in the guard already short-circuits, so res is safe. The explanation is not: it builds its arguments eagerly, and a skipped operand would leave an unbound name for _saferepr to read. Everything a skipped operand would have bound is set to None before the chain.

Nothing ever reads those Nones. _call_reprcompare breaks at the first falsy result and reports only that pair — which is exactly where the chain stopped, since a chain only short-circuits after a false comparison. That is also why the messages do not change: assert 1 < 3 < 5 <= 4 < 7 still reports assert 5 <= 4.

Notes for review

• The None assignment is inserted after comp.left's own statements, so a compound left operand is still evaluated exactly once and in place. With assert len(v) < g() < h() the generated code keeps @py_assert2 = len(v) first and only calls h() inside the guard.
• format_variables only exists when the pytest_assertion_pass hook is enabled, so it is read through getattr.

Tests

test_comparison_chain_short_circuits covers the wrong-exception case, the unwanted-call case, and that a chain which fails partway still reports the failing pair. test_comparisons and test_custom_reprcompare pin the existing messages and are unchanged.

Full suite: 4359 passed, 97 skipped, 14 xfailed.

Checklist

• Include documentation when adding new features.
• Include new tests or update existing tests when applicable.
• If AI agents were used, they are credited in Co-authored-by commit trailers.
• Create a new changelog file in the changelog directory.
• Add yourself to AUTHORS in alphabetical order.

Claude (Opus 5) was used; it is credited in the Co-authored-by trailers. I read the generated code with ast.unparse before and after, and I can answer questions on any part of it.

[SirHegel]

I moved this to draft for a few hours because I found a case my own change broke, and I would rather flag it than have a reviewer find it.

Nesting only the main statement body was not enough. An operand can also contribute statements that run only when the assertion fails — a nested boolop builds its explanation by appending to a list that is created in the main body. With the list creation skipped and the appends left running unconditionally:

def test_boolop_in_a_chain():
    a = b = 0
    assert 1 < 0 < (a or b)

E       AttributeError: 'NoneType' object has no attribute 'append'

That is worse than the bug being fixed. The full suite did not catch it — there was no test for a boolop inside a comparison chain — so I found it by reading the generated code with ast.unparse for a case I had not tried.

Fixed by nesting expl_stmts on the same condition, the way visit_BoolOp nests both. Two details fell out of that:

• Most operands contribute nothing to the explanation, and an ast.If with an empty body is invalid, so the empty ones are dropped afterwards — innermost first, since pruning a child can empty its parent.
• The names to pre-bind are now collected by walking the conditional blocks for Store targets, instead of tracking self.variables and format_variables by index. That picks up @py_format names, which pop_format_context only records when the assertion_pass hook is on, and it removes the getattr(self, "format_variables", None) branch that no test could reach — which is also what codecov/patch was complaining about.

assert 1 < 0 < (a or b) is now part of the regression test. Full suite: 4359 passed, 97 skipped, 14 xfailed. Back to ready for review.

[RonnyPfannschmidt]

please crosscheck if this is already part of my stack of rewrite bugfixes/enhancements

[SirHegel]

Sorry, I just checked and duplicated the result; I had some leftover GPT tokens and wanted to help out with computing power—I'm auditing and helping more effectively now. My apologies; you're doing great work, brother.

[SirHegel]

Sorry, I just checked and duplicated the result; I had some leftover GPT tokens and wanted to help out with computing power—I'm auditing and helping more effectively now. My apologies; you're doing great work, brother.

[SirHegel]

Cross-checked as you asked: #14822 already does this, from three weeks ago, and reaches the same shape. Closing in favour of your stack.

One thing that came out of the comparison — I left the detail on #14822 — is that a boolop in a skipped comparator raises AttributeError on that branch:

assert 1 < 0 < (a or b)   # AttributeError: 'NoneType' object has no attribute 'append'

origin/main gives assert 1 < 0 there, so it is a regression rather than something pre-existing. The cause and the fix are in that comment.

Sorry for the duplicate. I filtered issues on "no assignee, no comments" and #14819 is both, being your own — I should have looked for a PR referencing it before starting.