Add analyzer for redundant async state machines - #1666
Add analyzer for redundant async state machines#1666Andrew Arnott (AArnott) wants to merge 6 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new C# analyzer + code fix pair (VSTHRD202) that flags conservative cases of redundant async state machines where a method’s sole terminal await can be replaced by directly returning the underlying Task/Task<T>, along with documentation and tests to validate supported and excluded patterns.
Changes:
- Introduces
VSTHRD202RemoveUnnecessaryAsyncAnalyzerto detect single terminal-awaitmethods returning the exact sameTask/Task<T>(with optional.ConfigureAwait(bool)unwrapping). - Adds
VSTHRD202RemoveUnnecessaryAsyncCodeFixoffering a minimal fix and an exception-wrapping fix whenTask.FromExceptionis available. - Adds docfx documentation/TOC/index entries, localized strings, and analyzer/code-fix tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD202RemoveUnnecessaryAsyncAnalyzerTests.cs | Verifies diagnostics/fixes and ensures unsupported patterns are excluded. |
| src/Microsoft.VisualStudio.Threading.Analyzers/Strings.resx | Adds localized title/message and code-fix titles for VSTHRD202. |
| src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/VSTHRD202RemoveUnnecessaryAsyncAnalyzer.cs | Implements the VSTHRD202 analyzer logic and conservative exclusions. |
| src/Microsoft.VisualStudio.Threading.Analyzers.CodeFixes/VSTHRD202RemoveUnnecessaryAsyncCodeFix.cs | Implements ordered code fixes (minimal + exception-wrapping). |
| docfx/analyzers/VSTHRD202.md | Documents what the analyzer flags, available fixes, and behavioral caveats. |
| docfx/analyzers/toc.yml | Adds VSTHRD202 to the analyzers TOC. |
| docfx/analyzers/index.md | Adds VSTHRD202 to the analyzer index table. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/VSTHRD202RemoveUnnecessaryAsyncAnalyzer.cs:64
- The analyzer repeatedly re-traverses the method body via
method.DescendantNodes(ShouldDescendInto)(for awaits, using declarations, await-using, await-foreach). In Roslyn analyzers this can add noticeable overhead; cache the descendant nodes once and query that collection instead.
This issue also appears on line 119 of the same file.
SyntaxToken asyncKeyword = method.Modifiers.FirstOrDefault(modifier => modifier.IsKind(SyntaxKind.AsyncKeyword));
ImmutableArray<AwaitExpressionSyntax> awaitExpressions = method.DescendantNodes(ShouldDescendInto).OfType<AwaitExpressionSyntax>().ToImmutableArray();
if (asyncKeyword.RawKind == 0
|| awaitExpressions is not [AwaitExpressionSyntax awaitExpression]
|| context.SemanticModel.GetDeclaredSymbol(method, context.CancellationToken) is not IMethodSymbol { IsAsync: true } methodSymbol
src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/VSTHRD202RemoveUnnecessaryAsyncAnalyzer.cs:123
IsTaskConfigureAwaitmatches by method name/signature andUtils.IsTask(...). Elsewhere in this codebase, ConfigureAwait-like methods are matched viaCommonInterest.TaskConfigureAwait+QualifiedMember.IsMatch(e.g. CommonInterest.cs:69-72; VSTHRD003UseJtfRunAsyncAnalyzer.cs:245). Using that shared symbol matcher here reduces the risk of false positives/negatives if additional ConfigureAwait-like members are added or type names collide.
private static bool IsTaskConfigureAwait(IInvocationOperation invocation)
=> invocation.TargetMethod.Name == nameof(Task.ConfigureAwait)
&& invocation.TargetMethod.Parameters.Length == 1
&& invocation.TargetMethod.Parameters[0].Type.SpecialType == SpecialType.System_Boolean
&& Utils.IsTask(invocation.TargetMethod.ContainingType);
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.VisualStudio.Threading.Analyzers/Strings.resx:390
- The resource comment for this code-fix title mentions only "async"/"await", but the title text also includes the term "Task". For localization guidance consistency with VSTHRD202_MessageFormat, the comment should also call out that "Task" should not be translated.
<comment>"async" and "await" are C# keywords and should not be translated.</comment>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/Microsoft.VisualStudio.Threading.Analyzers.CodeFixes/VSTHRD202RemoveUnnecessaryAsyncCodeFix.cs:165
- RemoveAsyncModifier removes the async modifier after calling asyncKeyword.WithoutTrivia(), which drops any trailing trivia (e.g., comments/whitespace) attached to the async keyword. This can silently delete comments such as
async /* comment */ Task ...when applying the code fix. Preserve the removed token’s trailing trivia by transferring it to the next token (next modifier or return type) instead of discarding it.
SyntaxToken asyncKeyword = method.Modifiers.First(modifier => modifier.IsKind(SyntaxKind.AsyncKeyword));
int asyncKeywordIndex = method.Modifiers.IndexOf(asyncKeyword);
SyntaxTokenList modifiers = method.Modifiers.Replace(asyncKeyword, asyncKeyword.WithoutTrivia());
modifiers = modifiers.RemoveAt(asyncKeywordIndex);
method = method.WithModifiers(modifiers);
docfx/analyzers/VSTHRD202.md:49
- The documentation’s try/catch example uses
new CancellationToken(canceled: true), but the code fix currently emitsnew CancellationToken(true)(see tests in VSTHRD202RemoveUnnecessaryAsyncAnalyzerTests). Align the doc example with the actual code fix output to avoid confusion.
: new CancellationToken(canceled: true);
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (5)
Previously missed (4) — in code that hasn't changed since the last review.
src/Microsoft.VisualStudio.Threading.Analyzers.CodeFixes/VSTHRD202RemoveUnnecessaryAsyncCodeFix.cs:116
- GetReturnedTaskExpression only strips ConfigureAwait when awaitExpression.Expression is an InvocationExpressionSyntax. If the awaited expression is wrapped in parentheses (e.g.
await (SomethingElseAsync().ConfigureAwait(false))), this method will return the parenthesized invocation and the minimal fix will producereturn (SomethingElseAsync().ConfigureAwait(false));, which won’t compile because it returns a ConfiguredTaskAwaitable instead of Task. Unwrap ParenthesizedExpressionSyntax before checking for ConfigureAwait (but keep returning the original expression when no ConfigureAwait is present).
private static ExpressionSyntax GetReturnedTaskExpression(AwaitExpressionSyntax awaitExpression, SemanticModel semanticModel, CancellationToken cancellationToken)
{
if (awaitExpression.Expression is InvocationExpressionSyntax invocationExpression
&& invocationExpression.Expression is MemberAccessExpressionSyntax memberAccess
&& semanticModel.GetOperation(awaitExpression.Expression, cancellationToken) is IInvocationOperation invocation
src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/VSTHRD202RemoveUnnecessaryAsyncAnalyzer.cs:127
- UnwrapConfigureAwait only handles
awaitOperation.Operationwhen it is directly an IInvocationOperation. If the awaited expression is parenthesized, Roslyn can surface it as an IParenthesizedOperation, which would prevent ConfigureAwait unwrapping and can lead to missed diagnostics and/or incorrect fix application for patterns likeawait (task.ConfigureAwait(false)). Consider unwrapping parenthesized operations (and the corresponding syntax) before testing for ConfigureAwait.
private static IOperation UnwrapConfigureAwait(IAwaitOperation awaitOperation, SemanticModel semanticModel, CancellationToken cancellationToken)
{
if (awaitOperation.Operation is IInvocationOperation invocation && IsTaskConfigureAwait(invocation))
{
if (invocation.Instance is IOperation instance)
docfx/analyzers/VSTHRD202.md:14
- This rule also recognizes ConfigureAwaitRunInline (via CommonInterest.TaskConfigureAwait), not just
.ConfigureAwait(bool). Updating the wording here would keep the documentation aligned with the implementation.
This issue also appears on line 27 of the same file.
The analyzer also recognizes `.ConfigureAwait(bool)` on the final expression:
test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD202RemoveUnnecessaryAsyncAnalyzerTests.cs:216
- There’s currently no test covering a parenthesized awaited ConfigureAwait expression (e.g.
await (SomethingElseAsync().ConfigureAwait(false))). Adding one would help prevent regressions in ConfigureAwait unwrapping and ensure both analyzer and fixes behave correctly with extra parentheses.
public async Task ConfigureAwaitRunInline_OffersMinimalFix()
{
const string source = /* lang=c#-test */ """
using System.Threading.Tasks;
using Microsoft.VisualStudio.Threading;
class Test
{
Task SomethingElseAsync() => Task.CompletedTask;
[|async|] Task DoSomethingAsync()
{
await SomethingElseAsync().ConfigureAwaitRunInline();
}
}
docfx/analyzers/VSTHRD202.md:27
- The minimal fix removes any ConfigureAwait call matched by CommonInterest.TaskConfigureAwait (including ConfigureAwaitRunInline), not only
.ConfigureAwait(bool). Updating this sentence would avoid under-documenting what the fixer removes.
The minimal code fix removes `async`, `await`, and any `.ConfigureAwait(bool)` call:
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Async methods that only return one terminal awaited
Taskincur a state-machine cost that can be avoided. This adds informational guideline VSTHRD202 to identify conservative cases where the exact task can be returned directly.The analyzer excludes transformations involving multiple or non-terminal awaits, asynchronous disposal/enumeration, different task types, and other cases that would change required lifetime semantics. It offers two ordered fixes: a minimal removal of
async/await, and atry/catchvariant that wraps synchronous exceptions whenTask.FromExceptionis available.The diagnostic documentation describes the observable trade-offs, including changed debugging call stacks, exception and cancellation behavior, task identity, and the loss of compiler warning CS4014 after removing
async.Closes #518