From 29c87dea9c37a83a88cc4371de842dd7cedc188f Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Tue, 19 May 2026 10:57:10 +0200 Subject: [PATCH] Address review: capture readable LHS sub-expressions and restore runtime Func/Action filter - AnalyzeWritableOperand now recurses into readable sub-expressions of an assignment LHS or unary-update operand (member receiver, index targets and arguments) while leaving the writable storage location uncaptured. Failure details for manually-constructed assignment/update trees now include receiver values and assignment RHS, addressing reviewer feedback that the previous skip-entirely approach hid useful diagnostic information. - AnalyzeMemberExpression no longer drops captures for statically-typed Func/Action members. Filtering is now applied uniformly at detail-build time using the runtime value's type, matching the historical behavior so that a null delegate-typed member still shows up as 'null'. - Updated the perf trade-off comment near the Compile() call site to be more precise about why reference-keyed plan caching would not help the common inline 'Assert.That(() => ...)' usage pattern. - New regression tests cover the readable receiver-chain capture on assignment and pre-decrement trees, and lock in the runtime-typed delegate filter so null Func members keep surfacing in failure details. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../TestFramework/Assertions/Assert.That.cs | 88 +++++++++++++++---- .../Assertions/AssertTests.That.cs | 88 +++++++++++++++++++ 2 files changed, 159 insertions(+), 17 deletions(-) diff --git a/src/TestFramework/TestFramework/Assertions/Assert.That.cs b/src/TestFramework/TestFramework/Assertions/Assert.That.cs index 08f08673c6..13465ac6a5 100644 --- a/src/TestFramework/TestFramework/Assertions/Assert.That.cs +++ b/src/TestFramework/TestFramework/Assertions/Assert.That.cs @@ -112,8 +112,10 @@ private static bool EvaluateAndCollectDetails(Expression body, Dictionary ...)` because each call site + // constructs a fresh Expression> instance at runtime, but could be considered later if a + // workload that re-uses the same expression tree across calls demonstrates measurable overhead. var lambda = Expression.Lambda>(rewrittenBody, arrayParam); object?[] values = new object?[context.CaptureNames.Count]; @@ -131,8 +133,9 @@ private static bool EvaluateAndCollectDetails(Expression body, Dictionary + /// Analyzes the writable left-hand operand of an assignment or unary-update expression. + /// The writable storage location itself (e.g., a field/property/index access used as + /// the target of an assignment) is NOT added as a capture — wrapping it in a Block via + /// would make it non-writable and break compilation of the + /// rewritten lambda. However, its readable sub-expressions (the receiver of a member + /// access, the array reference and indices of an indexer) ARE analyzed normally so the + /// failure-detail message still reports those intermediate values for + /// manually-constructed expression trees. + /// + private static void AnalyzeWritableOperand(Expression? expr, AnalysisContext context, bool suppressIntermediateValues) + { + if (expr is null) + { + return; + } + + switch (expr) + { + // Field/property access used as a write target: traverse the receiver chain for + // reads, but don't capture the member itself. + case MemberExpression memberExpr when memberExpr.Expression is not null: + AnalyzeExpression(memberExpr.Expression, context, suppressIntermediateValues); + break; + + // `arr[i] = ...` via the BinaryExpression ArrayIndex form: the array reference and + // the index are both reads (assignments to single-dimensional arrays however use + // the IndexExpression form below). + case BinaryExpression { NodeType: ExpressionType.ArrayIndex } arrayIndex: + AnalyzeExpression(arrayIndex.Left, context, suppressIntermediateValues); + AnalyzeExpression(arrayIndex.Right, context, suppressIntermediateValues); + break; + + // Indexer `arr[i] = ...` or `obj[a, b] = ...`: the receiver and all index arguments + // are reads. + case IndexExpression indexExpr: + AnalyzeExpression(indexExpr.Object, context, suppressIntermediateValues); + foreach (Expression arg in indexExpr.Arguments) + { + AnalyzeExpression(arg, context, suppressIntermediateValues); + } + + break; + } + + // ParameterExpression (e.g., `++x` on a local) and static field access have no readable + // sub-expressions worth analyzing — they fall through the switch above with no-op. + } + private static void AnalyzeMethodCallExpression(MethodCallExpression callExpr, AnalysisContext context, bool suppressIntermediateValues = false) { // Special handling for indexers (get_Item calls) diff --git a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs index b3a5256c6b..3a9c0f6f38 100644 --- a/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs +++ b/test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs @@ -1483,6 +1483,94 @@ public void That_ManuallyConstructedPreIncrementAssignExpression_DoesNotThrow() act.Should().NotThrow(); box.Value.Should().Be(6); } + + private sealed class MutableContainer + { +#pragma warning disable SA1401 // Fields should be private - intentional: this type tests Expression.Assign on nested members. + public MutableBox Inner = new() { Value = 0 }; +#pragma warning restore SA1401 + } + + public void That_ManuallyConstructedAssignExpression_AnalyzesReceiverChainAndRhs() + { + // Construct: (container.Inner.Value = ComputeValue()) < 0 — fails, so message must + // include both the readable receiver chain (container.Inner) AND the RHS computation + // result captured from the sole evaluation of the assignment (#6690 single-pass guarantee). + var container = new MutableContainer(); + FieldInfo innerFi = typeof(MutableContainer).GetField(nameof(MutableContainer.Inner))!; + FieldInfo valueFi = typeof(MutableBox).GetField(nameof(MutableBox.Value))!; + + MemberExpression innerAccess = Expression.Field(Expression.Constant(container), innerFi); + MemberExpression valueAccess = Expression.Field(innerAccess, valueFi); + MethodCallExpression rhs = Expression.Call(typeof(MutableBoxHelper).GetMethod(nameof(MutableBoxHelper.ComputeValue))!); + BinaryExpression assign = Expression.Assign(valueAccess, rhs); + BinaryExpression body = Expression.LessThan(assign, Expression.Constant(0)); + var lambda = Expression.Lambda>(body); + + Action act = () => Assert.That(lambda); + + // Failure message should reflect that the readable receiver and RHS were both captured + // during the single evaluation of the assignment (i.e., neither the LHS subtree nor the + // RHS was skipped wholesale by the analyzer). + AssertFailedException ex = act.Should().Throw().Which; + ex.Message.Should().Contain("ComputeValue()"); + // The Inner member of the writable LHS receiver chain was captured as a read-side + // sub-expression (rather than the whole assignment being skipped wholesale). + ex.Message.Should().Contain("Inner"); + // The assignment side-effect must still apply since the lambda is evaluated exactly once. + container.Inner.Value.Should().Be(42); + } + + public void That_ManuallyConstructedPreIncrementExpression_AnalyzesReceiverChain() + { + // Construct: --container.Inner.Value > 0 — fails (0 - 1 = -1 is not > 0). + // The readable receiver chain (container.Inner) must still be captured even though the + // writable storage location (container.Inner.Value) is not. + var container = new MutableContainer { Inner = new MutableBox { Value = 0 } }; + FieldInfo innerFi = typeof(MutableContainer).GetField(nameof(MutableContainer.Inner))!; + FieldInfo valueFi = typeof(MutableBox).GetField(nameof(MutableBox.Value))!; + + MemberExpression innerAccess = Expression.Field(Expression.Constant(container), innerFi); + MemberExpression valueAccess = Expression.Field(innerAccess, valueFi); + UnaryExpression preDec = Expression.PreDecrementAssign(valueAccess); + BinaryExpression body = Expression.GreaterThan(preDec, Expression.Constant(0)); + var lambda = Expression.Lambda>(body); + + Action act = () => Assert.That(lambda); + + AssertFailedException ex = act.Should().Throw().Which; + // The Inner member of the writable LHS receiver chain was captured as a read-side + // sub-expression even though the writable target (Inner.Value) is not captured. + ex.Message.Should().Contain("Inner"); + // Side effect of the single root evaluation applied. + container.Inner.Value.Should().Be(-1); + } + + // Regression: a member whose static type is Func/Action but whose runtime value is null should + // still appear in the failure details, matching pre-PR behavior. Filtering must remain + // runtime-typed at detail-build time rather than static-typed at analysis time. + private sealed class HolderWithFuncField + { +#pragma warning disable SA1401 // Fields should be private - intentional: test fixture exposing a delegate-typed field. + public Func? Callback; +#pragma warning restore SA1401 + } + + public void That_NullDelegateTypedMember_StillAppearsInDetails() + { + var holder = new HolderWithFuncField { Callback = null }; + + Action act = () => Assert.That(() => holder.Callback != null); + + AssertFailedException ex = act.Should().Throw().Which; + ex.Message.Should().Contain("Callback"); + ex.Message.Should().Contain("null"); + } +} + +internal static class MutableBoxHelper +{ + public static int ComputeValue() => 42; } internal static class AssertTestsExtensions