Add MSTEST0064 to prefer async assertions#8256
Conversation
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds MSTEST0064 to detect MSTest synchronous exception assertions that block on async Task work and provide a C# code fix to use async assertion APIs.
Changes:
- Adds
PreferAsyncAssertionAnalyzerand registers MSTEST0064. - Adds
PreferAsyncAssertionFixerto rewrite assertions and async test signatures. - Adds resources, localization entries, release tracking, and analyzer/code-fix tests.
Show a summary per file
| File | Description |
|---|---|
src/Analyzers/MSTest.Analyzers/PreferAsyncAssertionAnalyzer.cs |
Adds MSTEST0064 analyzer logic. |
src/Analyzers/MSTest.Analyzers.CodeFixes/PreferAsyncAssertionFixer.cs |
Adds C# code fix for async assertions. |
src/Analyzers/MSTest.Analyzers/Helpers/DiagnosticIds.cs |
Registers MSTEST0064 ID. |
src/Analyzers/MSTest.Analyzers/AnalyzerReleases.Unshipped.md |
Adds release tracking entry. |
src/Analyzers/MSTest.Analyzers/Resources.resx |
Adds analyzer resource strings. |
src/Analyzers/MSTest.Analyzers.CodeFixes/CodeFixResources.resx |
Adds code-fix title resource. |
test/UnitTests/MSTest.Analyzers.UnitTests/PreferAsyncAssertionAnalyzerTests.cs |
Adds focused analyzer/code-fix tests. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.cs.xlf |
Adds localized analyzer resource placeholders. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.de.xlf |
Adds localized analyzer resource placeholders. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.es.xlf |
Adds localized analyzer resource placeholders. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlf |
Adds localized analyzer resource placeholders. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.it.xlf |
Adds localized analyzer resource placeholders. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.ja.xlf |
Adds localized analyzer resource placeholders. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.ko.xlf |
Adds localized analyzer resource placeholders. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.pl.xlf |
Adds localized analyzer resource placeholders. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.pt-BR.xlf |
Adds localized analyzer resource placeholders. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.ru.xlf |
Adds localized analyzer resource placeholders. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.tr.xlf |
Adds localized analyzer resource placeholders. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hans.xlf |
Adds localized analyzer resource placeholders. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hant.xlf |
Adds localized analyzer resource placeholders. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.cs.xlf |
Adds localized code-fix resource placeholder. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.de.xlf |
Adds localized code-fix resource placeholder. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.es.xlf |
Adds localized code-fix resource placeholder. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.fr.xlf |
Adds localized code-fix resource placeholder. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.it.xlf |
Adds localized code-fix resource placeholder. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.ja.xlf |
Adds localized code-fix resource placeholder. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.ko.xlf |
Adds localized code-fix resource placeholder. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.pl.xlf |
Adds localized code-fix resource placeholder. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.pt-BR.xlf |
Adds localized code-fix resource placeholder. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.ru.xlf |
Adds localized code-fix resource placeholder. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.tr.xlf |
Adds localized code-fix resource placeholder. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.zh-Hans.xlf |
Adds localized code-fix resource placeholder. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.zh-Hant.xlf |
Adds localized code-fix resource placeholder. |
Copilot's findings
Comments suppressed due to low confidence (5)
src/Analyzers/MSTest.Analyzers.CodeFixes/PreferAsyncAssertionFixer.cs:90
- The code fix does not handle static-imported assertions such as
using static ...Assert; Throws<Exception>(...). The analyzer still reports those invocations, but this method returns the originalThrowsname, so the fix wrapsawaitaround the synchronous assertion instead of callingThrowsAsync, which can break compilation after the lambda is rewritten to return aTask.
if (invocationExpression.Expression is not MemberAccessExpressionSyntax memberAccessExpression)
{
return invocationExpression;
}
src/Analyzers/MSTest.Analyzers.CodeFixes/PreferAsyncAssertionFixer.cs:140
- Changing a
voidtest method to return unqualifiedTaskrequiresSystem.Threading.Tasksto already be in scope. If the original test uses fully qualified task types or implicit usings are disabled, applying the fix introduces an unresolvedTask; the fixer should add the using or use a qualified/simplified type.
if (newMethodDeclaration.ReturnType.IsVoid())
{
newMethodDeclaration = newMethodDeclaration.WithReturnType(SyntaxFactory.IdentifierName("Task").WithTriviaFrom(newMethodDeclaration.ReturnType));
}
src/Analyzers/MSTest.Analyzers.CodeFixes/PreferAsyncAssertionFixer.cs:160
- The analyzer accepts block-bodied lambdas whose single operation is a return value, but the fixer only rewrites block bodies with an expression statement. For
Task<T>cases like() => { return BarAsync().GetAwaiter().GetResult(); }, the fix renames toThrowsAsyncand addsawaitwhile leaving a lambda that returns the synchronous result, causing the fixed code not to compile.
if (lambdaExpression.Body is BlockSyntax blockSyntax &&
blockSyntax.Statements.Count == 1 &&
blockSyntax.Statements[0] is ExpressionStatementSyntax expressionStatement)
src/Analyzers/MSTest.Analyzers/PreferAsyncAssertionAnalyzer.cs:127
- This treats every non-return child operation as meaningful, but VB lambdas can include implicit
ILabeledOperationnodes in the block. Because those labels are not skipped, otherwise valid VB cases such as a singleGetAwaiter().GetResult()statement can look like multiple operations and fail to produce the diagnostic despite the analyzer being registered for Visual Basic.
IOperation? candidateOperation = childOperation switch
{
IExpressionStatementOperation expressionStatementOperation => expressionStatementOperation.Operation,
IReturnOperation { ReturnedValue: { } returnedValue } => returnedValue,
IReturnOperation { IsImplicit: true } => null,
_ => childOperation,
src/Analyzers/MSTest.Analyzers.CodeFixes/PreferAsyncAssertionFixer.cs:111
- The analyzer unwraps conversions/delegate creation when identifying the lambda, but the fixer only handles a direct lambda syntax node. If the diagnosed action is explicitly cast (for example
(Action)(() => ...)), the fix still renames to the async assertion and addsawaitwhile leaving the blocking casted lambda unchanged, which can make the fixed call fail overload resolution.
if (argument.Expression is not LambdaExpressionSyntax lambdaExpression ||
!TryGetBlockedTaskExpressionFromLambda(lambdaExpression, out ExpressionSyntax? asyncExpression))
- Files reviewed: 33/33 changed files
- Comments generated: 2
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…favorizing-async-assertions
|
|
||
| if (newMethodDeclaration.ReturnType.IsVoid()) | ||
| { | ||
| newMethodDeclaration = newMethodDeclaration.WithReturnType(SyntaxFactory.IdentifierName("Task").WithTriviaFrom(newMethodDeclaration.ReturnType)); |
There was a problem hiding this comment.
Addressed in b71536c (in this push): when the test method is void the fixer now uses GetSpeculativeTypeInfo to decide between unqualified Task and fully-qualified System.Threading.Tasks.Task (see GetTaskReturnType).
| if (lambdaExpression.Body is BlockSyntax blockSyntax && | ||
| blockSyntax.Statements.Count == 1 && | ||
| blockSyntax.Statements[0] is ExpressionStatementSyntax expressionStatement) | ||
| { | ||
| return TryGetBlockedTaskExpression(expressionStatement.Expression, out asyncExpression); |
There was a problem hiding this comment.
Addressed in b71536c: TryGetBlockedTaskExpressionFromLambda now also handles a BlockSyntax whose single statement is a ReturnStatementSyntax with an expression.
| private static InvocationExpressionSyntax ReplaceAssertMethodName(InvocationExpressionSyntax invocationExpression) | ||
| { | ||
| if (invocationExpression.Expression is not MemberAccessExpressionSyntax memberAccessExpression) | ||
| { | ||
| return invocationExpression; |
There was a problem hiding this comment.
Addressed in b71536c: ReplaceAssertMethodName now handles both MemberAccessExpressionSyntax and bare SimpleNameSyntax (using static) invocations; WhenStaticallyImportedAssertion_CodeFixUsesAsyncAssertion covers the case.
| if (!newMethodDeclaration.Modifiers.Any(modifier => modifier.IsKind(SyntaxKind.AsyncKeyword))) | ||
| { | ||
| newMethodDeclaration = newMethodDeclaration.WithModifiers(newMethodDeclaration.Modifiers.Add(SyntaxFactory.Token(SyntaxKind.AsyncKeyword))); | ||
| } |
There was a problem hiding this comment.
Addressed in b71536c: when the containing method is already non-async Task-returning, TaskReturnStatementRewriter rewrites any return X; to await X; return; so the body remains valid after we add the async modifier. WhenNonAsyncTaskReturningTestMethodHasReturnExpression_CodeFixConvertsReturnToAwait covers the case.
| AwaitExpressionSyntax awaitExpression = SyntaxFactory.AwaitExpression(newInvocationExpression.WithoutLeadingTrivia()) | ||
| .WithLeadingTrivia(invocationExpression.GetLeadingTrivia()) | ||
| .WithAdditionalAnnotations(Formatter.Annotation); |
There was a problem hiding this comment.
Addressed in b71536c at the analyzer level: IsInsideUnsupportedAwaitContext now also stops on OperationKind.Lock so the diagnostic is no longer raised inside a lock block. WhenAssertionIsInsideLockStatement_NoDiagnostic covers the case.
| { | ||
| IExpressionStatementOperation expressionStatementOperation => expressionStatementOperation.Operation, | ||
| IReturnOperation { ReturnedValue: { } returnedValue } => returnedValue, | ||
| IReturnOperation { IsImplicit: true } => null, |
There was a problem hiding this comment.
Addressed in 190da15: the initial attempt to address this in b71536c by unconditionally skipping implicit operations was too aggressive — it silently dropped every C# expression-bodied-lambda diagnostic, because the synthetic IReturnOperation that wraps an expression-body's user expression is itself implicit. The helper now only filters out the specific VB Function synthetic implicit return (detected as an implicit ILocalReferenceOperation after walking down conversions on ReturnedValue) and still accepts C# implicit returns whose value is a real user expression. WhenVisualBasicFunctionLambdaBlocksOnGenericTask_Diagnostic covers the VB case; the previously-failing C# tests are green again.
|
@copilot address review comments |
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
…urns The previous skip-all-implicit guard for VB was over-broad: in C# the implicit IReturnOperation generated for an expression-bodied lambda wraps the user's expression as its ReturnedValue, so skipping it silently dropped every expression-bodied-lambda diagnostic (e.g. `() => BarAsync().GetAwaiter().GetResult()`). Only the VB Function lambda's synthetic implicit return-of-function-name-local needs to be filtered out, which we now detect by looking at the unwrapped ReturnedValue for an implicit ILocalReferenceOperation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
src/Analyzers/MSTest.Analyzers.CodeFixes/PreferAsyncAssertionFixer.cs:175
- This only rewrites returns for methods whose original return type is
Task. MSTest also treatsValueTasktest methods as valid, so applying this fix to a non-asyncValueTasktest method leavesreturn ValueTask...;statements in anasync ValueTaskmethod, which does not compile.
else if (!isAsync && IsTaskReturnType(originalMethodDeclaration, semanticModel, cancellationToken) && newMethodDeclaration.Body is { } body)
{
newMethodDeclaration = newMethodDeclaration.WithBody((BlockSyntax)new TaskReturnStatementRewriter().Visit(body)!);
src/Analyzers/MSTest.Analyzers.CodeFixes/PreferAsyncAssertionFixer.cs:285
- The rewriter converts every return expression in the method body into an
awaitfollowed byreturn. If a different return statement is inside alockblock, the fix emitsawaitinsidelock, which C# forbids even though the diagnostic assertion itself is outside the lock.
if (statement is ReturnStatementSyntax { Expression: { } returnExpression } returnStatement)
{
rewrittenStatements ??= AddUnchangedStatements(node.Statements, i);
rewrittenStatements.AddRange(CreateAwaitAndReturnStatements(returnStatement, returnExpression));
- Files reviewed: 46/46 changed files
- Comments generated: 3
| if (argument.Expression is not LambdaExpressionSyntax lambdaExpression || | ||
| !TryGetBlockedTaskExpressionFromLambda(lambdaExpression, out ExpressionSyntax? asyncExpression)) | ||
| { | ||
| newArgument = null; | ||
| return false; | ||
| } | ||
|
|
||
| LambdaExpressionSyntax newLambdaExpression = lambdaExpression switch | ||
| { | ||
| SimpleLambdaExpressionSyntax simpleLambda => simpleLambda.WithBody(asyncExpression.WithTriviaFrom(lambdaExpression.Body)), | ||
| ParenthesizedLambdaExpressionSyntax parenthesizedLambda => parenthesizedLambda.WithBody(asyncExpression.WithTriviaFrom(lambdaExpression.Body)), | ||
| _ => lambdaExpression, | ||
| }; | ||
|
|
||
| newArgument = argument.WithExpression(newLambdaExpression); | ||
| return true; | ||
| } | ||
|
|
| for (IOperation? current = operation.Parent; current is not null; current = current.Parent) | ||
| { | ||
| if (current is IAnonymousFunctionOperation or ILocalFunctionOperation || | ||
| current.Kind == OperationKind.Lock) |
| <trans-unit id="AreEquivalentFailedSummary"> | ||
| <source>Expected values to be structurally equivalent.</source> | ||
| <target state="new">Expected values to be structurally equivalent.</target> | ||
| <note /> | ||
| </trans-unit> |
New Feature
What does this feature do?
Adds an MSTest analyzer/code fix that flags synchronous exception assertions blocking on async work and recommends async assertion APIs instead.
becomes:
Implementation details
Analyzer
MSTEST0064forAssert.Throws/Assert.ThrowsExactlycalls that blockTask/Task<T>viaGetAwaiter().GetResult().Code fix
Assert.ThrowsAsync/Assert.ThrowsExactlyAsync.GetAwaiter().GetResult()chain from the asserted lambda.voidtest methods toasync Taskwhen required.Resources and coverage
Task,Task<T>, and non-diagnostic cases.