perf: improve Assertion.ExecuteCore state machine generation

[TimothyMakkison]

Improve Assertion.ExecuteCoreAsync state machine generation by moving logic into external methods to reduce the number of fields in the state machine.

• Not destructuring the tuple saves another 16 bytes, I don't know why
• I have absolutely no idea why .net generates its state machines this way, the amount of duplicate fields is weird (there are still 2 ValueTuple<TValue, Exception>) fields despite my best attempts
• Not sure about my naming of functions and variables.
• ExecuteCoreHandleException will change the thrown exception stacktrace, although I don't think this affects the user.

Before

After

[thomhurst]

Summary

Performance optimization that reduces state machine size for Assertion.ExecuteCoreAsync by extracting logic into separate methods.

Critical Issues

Syntax typo: Line 141 has a double semicolon:

var result = await CreateMetadataAndCheckAsync(contextResult.Value, contextResult.Exception);;

Should be a single semicolon.

Suggestions

None - the refactoring approach is sound. The stack trace concern mentioned in the PR description is a non-issue since throw without an argument preserves the original stack trace.

TUnit Rules Check

• ✅ Hot path performance optimization (minimal allocations, reduced state machine size)
• ✅ No public API changes (private methods only)
• ✅ No dual-mode concerns (TUnit.Assertions only)
• ✅ No snapshot updates needed
• ✅ Modern C# async patterns followed

Verdict

⚠️ REQUEST CHANGES - Fix the double semicolon typo on line 141

[TimothyMakkison]

var result = await CreateMetadataAndCheckAsync(contextResult.Value, contextResult.Exception);;

Fixed

[thomhurst]

Summary

Refactors Assertion.ExecuteCoreAsync to reduce state machine size by extracting metadata creation and exception handling into separate methods, reducing state machine fields by ~48 bytes.

Critical Issues

Syntax Error - TUnit.Assertions/Core/Assertion.cs:141

var result = await CreateMetadataAndCheckAsync(contextResult.Value, contextResult.Exception);;

Double semicolon will cause compilation failure. Remove one semicolon.

Suggestions

Stacktrace Impact - TUnit.Assertions/Core/Assertion.cs:149
The author notes that ExecuteCoreHandleException may change thrown exception stacktraces. While this is likely acceptable since:

• The exception is created in CreateException() which captures the proper location
• The additional stack frame is just the helper method
• Users see the assertion location from Context.ExpressionBuilder anyway

Consider verifying with a test that exception messages/stacks remain useful for debugging.

Method Naming - TUnit.Assertions/Core/Assertion.cs:153
ExecuteCoreHandleException doesn't handle exceptions—it throws or accumulates them. Consider ThrowOrAccumulateFailure or ProcessFailedAssertion for clarity.

Previous Review Status

Unable to fetch previous comments due to API permissions.

Verdict

⚠️ REQUEST CHANGES - Syntax error must be fixed before merge

[TimothyMakkison]

Fixed

Forgot to force push mb, renamed to ThrowOrAccumulateFailure

Before I forget this optimisation could be applied to the following in a future PR

if (Context.PendingPreWork != null)
{
    var preWork = Context.PendingPreWork;
    Context.PendingPreWork = null; // Clear BEFORE execution to prevent re-entry
    await preWork();
}

// If this is an And/OrAssertion (composite), delegate to AssertAsync which has custom logic
if (this is Chaining.AndAssertion<TValue> or Chaining.OrAssertion<TValue>)
{
    return await AssertAsync();
}

[thomhurst]

Summary

Optimizes async state machine generation for ExecuteCoreAsync by extracting logic into separate methods to reduce state machine field count.

Critical Issues

None found ✅

Suggestions

1. Minor Inaccuracy in PR Description
The PR description mentions "ExecuteCoreHandleException will change the thrown exception stacktrace" but the method is actually named ThrowOrAccumulateFailure and it doesn't alter stack traces (it calls throw directly with the original exception).

2. Consider Adding a Comment
Given this is a performance optimization with non-obvious benefits, consider adding a brief comment above CreateMetadataAndCheckAsync:

// Extracted to separate method to reduce async state machine size
// by avoiding DateTimeOffset fields in the main state machine
private Task<AssertionResult> CreateMetadataAndCheckAsync(...)

TUnit Rules Compliance

• ✅ Dual-Mode: Not applicable (assertions library, not discovery/metadata)
• ✅ Snapshot Testing: Not required (no public API changes - new methods are private)
• ✅ Performance First: This IS a hot path optimization (test execution) - aligns with Rule 4
• ✅ AOT Compatible: No reflection changes
• ✅ No VSTest: Not applicable

Performance Analysis

The refactoring extracts metadata creation and exception handling into separate scopes to prevent the C# compiler from hoisting fields into the main async state machine. This is a valid micro-optimization for a hot execution path (every assertion invocation). The tuple non-destructuring optimization is also valid - the compiler can sometimes generate more efficient code when avoiding tuple pattern matching.

Verdict

✅ APPROVE - Valid performance optimization with no critical issues. The code is functionally equivalent and correctly optimizes state machine generation for this hot path.