From 3656a922a57a7b29e01fda75a2233b3a9e425e44 Mon Sep 17 00:00:00 2001 From: SirHegel Date: Thu, 20 Aug 2026 12:16:01 -0500 Subject: [PATCH 1/3] Short-circuit comparison chains in rewritten asserts 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 then everything had already run, so a rewritten assert could call what Python would not, and could fail with an unrelated exception instead of AssertionError. Nest each comparison after the first inside an if on the previous result, which is what visit_BoolOp already does for and/or since #57. The explanation builds its arguments eagerly, so a skipped operand would leave an unbound name behind. Everything a skipped operand would have bound is set to None before the chain. Nothing reads those values: _call_reprcompare stops at the first false result, and a chain only short-circuits after one. Closes #14819. Co-authored-by: Claude --- AUTHORS | 1 + changelog/14819.bugfix.rst | 6 ++++++ src/_pytest/assertion/rewrite.py | 32 ++++++++++++++++++++++++++++++++ testing/test_assertrewrite.py | 29 +++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+) create mode 100644 changelog/14819.bugfix.rst diff --git a/AUTHORS b/AUTHORS index 2805077f42c..2845340e541 100644 --- a/AUTHORS +++ b/AUTHORS @@ -236,6 +236,7 @@ Jeff Rackauckas Jeff Widman Jenni Rinker Jens Tröger +Jhon Alvarez Jiajun Xu John Eddie Ayson John Litborn diff --git a/changelog/14819.bugfix.rst b/changelog/14819.bugfix.rst new file mode 100644 index 00000000000..5ba153a4830 --- /dev/null +++ b/changelog/14819.bugfix.rst @@ -0,0 +1,6 @@ +Comparison chains in an ``assert`` are now evaluated lazily, as Python evaluates them. + +Previously the rewriter evaluated every comparator, so ``assert a < b < c`` could +evaluate ``c`` even when ``a < b`` was false. A rewritten assertion could therefore +call what Python would not, or fail with an unrelated exception instead of +``AssertionError``. ``and`` and ``or`` already short-circuited; chains now do too. diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index 362c93d7253..bbcce08b03f 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -1108,7 +1108,25 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: expls: list[ast.expr] = [] syms: list[ast.expr] = [] results = [left_res] + # Python evaluates a comparison chain lazily: in "a < b < c", "c" is + # never evaluated when "a < b" is false. Nest every comparison after the + # first inside an "if" on the previous result so the rewritten form does + # the same, the way visit_BoolOp already does for "and" and "or". + outer_statements = self.statements + # comp.left is already emitted; the chain's own work starts here. + chain_starts_at = len(self.statements) + # Whatever a skipped operand would have bound is set to None up front: + # the explanation builds its arguments eagerly, and _call_reprcompare + # stops at the first false result, so it never reads those values. + may_be_skipped: list[str] = [] for i, op, next_operand in it: + if i: + self.statements.append(ast.If(load_names[i - 1], (inner := []), [])) + self.statements = inner + first_new_variable = len(self.variables) + # format_variables only exists with the assertion_pass hook on. + format_variables = getattr(self, "format_variables", None) + first_new_format_variable = len(format_variables or ()) match (next_operand, left_res): case ( ast.NamedExpr(target=ast.Name(id=target_id)), @@ -1127,7 +1145,21 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: expls.append(ast.Constant(expl)) res_expr = ast.copy_location(ast.Compare(left_res, [op], [next_res]), comp) self.statements.append(ast.Assign([store_names[i]], res_expr)) + if i: + may_be_skipped.append(res_variables[i]) + may_be_skipped.extend(self.variables[first_new_variable:]) + if format_variables is not None: + may_be_skipped.extend(format_variables[first_new_format_variable:]) left_res, left_expl = next_res, next_expl + self.statements = outer_statements + if may_be_skipped: + self.statements.insert( + chain_starts_at, + ast.Assign( + [ast.Name(name, ast.Store()) for name in may_be_skipped], + ast.Constant(None), + ), + ) # Use pytest.assertion.util._reprcompare if that's available. expl_call = self.helper( "_call_reprcompare", diff --git a/testing/test_assertrewrite.py b/testing/test_assertrewrite.py index 12e12449693..4b105bccc70 100644 --- a/testing/test_assertrewrite.py +++ b/testing/test_assertrewrite.py @@ -889,6 +889,35 @@ def f5() -> None: getmsg(f5, must_pass=True) + def test_comparison_chain_short_circuits(self) -> None: + def f1() -> None: + assert 1 < 0 < 1 / 0 + + # Not a ZeroDivisionError: Python never evaluates the last comparator. + assert getmsg(f1) == """assert 1 < 0""" + + def f2() -> None: + calls = [] + + def sentinel() -> int: + calls.append("called") + return 5 + + try: + assert 1 < 0 < sentinel() + except AssertionError: + pass + assert calls == [] + + getmsg(f2, must_pass=True) + + def f3() -> None: + a, b, c = range(3) + assert c < b < a < 1 / 0 # type: ignore[operator] + + # The chain is walked left to right, so the failure at "2 < 1" stops it. + assert getmsg(f3) == """assert 2 < 1""" + def test_len(self, request) -> None: def f(): values = list(range(10)) From c59b94e1e73a2e801693bd4d0c35c1ec89bc2434 Mon Sep 17 00:00:00 2001 From: SirHegel Date: Thu, 20 Aug 2026 12:18:55 -0500 Subject: [PATCH 2/3] Annotate inner and drop an unused type: ignore Co-authored-by: Claude --- src/_pytest/assertion/rewrite.py | 3 ++- testing/test_assertrewrite.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index bbcce08b03f..73dbbc57105 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -1121,7 +1121,8 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: may_be_skipped: list[str] = [] for i, op, next_operand in it: if i: - self.statements.append(ast.If(load_names[i - 1], (inner := []), [])) + inner: list[ast.stmt] = [] + self.statements.append(ast.If(load_names[i - 1], inner, [])) self.statements = inner first_new_variable = len(self.variables) # format_variables only exists with the assertion_pass hook on. diff --git a/testing/test_assertrewrite.py b/testing/test_assertrewrite.py index 4b105bccc70..e345b5f190c 100644 --- a/testing/test_assertrewrite.py +++ b/testing/test_assertrewrite.py @@ -913,7 +913,7 @@ def sentinel() -> int: def f3() -> None: a, b, c = range(3) - assert c < b < a < 1 / 0 # type: ignore[operator] + assert c < b < a < 1 / 0 # The chain is walked left to right, so the failure at "2 < 1" stops it. assert getmsg(f3) == """assert 2 < 1""" From bdbe261be1f7a82cfaddd11d8a56b140973c6a3f Mon Sep 17 00:00:00 2001 From: SirHegel Date: Thu, 20 Aug 2026 12:36:16 -0500 Subject: [PATCH 3/3] Also skip the explanation statements of a skipped operand An operand can contribute statements that run only when the assertion fails: a nested boolop appends to an explanation list built in the main statement body. Nesting only the main statements left those appends running unconditionally, against a list the skipped operand never bound, so assert 1 < 0 < (a or b) raised AttributeError on None. Nest expl_stmts on the same condition. Most operands add nothing there, and an If with an empty body is not valid ast, so the empty ones are dropped afterwards, innermost first: pruning a child can empty its parent. Collect the names to pre-bind by walking the conditional blocks for Store targets, rather than tracking self.variables and format_variables by index. That picks up @py_format names too, which pop_format_context only records when the assertion_pass hook is enabled, and drops a branch that no test could reach. Co-authored-by: Claude --- src/_pytest/assertion/rewrite.py | 43 ++++++++++++++++++++++---------- testing/test_assertrewrite.py | 8 ++++++ 2 files changed, 38 insertions(+), 13 deletions(-) diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index 73dbbc57105..dd35072ad66 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -1113,21 +1113,26 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: # first inside an "if" on the previous result so the rewritten form does # the same, the way visit_BoolOp already does for "and" and "or". outer_statements = self.statements + outer_expl_stmts = self.expl_stmts # comp.left is already emitted; the chain's own work starts here. chain_starts_at = len(self.statements) - # Whatever a skipped operand would have bound is set to None up front: - # the explanation builds its arguments eagerly, and _call_reprcompare - # stops at the first false result, so it never reads those values. - may_be_skipped: list[str] = [] + conditional_blocks: list[list[ast.stmt]] = [] + fail_ifs: list[tuple[list[ast.stmt], ast.If]] = [] for i, op, next_operand in it: if i: inner: list[ast.stmt] = [] self.statements.append(ast.If(load_names[i - 1], inner, [])) self.statements = inner - first_new_variable = len(self.variables) - # format_variables only exists with the assertion_pass hook on. - format_variables = getattr(self, "format_variables", None) - first_new_format_variable = len(format_variables or ()) + # An operand can also contribute statements that run only when + # the assertion fails, such as a nested boolop appending to its + # explanation list. Skip those too, or they run against names + # the skipped operand never bound. + fail_inner: list[ast.stmt] = [] + fail_if = ast.If(load_names[i - 1], fail_inner, []) + fail_ifs.append((self.expl_stmts, fail_if)) + self.expl_stmts.append(fail_if) + self.expl_stmts = fail_inner + conditional_blocks += (inner, fail_inner) match (next_operand, left_res): case ( ast.NamedExpr(target=ast.Name(id=target_id)), @@ -1146,13 +1151,25 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: expls.append(ast.Constant(expl)) res_expr = ast.copy_location(ast.Compare(left_res, [op], [next_res]), comp) self.statements.append(ast.Assign([store_names[i]], res_expr)) - if i: - may_be_skipped.append(res_variables[i]) - may_be_skipped.extend(self.variables[first_new_variable:]) - if format_variables is not None: - may_be_skipped.extend(format_variables[first_new_format_variable:]) left_res, left_expl = next_res, next_expl self.statements = outer_statements + self.expl_stmts = outer_expl_stmts + # Most operands contribute nothing to the explanation, and an "if" with + # an empty body is not valid ast. + for parent, fail_if in reversed(fail_ifs): # innermost first + if not fail_if.body: + parent.remove(fail_if) + # The explanation builds its arguments eagerly, so whatever the skipped + # statements would have bound is set to None first. Nothing reads those + # values: _call_reprcompare stops at the first false result, and a chain + # only short-circuits after one. + may_be_skipped = dict.fromkeys( + node.id + for block in conditional_blocks + for stmt in block + for node in ast.walk(stmt) + if isinstance(node, ast.Name) and isinstance(node.ctx, ast.Store) + ) if may_be_skipped: self.statements.insert( chain_starts_at, diff --git a/testing/test_assertrewrite.py b/testing/test_assertrewrite.py index e345b5f190c..9adabec2fb1 100644 --- a/testing/test_assertrewrite.py +++ b/testing/test_assertrewrite.py @@ -918,6 +918,14 @@ def f3() -> None: # The chain is walked left to right, so the failure at "2 < 1" stops it. assert getmsg(f3) == """assert 2 < 1""" + def f4() -> None: + a = b = 0 + # A boolop in a skipped comparator also contributes statements that + # only run when the assertion fails. Those have to be skipped too. + assert 1 < 0 < (a or b) + + assert getmsg(f4) == """assert 1 < 0""" + def test_len(self, request) -> None: def f(): values = list(range(10))