[Andrew Arnott (AArnott)]

VSTHRD002 currently skips entire Task-returning code blocks because VSTHRD103 covers synchronous waits in Task-returning methods and delegates. This leaves a gap for nested non-Task-returning functions, such as Action lambdas, because VSTHRD103 also excludes those functions.

Register VSTHRD002 for Task-returning code blocks, but analyze only nested functions whose return type is not async-compatible. This preserves the division between VSTHRD002 and VSTHRD103 while detecting synchronous waits in assigned lambdas and callback arguments.

Adds regression coverage for nested Action forms and verifies that Task-returning delegates remain excluded to avoid duplicate diagnostics.

Fixes #632
Fixes #1427

[Copilot]

Pull request overview

This PR adjusts the VSTHRD002 analyzer so it can detect synchronous waits inside nested non-Task-returning functions (e.g., Action lambdas) that appear within Task-returning methods, closing the false-negative gap described in #632 and #1427 while still avoiding duplicate diagnostics with VSTHRD103.

Changes:

• Register VSTHRD002 analysis for Task-returning code blocks, but gate reporting to nested functions whose return type is not async-compatible.
• Add regression tests covering nested Action lambdas and callback arguments inside Task-returning methods.
• Add a test ensuring Task-returning delegates remain excluded (to avoid duplicating VSTHRD103).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/VSTHRD002UseJtfRunAnalyzer.cs	Extends analyzer registration to Task-returning code blocks and filters analysis to nested non-async-compatible functions.
test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD002UseJtfRunAnalyzerTests.cs	Adds regression coverage for nested Action forms and confirms Task-returning delegates stay excluded.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (4)

test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD002UseJtfRunAnalyzerTests.cs:416

• VerifyCodeFixAsync(test, test) is ambiguous here: if the code fix is expected to change code, these tests won’t assert the transformation; if no code fix exists for these diagnostics, some verifier implementations will fail because a fix cannot be applied. If the intent is “diagnostic only”, prefer CSVerify.VerifyAnalyzerAsync(test) while keeping the [| |] markup; if a fix is expected, provide an explicit expected fixed-source string that reflects the intended fix output.

        await CSVerify.VerifyCodeFixAsync(test, test);

test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD002UseJtfRunAnalyzerTests.cs:434

• VerifyCodeFixAsync(test, test) is ambiguous here: if the code fix is expected to change code, these tests won’t assert the transformation; if no code fix exists for these diagnostics, some verifier implementations will fail because a fix cannot be applied. If the intent is “diagnostic only”, prefer CSVerify.VerifyAnalyzerAsync(test) while keeping the [| |] markup; if a fix is expected, provide an explicit expected fixed-source string that reflects the intended fix output.

        await CSVerify.VerifyCodeFixAsync(test, test);

src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/VSTHRD002UseJtfRunAnalyzer.cs:75

• This change registers syntax node actions for all methods (including Task-returning ones) and then filters on every invocation/member-access via Ancestors() plus SemanticModel.GetEnclosingSymbol(...). In Task-heavy codebases this can significantly increase analyzer overhead because these callbacks run for every invocation/member access in those methods. Consider restructuring to avoid per-node semantic queries when analyzeWholeCodeBlock is false—for example, in Task-returning methods register actions on AnonymousFunctionExpressionSyntax / LocalFunctionStatementSyntax and only walk/analyze inside those bodies when the nested function is non-Task-returning. This keeps correctness while reducing the number of callbacks and semantic model lookups.

                    if (propertySymbol is object || methodSymbol is object)
                    {
                        bool analyzeWholeCodeBlock = propertySymbol is object || !methodSymbol!.HasAsyncCompatibleReturnType();
                        codeBlockContext.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(c => AnalyzeInvocation(c, taskSymbol, analyzeWholeCodeBlock)), SyntaxKind.InvocationExpression);
                        codeBlockContext.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(c => AnalyzeMemberAccess(c, taskSymbol, analyzeWholeCodeBlock)), SyntaxKind.SimpleMemberAccessExpression);
                    }

src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/VSTHRD002UseJtfRunAnalyzer.cs:99

• This change registers syntax node actions for all methods (including Task-returning ones) and then filters on every invocation/member-access via Ancestors() plus SemanticModel.GetEnclosingSymbol(...). In Task-heavy codebases this can significantly increase analyzer overhead because these callbacks run for every invocation/member access in those methods. Consider restructuring to avoid per-node semantic queries when analyzeWholeCodeBlock is false—for example, in Task-returning methods register actions on AnonymousFunctionExpressionSyntax / LocalFunctionStatementSyntax and only walk/analyze inside those bodies when the nested function is non-Task-returning. This keeps correctness while reducing the number of callbacks and semantic model lookups.

    private static bool ShouldAnalyze(SyntaxNodeAnalysisContext context, bool analyzeWholeCodeBlock)
    {
        if (analyzeWholeCodeBlock)
        {
            return true;
        }

        SyntaxNode? containingFunction = context.Node.Ancestors().FirstOrDefault(
            node => node is AnonymousFunctionExpressionSyntax or LocalFunctionStatementSyntax or BaseMethodDeclarationSyntax);
        if (containingFunction is not AnonymousFunctionExpressionSyntax and not LocalFunctionStatementSyntax)
        {
            return false;
        }

        // Task-returning methods and delegates are covered by VSTHRD103. VSTHRD002 covers
        // nested functions that do not return Task so that every synchronous wait is diagnosed.
        return context.SemanticModel.GetEnclosingSymbol(context.Node.SpanStart, context.CancellationToken) is IMethodSymbol containingMethod
            && !containingMethod.HasAsyncCompatibleReturnType();
    }

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/VSTHRD002UseJtfRunAnalyzer.cs:96

• The comment mentions only "Task-returning" functions, but the code uses HasAsyncCompatibleReturnType(), which also treats other async-compatible return types (e.g., ValueTask, IAsyncEnumerable, types with AsyncMethodBuilderAttribute) as covered by VSTHRD103. Updating the comment would avoid misleading future maintainers about the actual behavior.

        // Task-returning methods and delegates are covered by VSTHRD103. VSTHRD002 covers
        // nested functions that do not return Task so that every synchronous wait is diagnosed.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.