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/changelog/14817.improvement.rst b/changelog/14817.improvement.rst new file mode 100644 index 00000000000..da6d5d47bf0 --- /dev/null +++ b/changelog/14817.improvement.rst @@ -0,0 +1,4 @@ +Assertion failure messages now show a method call on a single line, without the bound method as a separate intermediate:: + + assert 42 == 100 + + where 42 = Obj().compute() diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index 27953336c5c..a0e203debcc 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]: @@ -1059,7 +1080,21 @@ def visit_Call(self, call: ast.Call) -> tuple[ast.Name, str]: # The callee and every argument are evaluated left to right, so each of # them has to be frozen against walrus operators in what follows. operands = [*call.args, *(keyword.value for keyword in call.keywords)] - new_func, func_expl = self.visit_operand(call.func, operands) + if isinstance(call.func, ast.Attribute) and isinstance(call.func.ctx, ast.Load): + # obj.method(...) reads better flat -- "where 42 = Obj().compute()" + # rather than a separate "where compute = Obj().compute" line. The + # bound method still gets a temporary of its own, because Python + # looks it up before evaluating the arguments; that is also what + # keeps the receiver ordered ahead of them. + receiver, receiver_expl = self.visit(call.func.value) + new_func: ast.expr = self.assign( + ast.copy_location( + ast.Attribute(receiver, call.func.attr, ast.Load()), call.func + ) + ) + func_expl = f"{receiver_expl}.{call.func.attr}" + else: + new_func, func_expl = self.visit_operand(call.func, operands) arg_expls = [] new_args = [] new_kwargs = [] @@ -1088,9 +1123,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 +1167,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/python/raises_group.py b/testing/python/raises_group.py index 950e71753c2..4a53540267f 100644 --- a/testing/python/raises_group.py +++ b/testing/python/raises_group.py @@ -1237,11 +1237,10 @@ def test_assert_matches() -> None: match=wrap_escape( "`ValueError()` is not an instance of `TypeError`\n" "assert False\n" - " + where False = matches(ValueError())\n" - " + where matches = RaisesExc(TypeError).matches" + " + where False = RaisesExc(TypeError).matches(ValueError())" ), ): - # you'd need to do this arcane incantation + # binding the RaisesExc is still how you get at ``fail_reason`` assert (m := RaisesExc(TypeError)).matches(e), m.fail_reason # but even if we add assert_matches, will people remember to use it? diff --git a/testing/test_assertrewrite_coverage.py b/testing/test_assertrewrite_coverage.py index 43a3f668f73..e5f011c4c93 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(""" @@ -638,7 +678,41 @@ def check(): class TestIntrospectionMethodCall: - """Method calls — currently show the bound method as its own "where" line.""" + """Method calls — flat obj.method() display without bound-method noise.""" + + def test_method_call_flat_format(self) -> None: + """Method calls show 'where result = obj.method()' in one line.""" + assert_introspects( + """ + def check(): + class Obj: + def compute(self): + return 42 + def __repr__(self): + return "Obj()" + obj = Obj() + assert obj.compute() == 100 + """, + must_contain=["where 42 = Obj().compute()"], + ) + + def test_method_call_no_bound_method_noise(self) -> None: + """No separate 'where compute = obj.compute' line.""" + msg = get_failure_message(""" + def check(): + class Obj: + def compute(self): + return 42 + def __repr__(self): + return "Obj()" + obj = Obj() + assert obj.compute() == 100 + """) + lines = msg.splitlines() + for line in lines: + assert "where compute = " not in line, ( + f"Noisy bound-method intermediate found:\n{msg}" + ) def test_callable_variable_shows_result(self) -> None: # Current behavior: shows full function repr, not variable name @@ -976,6 +1050,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 +1078,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(""" @@ -1097,6 +1201,24 @@ def collect(**kwargs): return "passed", mapping """) + def test_method_lookup_precedes_arguments(self) -> None: + """Guard: the bound method is looked up before the arguments run.""" + assert_evaluation_order(""" + def check(): + trace = [] + class Box: + @property + def take(self): + trace.append("lookup") + return lambda value: value + obj = Box() + try: + assert obj.take(trace.append("argument")) is None + except AssertionError: + return "raised", trace + return "passed", trace + """) + def test_ifexp_branches_in_order(self) -> None: """Guard: the condition is evaluated before the selected branch.""" assert_evaluation_order(""" @@ -1120,6 +1242,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( @@ -1144,6 +1278,42 @@ def check(): must_contain=["42", "100"], ) + def test_method_call_with_args(self) -> None: + """Method call with arguments shows flat format.""" + assert_introspects( + """ + def check(): + class Calculator: + def add(self, a, b): + return a + b + def __repr__(self): + return "Calc()" + c = Calculator() + assert c.add(2, 3) == 10 + """, + must_contain=["where 5 = Calc().add(2, 3)"], + ) + + def test_chained_method_calls(self) -> None: + """Chained method call: obj.method1().method2().""" + assert_introspects( + """ + def check(): + class Builder: + def __init__(self, val=0): + self.val = val + def add(self, n): + return Builder(self.val + n) + def result(self): + return self.val + def __repr__(self): + return f"Builder({self.val})" + b = Builder() + assert b.add(5).result() == 100 + """, + must_contain=["where 5 = ", ".result()"], + ) + def test_subscript_on_method_result(self) -> None: """Subscript on method return value: obj.method()[key].""" assert_introspects( @@ -1160,6 +1330,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(""" @@ -1229,3 +1411,14 @@ def check(): assert d["key"] == 100, "custom failure message" """) assert "custom failure message" in msg + + def test_method_call_on_global(self) -> None: + """Method call on a global/module-level object.""" + assert_introspects( + """ + items = [1, 2, 3] + def check(): + assert items.count(99) == 1 + """, + must_contain=["where 0 = [1, 2, 3].count(99)"], + )