Fix VSTHRD003 task origin edge cases

[Andrew Arnott (AArnott)]

Summary

• recognize a convention-based Microsoft.VisualStudio.Threading.CompletedTaskAttribute on methods, readonly fields, and get-only properties, without adding a public runtime API or injected source
• treat top-level-statement locals as belonging to the compiler-generated Main method while continuing to diagnose foreign fields
• lock in detection when a task field is assigned before being returned from an expression-bodied JoinableTaskFactory.Run delegate
• clarify Rule 3 with JoinAsync(cancellationToken), direct JoinableTask awaiting, and synchronous Join() examples

Issue outcomes

• Fixes VSTHRD003 support for marking known completed tasks as non-foreign #1509
• Fixes VSTHRD003 False positive in top-level statements #1370
• Closes Third threading rule could be more clear about use of JoinAsync #697
• Fixes VSTHRD003 fails to identify when 'foreign task' is assigned to field and awaited on within the same method #679

All four issues in this subset are addressed; none are left unresolved. The completed-task marker is intentionally convention-based and locally definable, avoiding the unresolved public-API versus injected-source packaging tradeoff in #1510.

Validation

• VSTHRD003 analyzer tests: 186 passed across net472, net8.0, and net8.0-windows
• full analyzer test project: 1,473 passed, 33 existing skips
• full Release build: succeeded with 0 warnings and 0 errors

[Copilot]

Pull request overview

This PR updates the VSTHRD003 analyzer and docs to better handle several task-origin edge cases (convention-based “completed task” markers, top-level statements, and some JoinableTaskFactory patterns) while extending test coverage to lock in the intended behaviors.

Changes:

• Add convention-based recognition of Microsoft.VisualStudio.Threading.CompletedTaskAttribute (methods, readonly fields, get-only properties) to suppress VSTHRD003 where tasks are asserted to be known-completed.
• Adjust analyzer behavior for top-level statements and update tests to validate correct diagnostics in console-app top-level code.
• Clarify Rule #3 documentation/examples around JoinAsync(cancellationToken), direct JoinableTask awaiting, and synchronous Join().

Reviewed changes

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

Show a summary per file

File	Description
test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD003UseJtfRunAsyncAnalyzerTests.cs	Adds regression tests covering completed-task attribute behavior, top-level statements behavior, and a JTF.Run/field-assignment edge case.
src/Microsoft.VisualStudio.Threading.Analyzers/Types.cs	Introduces Types.CompletedTaskAttribute descriptors used for convention-based attribute matching.
src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/VSTHRD003UseJtfRunAsyncAnalyzer.cs	Implements attribute-based suppression, adds containing-member checks for return/arrow bodies, and special-cases top-level locals.
docfx/docs/threading_rules.md	Updates Rule #3 examples to include JoinAsync(cancellationToken), await joinableTask, and Join().
docfx/analyzers/VSTHRD003.md	Documents the convention-based CompletedTaskAttribute approach and its intended constraints.

Suppressed comments (1)

src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/VSTHRD003UseJtfRunAsyncAnalyzer.cs:361

• This early-return suppresses VSTHRD003 for any awaited/returned top-level local whenever GetContainingFunction can’t find a block. That avoids the false positive from #1370, but it also skips diagnostics for locals that merely alias a foreign task (e.g. var t = State.Task; await t;), which is not equivalent to treating them as belonging to the synthesized Main method.

        {
            // Top-level locals belong to the compiler-generated Main method, even though there is no
            // method declaration in syntax for GetContainingFunction to discover.
            return null;
        }

---

💡 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 7 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD003UseJtfRunAsyncAnalyzerTests.cs:1109

• This test is named around “completed” tasks, but uses Task.Delay(1) and leaves task uninitialized, which makes the example misleading (and the block property currently returns a null task). Using Task.CompletedTask/Task.FromResult(...) improves clarity while still exercising the analyzer’s attribute recognition.

                private static Task task;

                [CompletedTask]
                private static readonly Task CompletedField = Task.Delay(1);

test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD003UseJtfRunAsyncAnalyzerTests.cs:1126

• The [CompletedTask] methods in this test return their Task parameter, which contradicts the documented contract (“always return completed tasks”) and can confuse future readers. Returning a known-completed task keeps the test intent (attribute is trusted) without modeling an invalid usage pattern.

                [CompletedTask]
                private static Task ReturnCompletedTask(Task task)
                {
                    return task;
                }

test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD003UseJtfRunAsyncAnalyzerTests.cs:1144

• Similarly, the local function marked [CompletedTask] returns its Task parameter, which doesn’t align with the intended meaning of the attribute. Returning a completed task makes the example consistent with the guidance.

                    [CompletedTask]
                    static Task ReturnCompletedTaskLocal(Task task)
                    {
                        return task;
                    }

docfx/analyzers/VSTHRD003.md:23

• The attribute-definition snippet won’t compile as written because it uses AttributeUsage/Attribute without using System; (or fully qualifying these types). Since the snippet is inside a markdown code fence, the simplest self-contained fix is to fully qualify these types.

    [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property | AttributeTargets.Method)]
    internal sealed class CompletedTaskAttribute : Attribute

[Copilot]

Pull request overview

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

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD003UseJtfRunAsyncAnalyzerTests.cs:1108

• These new CompletedTaskAttribute tests initialize "completed" members with Task.Delay(1), which is not completed and can be misleading for readers. Consider using a task that is completed synchronously but still not recognized by existing heuristics (so the attribute behavior is still exercised).

This issue also appears on line 1172 of the same file.

                [CompletedTask]
                private static readonly Task CompletedField = Task.Delay(1);

                [CompletedTask]
                private static Task CompletedProperty { get; } = Task.Delay(1);

src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/VSTHRD003UseJtfRunAsyncAnalyzer.cs:134

• AnalyzeCompletedTaskAttribute only reports VSTHRD116 for the first variable in a non-readonly field declaration (Variables[0]). If a declaration contains multiple fields, the other mutable fields will still be treated as CompletedTask but won’t get an invalid-usage diagnostic.

            FieldDeclarationSyntax field when !field.Modifiers.Any(SyntaxKind.ReadOnlyKeyword) =>
                context.SemanticModel.GetDeclaredSymbol(field.Declaration.Variables[0], context.CancellationToken),

test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD003UseJtfRunAsyncAnalyzerTests.cs:1176

• This mutable-member CompletedTaskAttribute test also uses Task.Delay(1) for the attributed members, which conflicts with the intent implied by the attribute name/documentation. Using a synchronously-completed task (e.g., Task.WhenAll(Task.CompletedTask)) keeps the example semantically aligned while still verifying that VSTHRD116 is produced for mutability.

                [{|#0:CompletedTask|}]
                private static Task CompletedField = Task.Delay(1);

                [{|#1:CompletedTask|}]
                private static Task CompletedProperty { get; set; } = Task.Delay(1);

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/VSTHRD003UseJtfRunAsyncAnalyzer.cs:135

• AnalyzeCompletedTaskAttribute only uses field.Declaration.Variables[0] to get the field symbol, so a single [CompletedTask] applied to a multi-variable field declaration will report VSTHRD013 with the wrong member name (and only one diagnostic). Also, the property/indexer arms call GetDeclaredSymbol twice unnecessarily.

        string? mutableMemberName;
        switch (attribute.Parent?.Parent)
        {
            case FieldDeclarationSyntax field when !field.Modifiers.Any(SyntaxKind.ReadOnlyKeyword):
                mutableMemberName = string.Join(", ", field.Declaration.Variables.Select(variable => variable.Identifier.ValueText));

[Copilot]

Pull request overview

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