Fix Assert.That to not evaluate expression twice (re-opens #6699)

[Evangelink]

Re-opens PR #6699 (which was closed without merging).

Fixes Assert.That to not evaluate the lambda body twice for the assert and for the message. Concretely, Assert.That(() => box.GetValueWithSideEffect() != 0) used to invoke GetValueWithSideEffect() once to evaluate the condition, then re-evaluate sub-expressions a second time to build the diagnostic message - producing wrong/confusing values for side-effecting operands. See #6690.

What's in this branch

• New EvaluateExpression / EvaluateAllSubExpressions pair that walks the expression tree bottom-up, caches each sub-expression value, and lets ExtractDetails read from the cache instead of recompiling and re-invoking.
• New AssertThatFailedToEvaluate resource (<Failed to evaluate>) for sub-expressions that the cache couldn't resolve.
• Companion tests in AssertTests.That.cs.

Merge notes

This branch was rebased on top of latest main (commit 6b0bae4). Resolved conflicts:

• Assert.That.cs: kept the new EvaluateExpression / cache logic from this branch; adopted main's more efficient StringBuilder.Append(char, int) overload while preserving the MaxConsecutiveParentheses named constant from this branch.
• FrameworkMessages.resx: kept AssertThatFailedToEvaluate from this branch alongside all the new RFC 012 summary entries from main; xlf files regenerated via dotnet msbuild /t:UpdateXlf.
• Guarded [RequiresDynamicCode(...)] attributes with #if NET7_0_OR_GREATER (the attribute is not available on netstandard2.0 / net462 targets and the branch was previously failing to build on those).

Validation

• dotnet build src/TestFramework/TestFramework/TestFramework.csproj -c Debug -> clean across all 4 target frameworks (netstandard2.0, net462, net8.0, net9.0).
• dotnet test test/UnitTests/TestFramework.UnitTests/TestFramework.UnitTests.csproj -- --treenode-filter "/*/*/AssertTests/That*" -> 256/256 passed across net8.0, net8.0-windows10.0.18362.0, net9.0, net48.

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

[Copilot]

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

[Evangelink]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Resolved by merge commit 9889353 on this branch. I merged origin/main, resolved the conflicted files, and pushed the updated branch state.

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (7)

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

• This evaluates both operands for every binary expression before evaluating the parent, which changes C# short-circuit semantics for &&, ||, and ??. For example, Assert.That(() => false && Throw()) used to fail the assertion without invoking the right side, but this traversal evaluates the right side while building the cache and can invoke side effects or throw before the assertion result is computed.

                case BinaryExpression binaryExpr:
                    // Evaluate both operands before evaluating the binary operation
                    EvaluateAllSubExpressions(binaryExpr.Left, cache);
                    EvaluateAllSubExpressions(binaryExpr.Right, cache);
                    hasChildren = true;

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

• Conditional expressions are evaluated by visiting both branches up front. That does not match runtime ternary/null-conditional semantics, where only the selected branch is evaluated, so assertions such as flag ? true : Throw() can execute code that should be unreachable and fail before the actual condition result is known.

                case ConditionalExpression conditionalExpr:
                    // For ternary expressions, evaluate all three parts
                    EvaluateAllSubExpressions(conditionalExpr.Test, cache);
                    EvaluateAllSubExpressions(conditionalExpr.IfTrue, cache);
                    EvaluateAllSubExpressions(conditionalExpr.IfFalse, cache);
                    hasChildren = true;

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

• This catch also runs for exceptions thrown while evaluating the assertion condition itself. Previously condition.Compile()() propagated those user exceptions; now the root expression can be cached as the "<Failed to evaluate>" string and EvaluateExpression later casts it to bool, producing an unrelated InvalidCastException instead of the original exception.

        catch
        {
            // If evaluation fails (e.g., null reference, division by zero), mark it as failed
            // rather than throwing. This allows us to continue and provide diagnostic information.
            cache[expr] = FrameworkMessages.AssertThatFailedToEvaluate;

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

• Constructor arguments are evaluated and cached here, but ReplaceSubExpressionsWithConstants does not rebuild NewExpression, so the later DynamicInvoke evaluates the original constructor expression and its arguments again. This leaves expressions like Assert.That(() => new Foo(box.Next()).Value == 0) still invoking side-effecting arguments twice.

                case NewExpression newExpr:
                    // For object creation, evaluate all constructor arguments
                    foreach (Expression argument in newExpr.Arguments)
                    {
                        EvaluateAllSubExpressions(argument, cache);
                        hasChildren = true;
                    }

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

• Array element expressions are evaluated and cached here, but new-array expressions are not rebuilt with cached constants before the parent is invoked. A failing assertion over new[] { box.Next() } will still call Next() once during this traversal and again when the original NewArrayExpression is compiled.

                case NewArrayExpression newArrayExpr:
                    // For array creation, evaluate all element expressions
                    foreach (Expression expression in newArrayExpr.Expressions)
                    {
                        EvaluateAllSubExpressions(expression, cache);
                        hasChildren = true;
                    }

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

• Collection initializer arguments are evaluated here, but ReplaceSubExpressionsWithConstants has no ListInitExpression case, so evaluating the initializer later uses the original Add(...) arguments and re-invokes any side-effecting expressions. This means the fix does not cover assertions involving collection initializers.

                case ListInitExpression listInitExpr:
                    // For collection initializers, evaluate the new expression and all initializer arguments
                    EvaluateAllSubExpressions(listInitExpr.NewExpression, cache);
                    foreach (ElementInit initializer in listInitExpr.Initializers)
                    {
                        foreach (Expression argument in initializer.Arguments)
                        {
                            EvaluateAllSubExpressions(argument, cache);
                        }
                    }

                    hasChildren = true;

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

• Invocation arguments are evaluated during the cache walk, but invocation expressions are not rebuilt with cached constants before DynamicInvoke. A predicate call such as Assert.That(() => predicate(box.Next())) will therefore evaluate box.Next() during argument traversal and then again when the original invocation expression is executed.

                case InvocationExpression invocationExpr:
                    // For delegate invocations, evaluate the delegate and all arguments
                    EvaluateAllSubExpressions(invocationExpr.Expression, cache);
                    foreach (Expression argument in invocationExpr.Arguments)
                    {
                        EvaluateAllSubExpressions(argument, cache);
                    }

                    hasChildren = true;

• Files reviewed: 17/17 changed files
• Comments generated: 3

[Evangelink]

@copilot address review comments

[Evangelink]

Pushed 5775de2 to fix the test failure from the latest CI run:

That_FieldAccessWithSideEffectingParent_DoesNotEvaluateTwice expected only provider.GetBox().Value = 1 in the failure details, but the actual output also included provider.GetBox() = <ValueBox> for the intermediate method call. AddMemberExpressionToDetails already skipped extracting child variables when the receiver is a MemberExpression (to avoid showing both person and person.Name); extended the same exclusion to MethodCallExpression so we don't surface the intermediate side-effecting receiver as a separate detail entry when it's part of a longer member chain.

All 66 Assert.That unit tests pass locally on net9.0.

[Copilot]

Copilot's findings

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

[Evangelink]

@copilot resolve the merge conflicts in this pull request

[Evangelink]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Resolved in merge commit 1d6d948: I merged origin/main, fixed conflicts in Assert.That.cs and AssertTests.That.cs, and pushed the updated branch.

[Copilot]

@copilot resolve the merge conflicts in this pull request

The merge commit 1d6d948 successfully resolved all conflicts. However, I notice that PR #8306 (commit 29545ad) has already merged a solution for the same Assert.That issue (#6690) to main.

This branch now contains a different implementation approach than what's in main. If you'd like to close this PR in favor of the already-merged solution, or if you'd prefer to keep working on this branch with a different approach, please let me know how you'd like to proceed.