Assert.That: fix Expression.Assign double-evaluation and assorted cleanups

[Evangelink]

Fix a single-pass guarantee violation for Expression.Assign (and compound assignments) plus a few related cleanups in Assert.That. Also addresses leftover review feedback from #8308.

Bug: Expression.Assign ran the RHS twice

Empirical repro on main:

static int Counter_Calls;
static int ComputeValue() { Counter_Calls++; return 42; }

// container.Inner.Value = ComputeValue() < 0   (fails)
Assert.That(lambda);
// Counter_Calls == 2   ← bug: ComputeValue ran twice

Root cause

The bottom-up evaluator walked Left and Right, cached the RHS value, then went through the generic "rebuild and compile" path. That replaces children with constants — but the Left of an Assign is a writable target. The rebuilt Assign(Constant, Constant) was invalid, the outer try/catch sentineled it, and the parent expression's later rebuild fell back to invoking the original assignment from scratch — running the RHS side effects a second time.

The same hazard affects every compound assignment (AddAssign, MultiplyAssign, …) and the unary assignment forms (PreIncrementAssign, PostIncrementAssign, …).

Fix

Special-case assignment node types in EvaluateAllSubExpressions:

1. Walk Left to capture diagnostics and cache any side-effecting sub-children.
2. Walk Right exactly once.
3. Rebuild Left via ReplaceSubExpressionsWithConstants so the writable wrapper (MemberExpression / IndexExpression / ParameterExpression) is preserved while its sub-children become constants. This also fixes double-evaluation of side-effecting Left sub-children (e.g., provider.GetBox().Value = 42).
4. Substitute Right with its cached constant and invoke the rebuilt assignment exactly once.

RequiresSinglePassEvaluation now also recognizes assignment node types, so a compound assignment whose only side effect is the assignment itself can't slip into the fast path and double-execute.

Cleanups

• Removed dead IsSimpleExpression / IsVariableReference — IsSimpleExpression only returned true for ConstantExpression, which is handled before it's called. The cached-value lookup branch in GetIndexArgumentDisplay was unreachable. Simplified GetIndexArgumentDisplay and dropped its unused evaluationCache parameter. Removed the orphaned TranslateFailureSentinel helper as well.
• Cached the netstandard/net462 Regex instances for the display-class cleanup and list-init cleanup pipelines (previously a fresh compiled Regex was allocated per failed assertion). On NET, list-init patterns are now GeneratedRegex instead of inline string patterns.
• Reduced substring allocations in RemoveAnonymousTypeWrappers / TryMatchListInitPattern via a new non-allocating HasSubstringAt(input, start, value) helper backed by string.CompareOrdinal + bounds.
• Simplified IsFuncOrActionType to call GetGenericTypeDefinition() at most once.

PR #8308 review feedback

The Copilot AI review left two comments on #8308 that landed after merge:

1. "This new test method is missing a test attribute" — Not actionable: the file uses TestContainer, which treats any public parameterless method as a test (test/Utilities/TestFramework.ForTestingMSTest/TestContainer.cs:6-11). No attribute needed.
2. "...sub-expressions are not object-typed" — Already addressed by the rename to That_TwoCallsReturningSameMutatedReference_DeduplicatesInDetails in the merged version. The test exercises the actual scenario its name describes (reference-identity dedup after mutation) regardless of static type.

Tests

Added three regression tests:

• That_ManuallyConstructedAssign_EvaluatesRhsExactlyOnce — the main bug.
• That_CompoundAssignmentOnField_AppliesExactlyOnceOnFailure — exercises the RequiresSinglePassEvaluation fix for compound assignments without other side-effecting nodes.
• That_AssignToFieldOfSideEffectingReceiver_EvaluatesReceiverExactlyOnce — exercises the ReplaceSubExpressionsWithConstants Left-rebuild fix for side-effecting receivers.

All 1,157 TestFramework.UnitTests pass on net48 / net8.0 / net9.0.

[Copilot]

Pull request overview

This PR fixes Assert.That’s single-pass evaluation guarantee for assignment expressions (Expression.Assign, compound assignments, and unary assignment forms) by special-casing how those nodes are evaluated and rebuilt so side effects aren’t accidentally re-executed during diagnostic extraction. It also includes targeted cleanups in the diagnostic rendering pipeline and adds regression tests to lock in the corrected behavior.

Changes:

• Special-cases assignment expression node types in EvaluateAllSubExpressions and RequiresSinglePassEvaluation to prevent invalid rebuilds and double execution.
• Simplifies index-argument display formatting and removes dead/unreachable helper logic.
• Optimizes diagnostic text cleanup via cached/source-generated regex and reduced substring allocations; adds regression tests for assignment single-evaluation.

Show a summary per file

File	Description
test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs	Adds regression tests covering RHS single-evaluation, compound assignment single-application, and side-effecting receiver evaluation.
src/TestFramework/TestFramework/Assertions/Assert.That.cs	Implements assignment-aware single-pass evaluation and diagnostic cleanup performance improvements.

Copilot's findings

Comments suppressed due to low confidence (1)

src/TestFramework/TestFramework/Assertions/Assert.That.cs:278

• Similarly for unary assignment nodes, EvaluateAllSubExpressions(unaryExpr.Operand, cache) evaluates the operand expression (including property/indexer getters) and then the rebuilt Pre/PostIncrementAssign will evaluate it again as part of its semantics. This can duplicate side effects on writable operands. A dedicated walk that evaluates only receiver/index-argument subexpressions (but not the writable wrapper itself) would avoid double getter execution.

                case UnaryExpression unaryExpr when IsAssignmentUnaryNodeType(unaryExpr.NodeType):
                    EvaluateAllSubExpressions(unaryExpr.Operand, cache);
                    Expression rebuiltUnaryOperand = ReplaceSubExpressionsWithConstants(unaryExpr.Operand, cache);
                    UnaryExpression rebuiltUnary = unaryExpr.Update(rebuiltUnaryOperand);
                    cache[unaryExpr] = Expression.Lambda(rebuiltUnary).Compile().DynamicInvoke();
                    return;

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

[Evangelink]

#	Dimension	Verdict
13	Test Completeness	🔴 BLOCKING — 3 tests missing [TestMethod]
10	Test Isolation	🟡 MAJOR — static mutable CountingComputeValue.Calls

✅ 19/21 dimensions clean.

• Test Completeness (#13) — That_ManuallyConstructedAssign_EvaluatesRhsExactlyOnce, That_CompoundAssignmentOnField_AppliesExactlyOnceOnFailure, and That_AssignToFieldOfSideEffectingReceiver_EvaluatesReceiverExactlyOnce all lack [TestMethod]. MSTest will never discover them; the three regression scenarios they document are completely unguarded in CI.
• Test Isolation (#10) — CountingComputeValue.Calls is a non-atomic static mutable field. Reset (= 0) and assertion (Should().Be(1)) are not atomic under parallel execution. Prefer an instance-based counter or Interlocked operations.

The production-side changes (assignment special-casing in EvaluateAllSubExpressions, RequiresSinglePassEvaluation, HasSubstringAt, regex caching) look correct and well-reasoned.

Generated by Expert Code Review (on open) for issue #8363 · ● 19.5M