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..dd35072ad66 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -1108,7 +1108,31 @@ 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 + outer_expl_stmts = self.expl_stmts + # comp.left is already emitted; the chain's own work starts here. + chain_starts_at = len(self.statements) + 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 + # 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)), @@ -1128,6 +1152,32 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: res_expr = ast.copy_location(ast.Compare(left_res, [op], [next_res]), comp) self.statements.append(ast.Assign([store_names[i]], res_expr)) 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, + 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..9adabec2fb1 100644 --- a/testing/test_assertrewrite.py +++ b/testing/test_assertrewrite.py @@ -889,6 +889,43 @@ 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 + + # 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))