From 431245de75f25f371d1f19538c4cdef1118f3668 Mon Sep 17 00:00:00 2001 From: Evangelink Date: Tue, 19 May 2026 14:09:11 +0200 Subject: [PATCH] Assert.That follow-ups from PR #8306 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses three open review threads on the merged PR: * #3264506474 / #3264506501 — assignment and unary-update nodes were skipped entirely during analysis to keep the writable LHS / operand out of CaptureRewriter's non-writable Block wrapping. That kept hand- built expression trees compiling but lost diagnostic detail for any reads on the RHS or beneath the writable target. Move the writable- context guard into CaptureRewriter (new VisitBinary / VisitUnary / VisitWritableTarget) and let AnalyzeExpression process both sides normally. Tested with reused-instance LHS/RHS (e.g. \ox.Value = box.Value * 2\) to lock down the rubber-duck reused-node concern. * #3264506519 — the comment in EvaluateAndCollectDetails described the Func/Action filter as runtime-type-based but AnalyzeMemberExpression also filtered statically, silently dropping members typed as Func/Action that held a null value. Remove the analysis-time static-type filter, keep the runtime-type filter, and clarify the comment so null Func members render as \ ull\ again (matching the original behavior). Adds three regression tests: * That_ManuallyConstructedAssignExpression_CapturesValuesFromRhsAndLhs * That_ManuallyConstructedAssignExpression_WithReusedLhsInstance_DoesNotThrow * That_WithNullFuncDelegate_RendersAsNullInDetails All 884 AssertTests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../TestFramework/Assertions/Assert.That.cs | 84 ++++++++++++++----- .../Assertions/AssertTests.That.cs | 76 ++++++++++++++++- 2 files changed, 138 insertions(+), 22 deletions(-) diff --git a/src/TestFramework/TestFramework/Assertions/Assert.That.cs b/src/TestFramework/TestFramework/Assertions/Assert.That.cs index 08f08673c6..a80742faff 100644 --- a/src/TestFramework/TestFramework/Assertions/Assert.That.cs +++ b/src/TestFramework/TestFramework/Assertions/Assert.That.cs @@ -131,8 +131,9 @@ private static bool EvaluateAndCollectDetails(Expression body, Dictionarye at slot i is /// replaced by { var t = e; captures[i] = (object)t; t }, so e is evaluated exactly once. + /// Expressions that appear in a writable position (assignment LHS, unary-update operand) are visited + /// without wrapping the top-level node in a so the rewritten lambda + /// still compiles; reads under the writable target are still captured normally. /// private sealed class CaptureRewriter : ExpressionVisitor { @@ -613,6 +608,53 @@ public CaptureRewriter(Dictionary captureMap, ParameterExpressi return base.Visit(node); } + + protected override Expression VisitBinary(BinaryExpression node) + { + if (IsAssignmentNodeType(node.NodeType)) + { + // Keep the LHS in a writable form (don't wrap it in a Block) so the rewritten + // lambda still compiles, but recurse into its children so reads beneath the + // writable target are still captured. The RHS is processed normally. + Expression left = VisitWritableTarget(node.Left); + Expression right = Visit(node.Right)!; + return node.Update(left, node.Conversion, right); + } + + return base.VisitBinary(node); + } + + protected override Expression VisitUnary(UnaryExpression node) + { + if (IsUnaryUpdateNodeType(node.NodeType)) + { + // The operand of an update expression must remain writable; recurse into + // its children for captures but leave the operand itself unwrapped. + Expression operand = VisitWritableTarget(node.Operand); + return node.Update(operand); + } + + return base.VisitUnary(node); + } + + /// + /// Visits a node that must remain writable for the surrounding assignment/update expression + /// to compile. The node itself is NOT wrapped in a capture block; its children are visited + /// normally so reads beneath the writable target still get captured. + /// + private Expression VisitWritableTarget(Expression node) + => node switch + { + // For `obj.Field`/`obj.Property`, visit the receiver normally (a read) but + // leave the MemberExpression itself writable. + MemberExpression me when me.Expression is not null + => me.Update(Visit(me.Expression)), + BinaryExpression { NodeType: ExpressionType.ArrayIndex } ai + => ai.Update(Visit(ai.Left)!, ai.Conversion, Visit(ai.Right)!), + IndexExpression ix when ix.Object is not null + => ix.Update(Visit(ix.Object), Visit(ix.Arguments)), + _ => node, + }; } #if !NET diff --git a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs index b3a5256c6b..53e6795f88 100644 --- a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs +++ b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs @@ -1442,7 +1442,8 @@ public void That_ExtensionMethodOnThis_RendersAsThis() // ---- Assignment / update expression trees must not cause compilation failures ----------- // C# expression-tree lambdas do not allow assignment operators (the compiler emits CS0832), // so these nodes can only appear in manually-constructed expression trees. The fix ensures - // CaptureRewriter skips them rather than wrapping a writable LHS in a non-writable Block. + // CaptureRewriter does not wrap a writable LHS / unary-update operand in a non-writable + // Block, while still capturing reads beneath the writable target and on the RHS. private sealed class MutableBox { #pragma warning disable SA1401 // Fields should be private - intentional: this type tests Expression.Assign on a field. @@ -1483,6 +1484,79 @@ public void That_ManuallyConstructedPreIncrementAssignExpression_DoesNotThrow() act.Should().NotThrow(); box.Value.Should().Be(6); } + + public void That_ManuallyConstructedAssignExpression_CapturesValuesFromRhsAndLhs() + { + // Construct: (box.Value = (rhsValue + 1)) < expected + // The previous implementation skipped the entire Assign node during analysis and lost + // both the assigned member ("box.Value") and any read on the RHS. + var box = new MutableBox(); + int rhsValue = 9; + int expected = 0; + FieldInfo fi = typeof(MutableBox).GetField(nameof(MutableBox.Value))!; + MemberExpression boxField = Expression.Field(Expression.Constant(box), fi); + + // Borrow a real captured-local MemberExpression by compiling a tiny C# lambda. + Expression> rhsLambda = () => rhsValue; + Expression rhsRead = rhsLambda.Body; + + BinaryExpression assign = Expression.Assign( + boxField, + Expression.Add(rhsRead, Expression.Constant(1))); + BinaryExpression body = Expression.LessThan(assign, Expression.Constant(expected)); + var lambda = Expression.Lambda>(body); + + Action act = () => Assert.That(lambda); + + AssertFailedException ex = act.Should().Throw().Which; + // RHS read is captured (with its display-class wrapper stripped). + ex.Message.Should().Contain("rhsValue = 9"); + // LHS member is captured too; the fallback re-evaluation surfaces the post-assignment value. + ex.Message.Should().Contain(".Value = 10"); + // The assignment must have actually run (preserved by the writable-target guard). + box.Value.Should().Be(10); + } + + public void That_ManuallyConstructedAssignExpression_WithReusedLhsInstance_DoesNotThrow() + { + // Hand-built tree where the same MemberExpression instance is used both as the + // assignment LHS and inside the RHS. CaptureRewriter must leave the LHS writable + // even though analysis adds the shared instance to its capture map (because it's + // also a read on the RHS). Locks down rubber-duck's reused-node concern. + var box = new MutableBox { Value = 3 }; + FieldInfo fi = typeof(MutableBox).GetField(nameof(MutableBox.Value))!; + MemberExpression field = Expression.Field(Expression.Constant(box), fi); + // box.Value = box.Value * 2 + BinaryExpression assign = Expression.Assign(field, Expression.Multiply(field, Expression.Constant(2))); + BinaryExpression body = Expression.GreaterThan(assign, Expression.Constant(0)); + var lambda = Expression.Lambda>(body); + + Action act = () => Assert.That(lambda); + + act.Should().NotThrow(); + box.Value.Should().Be(6); + } + + // ---- Func/Action filtering preserves historical null-rendering behavior ----------------- + public void That_WithNullFuncDelegate_RendersAsNullInDetails() + { + // A member statically typed as Func<...> but holding `null` must still appear in the + // failure details (as `null`) — matching the historical behavior. Previously the + // static-type filter at analysis time dropped these silently. + Func? predicate = null; + bool flag = false; + + Action act = () => Assert.That(() => flag || predicate != null); + + act.Should().Throw() + .WithMessage( + """ + Assert.That(() => flag || predicate != null) failed. + Details: + flag = False + predicate = null + """); + } } internal static class AssertTestsExtensions