Assert.That: capture readable LHS sub-expressions and restore runtime Func/Action filter

[Evangelink]

Follow-up to #8306 (merged) addressing the remaining unresolved review comments on Assert.That(Expression<Func<bool>>).

Summary of review comments addressed

1. Capture readable LHS sub-expressions of assignment / unary-update nodes (review comments 3264506474, 3264506501)

The previous behavior was to skip assignment-style BinaryExpression and unary-update UnaryExpression nodes entirely in AnalyzeExpression, which avoided the writable-LHS-wrapped-in-Block compile issue but also dropped diagnostic information for manually-constructed expression trees.

AnalyzeWritableOperand now recurses into the readable sub-expressions of the writable LHS (member receiver, index target and arguments) while leaving the writable storage location itself uncaptured. The RHS of assignments is now analyzed normally as well. Empirically verified that MemberExpression(Block(...), Field) = value and Expression.PreIncrementAssign(MemberExpression(Block(...), Field)) both compile and assign correctly via the .NET Expression API.

2. Restore runtime-typed Func/Action filter (review comment 3264506519)

AnalyzeMemberExpression previously skipped captures for members whose static type was Func/Action. This was inconsistent with the comment at the detail-build loop ("matching the historical behavior, using runtime type as the existing code did") and dropped null delegate-typed members from failure details, which previously surfaced as null.

The static-type skip is removed; filtering is now applied uniformly at detail-build time using the runtime value's type. A regression test (That_NullDelegateTypedMember_StillAppearsInDetails) locks in the restored behavior.

3. Caching trade-off comment refresh (review comment 3264506539)

The existing perf trade-off comment near the Compile() site is updated to be more precise: reference-keyed plan caching would not help the common inline Assert.That(() => ...) pattern because each call site constructs a fresh Expression<Func<bool>> instance at runtime, but it could be considered later if a workload that re-uses the same expression tree across calls demonstrates measurable overhead. No caching is implemented in this PR.

Tests

Three new unit tests in AssertTests.That.cs:

• That_ManuallyConstructedAssignExpression_AnalyzesReceiverChainAndRhs — verifies the readable LHS receiver chain and the RHS computation result both appear in failure details for a manually-constructed (container.Inner.Value = ComputeValue()) < 0 tree.
• That_ManuallyConstructedPreIncrementExpression_AnalyzesReceiverChain — verifies the readable LHS receiver chain appears in failure details for --container.Inner.Value > 0 and that the side effect still applies (single-pass).
• That_NullDelegateTypedMember_StillAppearsInDetails — locks in the restored runtime-typed Func/Action filter so a null Func<int> field still surfaces as null in the details.

All 882 AssertTests pass on net9.0.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Improves Assert.That(Expression<Func<bool>>) failure-detail generation by capturing readable sub-expressions for assignment/unary-update nodes while keeping the compiled expression single-pass, and restores historical runtime-based Func/Action filtering.

Changes:

• Analyze assignment and unary-update LHS operands by recursing into readable receiver/index sub-expressions while avoiding captures that would break LValue writability.
• Move Func/Action filtering to detail-build time using runtime values (so null delegate-typed members still appear as null).
• Add unit tests covering manually-constructed assign/pre-decrement trees and the null delegate regression.

Show a summary per file

File	Description
test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs	Adds new unit tests for writable-LHS analysis and runtime delegate filtering behavior.
src/TestFramework/TestFramework/Assertions/Assert.That.cs	Updates expression analysis to safely capture readable sub-expressions for writable operands and adjusts runtime delegate filtering/comments.

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 6

[Evangelink]

Merged main and resolved conflicts

PR #8307 was merged into main in parallel with this work and replaced the AnalyzeExpression / CaptureRewriter architecture from #8306 that this PR was patching. The merge therefore had non-trivial conflicts in Assert.That.cs.

Resolution (commit f28f470):

• src/.../Assert.That.cs: accepted main's version wholesale. The writable-LHS analysis fix from this PR no longer applies — the method it patched no longer exists. IsFuncOrActionType in main already filters by runtime type (line 794), so the runtime-Func/Action filter intent of this PR is preserved on main.
• Tests kept: That_ManuallyConstructedAssignExpression_SurfacesRhsAndPreservesSideEffect (renamed) and That_NullDelegateTypedMember_StillAppearsInDetails. Both pass locally on main's architecture and remain useful regression coverage.
• Test dropped: That_ManuallyConstructedPreIncrementExpression_AnalyzesReceiverChain — it exposes a real but separate double-evaluation bug in main's EvaluateAllSubExpressions for PreDecrementAssign / PreIncrementAssign (the side effect runs twice, leaving Value = -2 instead of -1). That should be tracked and fixed in its own PR/issue; merging a failing test would block this one.

Review comments

All 6 Copilot review comments have been replied to:

• 3 [TestMethod] comments → false positives (file uses TestContainer, no [TestMethod] attributes used anywhere in the file).
• Brittle ComputeValue() assertion → relaxed to nameof(...).
• 2 BindingFlags suggestions → kept consistent with the convention used elsewhere in the file.