MSTEST0064: gate code fix against await-forbidden returns and unwrap explicit delegate creations (#8256 follow-up)#8329
Conversation
Follow-up to #8256. Addresses two issues raised in PR review: 1. The fixer's AwaitableReturnStatementRewriter would emit `await` inside `lock`, `unsafe`, and `fixed` blocks when a non-async `Task`/`ValueTask`-returning test method had `return X;` inside such a block, producing CS1996 (Cannot await in body of lock statement) and equivalent invalid code. The fixer now scans the containing method for return-with-expression statements inside await-forbidden blocks and skips offering the code fix when one would produce invalid code. The diagnostic is still reported so the user can manually convert the test method. 2. The analyzer reported diagnostics on explicit delegate creations such as `Assert.Throws<E>(new Action(() => task.GetAwaiter().GetResult()))`, but the fixer left the `new Action(...)` wrapper in place while renaming to `ThrowsAsync`. Since `ThrowsAsync` takes `Func<Task>`, the resulting code did not compile. The fixer now unwraps `ObjectCreationExpressionSyntax` arguments with a single lambda or anonymous-method argument and transforms the inner expression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR is a follow-up to MSTEST0064 (“prefer async assertions”) that hardens the code fix to avoid producing uncompilable code, and extends the fixer to handle an additional syntax form seen in real-world code.
Changes:
- Skip registering the MSTEST0064 code fix when it would require emitting
awaitinsidelock/unsafe/fixedblocks for non-asyncTask/ValueTasktest methods. - Extend the fixer to unwrap explicit delegate creations (e.g.,
new Action(...)) so the transformed call correctly matches*Asyncassertion signatures. - Add regression tests covering the above scenarios (delegate creation + no-fix-in-unawaitable-context).
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/MSTest.Analyzers.UnitTests/PreferAsyncAssertionAnalyzerTests.cs | Adds regression tests for explicit delegate creation unwrapping and for “diagnostic but no code fix” when returns appear in await-forbidden contexts. |
| src/Analyzers/MSTest.Analyzers.CodeFixes/PreferAsyncAssertionFixer.cs | Gates code-fix registration when return rewriting would force await into lock/unsafe/fixed contexts; unwraps new Action(...)-style arguments during transformation. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
Evangelink
left a comment
There was a problem hiding this comment.
| # | Dimension | Verdict |
|---|---|---|
| 13 | Test Completeness | 🟡 1 MODERATE — fixed block path untested |
| 18 | Analyzer & Code Fix Quality | 🟡 1 MODERATE — target-typed new() not handled |
✅ 19/21 dimensions clean.
- Analyzer Quality —
ImplicitObjectCreationExpressionSyntax(new()) not covered by the ObjectCreation unwrapping branch - Test Completeness —
VisitFixedStatementhas no test;lockandunsafeare covered butfixedis not
Overall: The two new features — suppressing the code fix when a non-async Task/ValueTask method returns inside a lock/unsafe/fixed block, and unwrapping new Action(...) delegate wrappers — are logically sound and the UnawaitableReturnDetector walker is correctly implemented. The guard conditions are consistent with the existing AwaitableReturnStatementRewriter scope. The two findings above are coverage gaps, not correctness regressions.
Generated by Expert Code Review (on open) for issue #8329 · ● 6M
|
@copilot address review comments |
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
|
@copilot fix test/UnitTests/MSTest.Analyzers.UnitTests/Verifiers/CSharpCodeFixVerifier`2.cs#L92 test/UnitTests/MSTest.Analyzers.UnitTests/Verifiers/CSharpCodeFixVerifier`2.cs(92,1): error : WhenAssertionActionIsTargetTypedDelegateCreation_CodeFixUnwrapsDelegateCreation [net8.0] Diagnostics: @azure-pipelines test/UnitTests/MSTest.Analyzers.UnitTests/PreferAsyncAssertionAnalyzerTests.cs(1067,1): error : WhenNonAsyncTaskMethodHasReturnInsideFixedBlock_DiagnosticReportedButNoCodeFixOffered [net8.0] Diagnostics: |
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Fixed in 1f9479f. I updated the target-typed |
|
@copilot fix test/UnitTests/MSTest.Analyzers.UnitTests/Verifiers/CSharpCodeFixVerifier`2.cs#L92 test/UnitTests/MSTest.Analyzers.UnitTests/Verifiers/CSharpCodeFixVerifier`2.cs(92,1): error : WhenAssertionActionIsTargetTypedDelegateCreation_CodeFixUnwrapsDelegateCreation [net8.0] Expected diagnostic: Actual diagnostic: |
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Fixed in 520976a. I changed the target-typed delegate creation test input to |
Follow-up to #8256. Addresses two issues raised in PR review that survived to the merged code.
What does this PR do?
1.
AwaitableReturnStatementRewriterno longer emitsawaitinside await-forbidden contextsFor a non-async
Task/ValueTask-returning test method, the fixer rewrites everyreturn X;intoawait X; return;. The rewriter already skipped lambdas, anonymous methods and local functions, but notlock/unsafe/fixedblocks. As a result, code like:would be rewritten to
await Task.CompletedTask; return;inside thelock, producingCS1996 Cannot await in the body of a lock statement.The fixer now walks the containing method (respecting the same boundaries as the rewriter) and, when a non-async
Task/ValueTasktest method has anyreturn X;inside alock,unsafe, orfixedblock, it skips registering the code fix. The diagnostic is still reported so the user is aware and can convert manually. (Originally flagged in the low-confidence reviewer feedback on commit190da15e.)2. Code fix now unwraps explicit delegate creations (
new Action(...))The analyzer reports
Assert.Throws<E>(new Action(() => task.GetAwaiter().GetResult()))because Roslyn represents the wrapper as anIDelegateCreationOperationwhose target is the inner anonymous function. The fixer only handled lambda / parenthesized / cast / anonymous-method syntactic arguments, so it would rename toThrowsAsyncwhile leavingnew Action(...)in place — butThrowsAsynctakesFunc<Task>, so the result didn't compile.TryReplaceActionExpressionnow also unwrapsObjectCreationExpressionSyntaxarguments with a single lambda or anonymous-method argument and applies the same transformation to the inner expression.Tests
Added 4 regression tests in
PreferAsyncAssertionAnalyzerTests:WhenAssertionActionIsExplicitDelegateCreation_CodeFixUnwrapsDelegateCreationWhenAssertionActionIsExplicitDelegateCreationWithAnonymousMethod_CodeFixUnwrapsDelegateCreationWhenNonAsyncTaskMethodHasReturnInsideLockBlock_DiagnosticReportedButNoCodeFixOfferedWhenNonAsyncValueTaskMethodHasReturnInsideUnsafeBlock_DiagnosticReportedButNoCodeFixOfferedAll 2152
MSTest.Analyzers.UnitTests(net8.0 + net472) pass locally.Built locally with
.\build.cmd -c Release(0 warnings, 0 errors).