diff --git a/changelog/14814.bugfix.rst b/changelog/14814.bugfix.rst new file mode 100644 index 00000000000..6e44ab4976f --- /dev/null +++ b/changelog/14814.bugfix.rst @@ -0,0 +1 @@ +Fixed assertion rewriting evaluating a walrus operator (``:=``) or a starred argument out of order when a later argument assigned to the same name, so ``assert collect(*items, identity(items := [9]))`` now passes the pre-assignment ``items``. diff --git a/changelog/14815.improvement.rst b/changelog/14815.improvement.rst new file mode 100644 index 00000000000..0e316edea07 --- /dev/null +++ b/changelog/14815.improvement.rst @@ -0,0 +1,4 @@ +Assertion failure messages now decompose subscript expressions, showing the container and the key that produced a value:: + + assert 1 == 99 + + where 1 = {'a': 1, 'b': 2}['a'] diff --git a/changelog/14816.improvement.rst b/changelog/14816.improvement.rst new file mode 100644 index 00000000000..ca3ba124728 --- /dev/null +++ b/changelog/14816.improvement.rst @@ -0,0 +1,4 @@ +Assertion failure messages now show the condition of a conditional expression:: + + assert 0 == 99 + + where 0 = (... if True else ...) diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index 27953336c5c..27d9285aa82 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -967,19 +967,40 @@ def visit_operand( """Visit an operand, freezing it against walrus operators in *later*. Operands are rewritten into statements that run in source order, but - a plain name is left as a bare load evaluated at the very end, when - the enclosing expression is assembled. A walrus operator in a later - operand rebinds that name in between, so both the value used and the - value reported would be the post-walrus one -- Python evaluates the - earlier operand first. Copy the value into a temporary instead. + two of them stay unhoisted and are evaluated at the very end, when the + enclosing expression is assembled -- after everything that follows + them: + + * a plain name, which a walrus operator in a later operand rebinds in + between, so the value used and the value reported would be the + post-walrus one; + * a walrus operator itself, which would then assign in the wrong + order, and be visible to the operands that were meant to precede it. + + Either way Python evaluates the earlier operand first, so copy it into + a temporary here. A starred argument is unwrapped and rewrapped, its + value being subject to the same problem. """ specifiers = set(self.explanation_specifiers) res, expl = self.visit(operand) - if isinstance(res, ast.Name) and res.id in _walrus_targets(later): - snapshot = self.assign(res) + value = res.value if isinstance(res, ast.Starred) else res + if isinstance(value, ast.NamedExpr): + needs_freeze = bool(later) + else: + # Every other operand arrives as a temporary: the visit_* methods + # hoist what they build, and generic_visit assigns whatever is left + # -- a literal included -- so a name is all that can reach here. + assert isinstance(value, ast.Name) + needs_freeze = value.id in _walrus_targets(later) + if needs_freeze: + snapshot = self.assign(value) for key in set(self.explanation_specifiers) - specifiers: self.explanation_specifiers[key] = self.display(snapshot) - res = snapshot + res = ( + ast.copy_location(ast.Starred(snapshot, res.ctx), res) + if isinstance(res, ast.Starred) + else snapshot + ) return res, expl def visit_BoolOp(self, boolop: ast.BoolOp) -> tuple[ast.Name, str]: @@ -1088,9 +1109,36 @@ def visit_Starred(self, starred: ast.Starred) -> tuple[ast.Starred, str]: new_starred = ast.Starred(res, starred.ctx) return new_starred, "*" + expl + def visit_IfExp(self, ifexp: ast.IfExp) -> tuple[ast.Name, str]: + # Introspect the condition but keep the branches as they are: only the + # selected one may be evaluated, so neither can be hoisted. That also + # keeps them ordered after the condition, which is where Python puts + # them, so no freeze is needed here. + cond_res, cond_expl = self.visit(ifexp.test) + res = self.assign( + ast.copy_location(ast.IfExp(cond_res, ifexp.body, ifexp.orelse), ifexp) + ) + res_expl = self.explanation_param(self.display(res)) + pat = "%s\n{%s = (... if %s else ...)\n}" + expl = pat % (res_expl, res_expl, cond_expl) + return res, expl + + def visit_Subscript(self, subscript: ast.Subscript) -> tuple[ast.Name, str]: + # For Slice objects (a[1:3]), fall back to generic — decomposing + # start/stop/step is rarely useful in assertion messages. + if isinstance(subscript.slice, ast.Slice): + return self.generic_visit(subscript) + value, value_expl = self.visit_operand(subscript.value, [subscript.slice]) + slice_res, slice_expl = self.visit(subscript.slice) + res = self.assign( + ast.copy_location(ast.Subscript(value, slice_res, ast.Load()), subscript) + ) + res_expl = self.explanation_param(self.display(res)) + pat = "%s\n{%s = %s[%s]\n}" + expl = pat % (res_expl, res_expl, value_expl, slice_expl) + return res, expl + def visit_Attribute(self, attr: ast.Attribute) -> tuple[ast.Name, str]: - if not isinstance(attr.ctx, ast.Load): - return self.generic_visit(attr) value, value_expl = self.visit(attr.value) res = self.assign( ast.copy_location(ast.Attribute(value, attr.attr, ast.Load()), attr) @@ -1105,8 +1153,6 @@ def visit_Compare(self, comp: ast.Compare) -> tuple[ast.expr, str]: left_res, left_expl = self.visit_operand(comp.left, comp.comparators) if isinstance(comp.left, ast.Compare | ast.BoolOp): left_expl = f"({left_expl})" - if isinstance(left_res, ast.NamedExpr): - left_res = self.assign(left_res) res_variables = [self.variable() for i in range(len(comp.ops))] load_names: list[ast.expr] = [ast.Name(v, ast.Load()) for v in res_variables] store_names = [ast.Name(v, ast.Store()) for v in res_variables] diff --git a/testing/test_assertrewrite_coverage.py b/testing/test_assertrewrite_coverage.py index 43a3f668f73..8af2a741c23 100644 --- a/testing/test_assertrewrite_coverage.py +++ b/testing/test_assertrewrite_coverage.py @@ -535,6 +535,26 @@ def check(): class TestIntrospectionSubscript: """Subscript / indexing.""" + def test_dict_subscript_shows_key_and_container(self) -> None: + assert_introspects( + """ + def check(): + d = {"a": 1, "b": 2} + assert d["a"] == 99 + """, + must_contain=["where 1 = ", "['a']"], + ) + + def test_list_subscript_shows_index_and_container(self) -> None: + assert_introspects( + """ + def check(): + items = [10, 20, 30] + assert items[1] == 99 + """, + must_contain=["where 20 = ", "[1]"], + ) + def test_subscript_semantics_preserved(self) -> None: assert_semantically_equivalent(""" def check(): @@ -557,6 +577,16 @@ def check(): class TestIntrospectionIfExp: """Ternary / if-expression.""" + def test_ifexp_shows_condition_value(self) -> None: + assert_introspects( + """ + def check(): + flag = True + assert (0 if flag else 1) == 1 + """, + must_contain=["if True else"], + ) + def test_ifexp_semantics_preserved(self) -> None: assert_semantically_equivalent(""" def check(): @@ -564,6 +594,16 @@ def check(): assert (0 if flag else 1) == 1 """) + def test_ifexp_in_compare_shows_result(self) -> None: + assert_introspects( + """ + def check(): + flag = True + assert (0 if flag else 1) == 99 + """, + must_contain=["assert 0 == 99", "if True else"], + ) + def test_ifexp_short_circuit_true(self) -> None: """Orelse branch must NOT be evaluated when condition is True.""" assert_passes_when_true(""" @@ -976,6 +1016,21 @@ def identity(v): return "passed", value """) + def test_starred_argument_precedes_walrus(self) -> None: + assert_evaluation_order(""" + def check(): + def identity(v): + return v + def collect(*values): + return values + items = [1] + try: + assert collect(*items, identity(items := [9])) == (1, [9]) + except AssertionError: + return "raised", items + return "passed", items + """) + def test_chained_compare_operands_in_order(self) -> None: assert_evaluation_order(""" def check(): @@ -989,6 +1044,21 @@ def identity(v): return "passed", value """) + def test_bare_walrus_argument_in_order(self) -> None: + """A walrus argument is evaluated in place, before the ones after it.""" + assert_evaluation_order(""" + def check(): + def identity(v): + return v + def collect(*values): + return values + try: + assert collect((x := 1), identity(x := 2)) == (1, 2) + except AssertionError: + return "raised", x + return "passed", x + """) + def test_container_literal_operand_in_order(self) -> None: """Guard: ``generic_visit`` hoists container literals into a temporary.""" assert_evaluation_order(""" @@ -1120,6 +1190,18 @@ def identity(v): class TestEdgeCases: """Regression and edge-case tests combining multiple expression types.""" + def test_subscript_with_variable_key(self) -> None: + """Subscript where the key is a variable (not constant).""" + assert_introspects( + """ + def check(): + d = {"hello": 42} + key = "hello" + assert d[key] == 100 + """, + must_contain=["where 42 = ", "['hello']"], + ) + def test_subscript_with_call_key(self) -> None: """Subscript where the key is a function call.""" assert_introspects( @@ -1160,6 +1242,18 @@ def __repr__(self): must_contain=["42", "100"], ) + def test_ifexp_with_call_condition(self) -> None: + """IfExp where condition is a function call.""" + assert_introspects( + """ + def check(): + def is_ready(): + return False + assert (1 if is_ready() else 0) == 1 + """, + must_contain=["if False else"], + ) + def test_walrus_in_subscript(self) -> None: """Walrus operator used as subscript key.""" assert_semantically_equivalent("""