Assert.That follow-ups from #8306 review

[Evangelink]

Follow-up to #8306 addressing three open review threads.

Review threads addressed

#3264506474 / #3264506501 — assignment / unary-update analysis lost RHS detail

The merged PR skipped assignment-style BinaryExpression nodes (=, +=, …) and unary-update UnaryExpression nodes (++x, x--, …) 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 (assignment) or beneath the writable target.

Moved the writable-context guard into CaptureRewriter (new VisitBinary / VisitUnary / VisitWritableTarget) and let AnalyzeExpression process both sides normally. The new VisitWritableTarget leaves the top-level writable node (e.g. obj.Field LHS) unwrapped while still recursing into its children so reads beneath the writable target are still captured.

This also robustly handles the rubber-duck-identified reused-node scenario: in a hand-built tree like Expression.Assign(field, Expression.Multiply(field, c)) where the same MemberExpression instance appears in both LHS and RHS, the rewriter discriminates per syntactic position (LHS stays writable, RHS gets wrapped) rather than per instance.

#3264506519 — Func/Action filter comment didn't match the code

The comment in EvaluateAndCollectDetails claimed the Func/Action filter used "runtime type as the existing code did", but AnalyzeMemberExpression added a static-type filter that silently dropped members typed as Func/Action holding a null value — a behavior regression vs. the original implementation.

Removed the analysis-time static-type filter, kept the runtime-type filter at detail-build time, and clarified the comments. Null Func/Action members now render as null again, matching the historical behavior.

Other threads on #8306 reviewed but not changed

Comment	Status
#3258059677 (line 468)	Already addressed in the merge — IsSafeToReevaluate recurses into children. Test That_ShortCircuitedExpression_DoesNotReevaluateMemberExpressionWithSideEffectingReceiver covers it.
#3258059733 (line 410)	Already addressed — IsCapturedThis uses IsAssignableFrom so inherited methods on this render correctly. Test That_InheritedInstanceMethodOnThis_RendersAsThis locks it down.
#3258059756 (line 330)	Already addressed — IsArrayGetMethod requires Object.Type.IsArray, so arbitrary Get(...) methods don't render as indexers. Test That_ArbitraryGetMethod_RendersAsMethodCall_NotIndexer covers it.
#3260908985 (line 375)	Already addressed — extension-method receivers go through IsCapturedThis. Test That_ExtensionMethodOnThis_RendersAsThis covers it.
#3264506539 (line 117)	Deferred. Caching by ConditionalWeakTable<Expression, …> is interesting but Expression<Func<bool>> lambdas are typically rebuilt per Assert.That call, so the benefit is speculative without a benchmark. The existing inline comment already documents it as a future mitigation.

Tests

Adds three regression tests in test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs:

• That_ManuallyConstructedAssignExpression_CapturesValuesFromRhsAndLhs — proves the new behavior: a hand-built box.Value = rhsValue + 1 tree surfaces both rhsValue and the post-assignment box.Value in the failure details.
• That_ManuallyConstructedAssignExpression_WithReusedLhsInstance_DoesNotThrow — locks down the reused-node case where the same MemberExpression instance is both LHS and RHS sub-expression.
• That_WithNullFuncDelegate_RendersAsNullInDetails — locks down that Func<…>-typed members holding null render as null (regression guard for the comment/code inconsistency).

All 884 AssertTests pass locally.

[Copilot]

Pull request overview

Note

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

Follow-up to #8306 to restore assertion “details” analysis for assignment/unary-update expression trees while keeping rewritten lambdas compilable, and to preserve historical rendering of null Func/Action members.

Changes:

• Move “writable target” handling into CaptureRewriter so assignment/update nodes can be analyzed without breaking compilation.
• Remove analysis-time static Func/Action filtering; keep runtime filtering during detail construction to allow null Func/Action members to render as null.
• Add regression tests covering assignment RHS/LHS capture behavior, reused LHS instance handling, and null Func rendering.

Show a summary per file

File	Description
test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs	Adds regression tests for assignment/unary-update capture behavior and null Func/Action detail rendering.
src/TestFramework/TestFramework/Assertions/Assert.That.cs	Reworks capture rewriting to preserve writability for assignment/update targets and adjusts Func/Action filtering semantics/comments.

Copilot's findings

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

[Evangelink]

Closing this PR — it has been superseded by #8307, which landed on main after this PR was opened and completely re-architected src/TestFramework/TestFramework/Assertions/Assert.That.cs (+1258 / −805 on that single file). All of the code paths this PR was touching no longer exist:

#8359 touches	After #8307
CaptureRewriter / VisitWritableTarget	Gone — no rewriting at all. The new code uses bottom-up EvaluateAllSubExpressions + constant substitution.
AnalyzeExpression / IsAssignmentNodeType / IsUnaryUpdateNodeType	Gone — assignment/update concerns are absorbed by the try/catch + FailedToEvaluateSentinel fallback.
AnalyzeMemberExpression static-type Func/Action filter	Gone — the new code uses only the runtime-type filter (IsFuncOrActionType(cachedValue?.GetType())) at detail-extraction time, which already preserves the null Func rendering behavior this PR was trying to restore.

Re: the three new review comments from Copilot:

• #3266169598 (value-type writable receivers in CaptureRewriter) — obsoleted; the rewriter class is gone.
• #3266169663 (struct receiver test) — obsoleted for the same reason.
• #3266169703 / #3266169743 / #3266169767 (suggesting [Test] attributes) — not applicable; this project uses TestFramework.ForTestingMSTest's TestContainer, where every public parameterless method is a test (see test/Utilities/TestFramework.ForTestingMSTest/TestContainer.cs).

Rebasing this PR would mean re-writing it from scratch against a fundamentally different design that already addresses the same concerns differently, so the cleaner action is to close.

---

Heads-up unrelated to this PR: while comparing the two architectures I noticed #8307 regressed two display fixes from #8306 (issue #6691):

1. The arbitrary-Get(...)-method special case at HandleMethodCallExpression line 831 now matches any instance method named Get — including box.Get(key) on non-array types — and renders it as box[key]. The original fix scoped this to callExpr.Object.Type.IsArray.
2. The this.Method(...) / Type.Method(...) display rendering (GetMethodCallDisplayName + IsCapturedThis) is no longer present; non-boolean method calls fall back to raw callExpr.ToString() cleanup.

Happy to open a separate PR re-porting those if you'd like — let me know.