Fix VSTHRD003 task origin edge cases

README.md

@@ -1,7 +1,6 @@
 # vs-threading
 
 [![Build Status](https://dev.azure.com/azure-public/vside/_apis/build/status/vs-threading)](https://dev.azure.com/azure-public/vside/_build/latest?definitionId=12)
-[![Join the chat at https://gitter.im/vs-threading/Lobby](https://badges.gitter.im/vs-threading/Lobby.svg)](https://gitter.im/vs-threading/Lobby?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge)
 
 ## Microsoft.VisualStudio.Threading
 

docfx/analyzers/VSTHRD003.md

@@ -10,6 +10,38 @@ When required to await a task that was started earlier, start it within a delega
 `JoinableTaskFactory.RunAsync`, storing the resulting `JoinableTask` in a field or variable.
 You can safely await the `JoinableTask` later.
 
+## Marking known-completed tasks
+
+Awaiting a task that is known to have already completed cannot deadlock. To identify cached
+completed tasks that the analyzer cannot recognize automatically, define an attribute with the
+fully qualified name `Microsoft.VisualStudio.Threading.CompletedTaskAttribute` in your project:
+
+```csharp
+namespace Microsoft.VisualStudio.Threading
+{
+    [System.AttributeUsage(System.AttributeTargets.Field | System.AttributeTargets.Property | System.AttributeTargets.Method)]
+    internal sealed class CompletedTaskAttribute : System.Attribute
+    {
+    }
+}
+```
+
+Apply `[CompletedTask]` to methods that always return completed tasks, `readonly` fields, or
+get-only properties:
+
+```csharp
+[CompletedTask]
+private static readonly Task TrueTask = Task.FromResult(true);
+
+[CompletedTask]
+private static Task<bool> FalseTask { get; } = Task.FromResult(false);
+```
+
+The analyzer reports [VSTHRD013](VSTHRD013.md) on `[CompletedTask]` when it is applied to a mutable field,
+settable property, or ref-returning property, because its value can later be replaced with an
+incomplete task. Consumers still take the attribute at face value, so the diagnostic is reported
+at the invalid attribute application rather than at each use of the member.
+
 ## Simple examples of patterns that are flagged by this analyzer
 
 The following example would likely deadlock if `MyMethod` were called on the main thread,

docfx/analyzers/VSTHRD013.md

@@ -0,0 +1,28 @@
+# VSTHRD013 Apply `CompletedTaskAttribute` only to immutable members
+
+`Microsoft.VisualStudio.Threading.CompletedTaskAttribute` tells VSTHRD003 that a member always
+produces a completed task. Applying it to a mutable member is unsafe because the member can later
+be assigned an incomplete task.
+
+## Examples of patterns that are flagged by this analyzer
+
+```csharp
+[CompletedTask]
+private static Task CachedTask { get; set; } = Task.CompletedTask;
+```
+
+The diagnostic is reported on `[CompletedTask]`, not at each use of `CachedTask`. Consumers take
+the attribute at face value and do not report VSTHRD003.
+
+## Solution
+
+Apply `[CompletedTask]` only to methods that always return completed tasks, `readonly` fields, or
+non-ref get-only properties:
+
+```csharp
+[CompletedTask]
+private static readonly Task CachedTask = Task.CompletedTask;
+```
+
+If the member must remain mutable, remove `[CompletedTask]`. VSTHRD003 will then analyze each use
+normally.

docfx/analyzers/index.md

@@ -13,6 +13,7 @@ ID | Title | Severity | Supports | Default diagnostic severity
 [VSTHRD010](VSTHRD010.md) | Invoke single-threaded types on Main thread | Critical | [1st rule](../docs/threading_rules.md#Rule1) | Warning
 [VSTHRD011](VSTHRD011.md) | Use `AsyncLazy<T>` | Critical | [3rd rule](../docs/threading_rules.md#Rule3) | Error
 [VSTHRD012](VSTHRD012.md) | Provide JoinableTaskFactory where allowed | Critical | [All rules](../docs/threading_rules.md) | Warning
+[VSTHRD013](VSTHRD013.md) | Apply `CompletedTaskAttribute` only to immutable members | Critical | [VSTHRD003](VSTHRD003.md) | Warning
 [VSTHRD100](VSTHRD100.md) | Avoid `async void` methods | Advisory | | Warning
 [VSTHRD101](VSTHRD101.md) | Avoid unsupported async delegates | Advisory | [VSTHRD100](VSTHRD100.md) | Warning
 [VSTHRD102](VSTHRD102.md) | Implement internal logic asynchronously | Advisory | [2nd rule](../docs/threading_rules.md#Rule2) | Info

docfx/analyzers/toc.yml

@@ -10,6 +10,7 @@ items:
 - href: VSTHRD010.md
 - href: VSTHRD011.md
 - href: VSTHRD012.md
+- href: VSTHRD013.md
 - href: VSTHRD100.md
 - href: VSTHRD101.md
 - href: VSTHRD102.md

docfx/docs/threading_rules.md

@@ -59,18 +59,28 @@ JoinableTask longRunningAsyncWork = joinableTaskFactoryInstance.RunAsync(
     });
 ```
 
-then later that async work becomes blocking:
+Then later asynchronous code can join that work while waiting for it:
 
 ```csharp
-longRunningAsyncWork.Join();
+async Task WaitForLongRunningWorkAsync(CancellationToken cancellationToken)
+{
+    await longRunningAsyncWork.JoinAsync(cancellationToken);
+}
 ```
 
-or perhaps
+When cancellation is not required, directly awaiting the `JoinableTask` is equivalent to
+calling `JoinAsync(CancellationToken.None)`:
 
 ```csharp
 await longRunningAsyncWork;
 ```
 
+Synchronous code can join and block on the work:
+
+```csharp
+longRunningAsyncWork.Join();
+```
+
 Note however that this extra step is not necessary when awaiting is
 done immediately after kicking off an asynchronous operation.
 

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

@@ -40,6 +40,8 @@ public class VSTHRD003UseJtfRunAsyncAnalyzer : DiagnosticAnalyzer
 {
     public const string Id = "VSTHRD003";
 
+    public const string InvalidCompletedTaskAttributeId = "VSTHRD013";
+
     internal static readonly DiagnosticDescriptor Descriptor = new DiagnosticDescriptor(
         id: Id,
         title: new LocalizableResourceString(nameof(Strings.VSTHRD003_Title), Strings.ResourceManager, typeof(Strings)),
@@ -49,12 +51,21 @@ public class VSTHRD003UseJtfRunAsyncAnalyzer : DiagnosticAnalyzer
         defaultSeverity: DiagnosticSeverity.Warning,
         isEnabledByDefault: true);
 
+    internal static readonly DiagnosticDescriptor InvalidCompletedTaskAttributeDescriptor = new DiagnosticDescriptor(
+        id: InvalidCompletedTaskAttributeId,
+        title: new LocalizableResourceString(nameof(Strings.VSTHRD013_Title), Strings.ResourceManager, typeof(Strings)),
+        messageFormat: new LocalizableResourceString(nameof(Strings.VSTHRD013_MessageFormat), Strings.ResourceManager, typeof(Strings)),
+        helpLinkUri: Utils.GetHelpLink(InvalidCompletedTaskAttributeId),
+        category: "Usage",
+        defaultSeverity: DiagnosticSeverity.Warning,
+        isEnabledByDefault: true);
+
     /// <inheritdoc />
     public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics
     {
         get
         {
-            return ImmutableArray.Create(Descriptor);
+            return ImmutableArray.Create(Descriptor, InvalidCompletedTaskAttributeDescriptor);
         }
     }
 
@@ -69,10 +80,18 @@ public override void Initialize(AnalysisContext context)
         context.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(this.AnalyzeArrowExpressionClause), SyntaxKind.ArrowExpressionClause);
         context.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(this.AnalyzeLambdaExpression), SyntaxKind.SimpleLambdaExpression);
         context.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(this.AnalyzeLambdaExpression), SyntaxKind.ParenthesizedLambdaExpression);
+        context.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(this.AnalyzeCompletedTaskAttribute), SyntaxKind.Attribute);
     }
 
     private static bool IsSymbolAlwaysOkToAwait(ISymbol? symbol)
     {
+        if (symbol is IMethodSymbol or IFieldSymbol or IPropertySymbol &&
+            symbol.GetAttributes().Any(attribute => IsCompletedTaskAttribute(attribute.AttributeClass)))
+        {
+            // Consumers take this assertion at face value. Invalid applications are diagnosed at the declaration.
+            return true;
+        }
+
         if (symbol is IFieldSymbol field)
         {
             // Allow the TplExtensions.CompletedTask and related fields.
@@ -95,16 +114,68 @@ private static bool IsSymbolAlwaysOkToAwait(ISymbol? symbol)
         return false;
     }
 
+    private static bool IsCompletedTaskAttribute(INamedTypeSymbol? attributeType) =>
+        attributeType?.Name == Types.CompletedTaskAttribute.TypeName &&
+        attributeType.ContainingType is null &&
+        attributeType.BelongsToNamespace(Types.CompletedTaskAttribute.Namespace);
+
+    private void AnalyzeCompletedTaskAttribute(SyntaxNodeAnalysisContext context)
+    {
+        var attribute = (AttributeSyntax)context.Node;
+        if (context.SemanticModel.GetSymbolInfo(attribute, context.CancellationToken).Symbol is not IMethodSymbol attributeConstructor ||
+            !IsCompletedTaskAttribute(attributeConstructor.ContainingType))
+        {
+            return;
+        }
+
+        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));
+                break;
+            case PropertyDeclarationSyntax property:
+                IPropertySymbol? propertySymbol = context.SemanticModel.GetDeclaredSymbol(property, context.CancellationToken);
+                mutableMemberName = propertySymbol is { SetMethod: not null } || propertySymbol is { ReturnsByRef: true } ? propertySymbol.Name : null;
+                break;
+            case IndexerDeclarationSyntax indexer:
+                IPropertySymbol? indexerSymbol = context.SemanticModel.GetDeclaredSymbol(indexer, context.CancellationToken);
+                mutableMemberName = indexerSymbol is { SetMethod: not null } || indexerSymbol is { ReturnsByRef: true } ? indexerSymbol.Name : null;
+                break;
+            default:
+                mutableMemberName = null;
+                break;
+        }
+
+        if (mutableMemberName is not null)
+        {
+            context.ReportDiagnostic(Diagnostic.Create(InvalidCompletedTaskAttributeDescriptor, attribute.GetLocation(), mutableMemberName));
+        }
+    }
+
     private void AnalyzeArrowExpressionClause(SyntaxNodeAnalysisContext context)
     {
         var arrowExpressionClause = (ArrowExpressionClauseSyntax)context.Node;
-        if (arrowExpressionClause.Parent is MethodDeclarationSyntax)
+        ISymbol? containingSymbol = arrowExpressionClause.Parent switch
         {
-            Diagnostic? diagnostic = this.AnalyzeAwaitedOrReturnedExpression(arrowExpressionClause.Expression, context, context.CancellationToken);
-            if (diagnostic is object)
-            {
-                context.ReportDiagnostic(diagnostic);
-            }
+            MethodDeclarationSyntax method => context.SemanticModel.GetDeclaredSymbol(method, context.CancellationToken),
+            LocalFunctionStatementSyntax localFunction => context.SemanticModel.GetDeclaredSymbol(localFunction, context.CancellationToken),
+            PropertyDeclarationSyntax property => context.SemanticModel.GetDeclaredSymbol(property, context.CancellationToken),
+            IndexerDeclarationSyntax indexer => context.SemanticModel.GetDeclaredSymbol(indexer, context.CancellationToken),
+            AccessorDeclarationSyntax { Parent.Parent: PropertyDeclarationSyntax property } => context.SemanticModel.GetDeclaredSymbol(property, context.CancellationToken),
+            AccessorDeclarationSyntax { Parent.Parent: IndexerDeclarationSyntax indexer } => context.SemanticModel.GetDeclaredSymbol(indexer, context.CancellationToken),
+            _ => null,
+        };
+
+        if (containingSymbol is null || IsSymbolAlwaysOkToAwait(containingSymbol))
+        {
+            return;
+        }
+
+        Diagnostic? diagnostic = this.AnalyzeAwaitedOrReturnedExpression(arrowExpressionClause.Expression, context, context.CancellationToken);
+        if (diagnostic is object)
+        {
+            context.ReportDiagnostic(diagnostic);
         }
     }
 
@@ -124,6 +195,20 @@ private void AnalyzeLambdaExpression(SyntaxNodeAnalysisContext context)
     private void AnalyzeReturnStatement(SyntaxNodeAnalysisContext context)
     {
         var returnStatement = (ReturnStatementSyntax)context.Node;
+        CSharpUtils.ContainingFunctionData containingFunction = CSharpUtils.GetContainingFunction(returnStatement);
+        ISymbol? containingSymbol = containingFunction.Function switch
+        {
+            MethodDeclarationSyntax methodDeclaration => context.SemanticModel.GetDeclaredSymbol(methodDeclaration, context.CancellationToken),
+            LocalFunctionStatementSyntax localFunction => context.SemanticModel.GetDeclaredSymbol(localFunction, context.CancellationToken),
+            AccessorDeclarationSyntax { Parent.Parent: PropertyDeclarationSyntax propertyDeclaration } => context.SemanticModel.GetDeclaredSymbol(propertyDeclaration, context.CancellationToken),
+            AccessorDeclarationSyntax { Parent.Parent: IndexerDeclarationSyntax indexerDeclaration } => context.SemanticModel.GetDeclaredSymbol(indexerDeclaration, context.CancellationToken),
+            _ => null,
+        };
+        if (IsSymbolAlwaysOkToAwait(containingSymbol))
+        {
+            return;
+        }
+
         Diagnostic? diagnostic = this.AnalyzeAwaitedOrReturnedExpression(returnStatement.Expression, context, context.CancellationToken);
         if (diagnostic is object)
         {
@@ -228,7 +313,7 @@ private void AnalyzeAwaitExpression(SyntaxNodeAnalysisContext context)
                 symbolType = parameterSymbol.Type;
                 dataflowAnalysisCompatibleVariable = true;
                 break;
-            case IFieldSymbol fieldSymbol:
+            case IFieldSymbol fieldSymbol when !IsSymbolAlwaysOkToAwait(fieldSymbol):
                 symbolType = fieldSymbol.Type;
 
                 // If the field is readonly and initialized with Task.FromResult, it's OK.
@@ -287,7 +372,7 @@ private void AnalyzeAwaitExpression(SyntaxNodeAnalysisContext context)
                 }
 
                 break;
-            case IMethodSymbol methodSymbol:
+            case IMethodSymbol methodSymbol when !IsSymbolAlwaysOkToAwait(methodSymbol):
                 if (Utils.IsTask(methodSymbol.ReturnType) && focusedExpression is InvocationExpressionSyntax invocationExpressionSyntax)
                 {
                     // Consider all arguments
@@ -305,6 +390,8 @@ private void AnalyzeAwaitExpression(SyntaxNodeAnalysisContext context)
                     return expressionsToConsider.Select(e => this.AnalyzeAwaitedOrReturnedExpression(e, context, cancellationToken)).FirstOrDefault(r => r is object);
                 }
 
+                return null;
+            case IFieldSymbol or IMethodSymbol:
                 return null;
             default:
                 return null;
@@ -317,6 +404,15 @@ private void AnalyzeAwaitExpression(SyntaxNodeAnalysisContext context)
 
         // Report warning if the task was not initialized within the current delegate or lambda expression
         containingFunc ??= CSharpUtils.GetContainingFunction(focusedExpression);
+        if (containingFunc.Value.BlockOrExpression is null &&
+            symbolToConsider.Symbol is ILocalSymbol &&
+            focusedExpression.Ancestors().Any(ancestor => ancestor is GlobalStatementSyntax))
+        {
+            // 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;
+        }
+
         if (containingFunc.Value.BlockOrExpression is BlockSyntax delegateBlock)
         {
             if (dataflowAnalysisCompatibleVariable)

src/Microsoft.VisualStudio.Threading.Analyzers/Strings.resx

@@ -172,6 +172,14 @@ Start the work within this context, or use JoinableTaskFactory.RunAsync to start
   <data name="VSTHRD003_Title" xml:space="preserve">
     <value>Avoid awaiting foreign Tasks</value>
   </data>
+  <data name="VSTHRD013_MessageFormat" xml:space="preserve">
+    <value>CompletedTaskAttribute cannot be applied to mutable member "{0}". Apply it only to methods, readonly fields, or non-ref get-only properties.</value>
+    <comment>CompletedTaskAttribute is a type name and should not be translated. {0} is the name of a field, property, or indexer.</comment>
+  </data>
+  <data name="VSTHRD013_Title" xml:space="preserve">
+    <value>Apply CompletedTaskAttribute only to immutable members</value>
+    <comment>CompletedTaskAttribute is a type name and should not be translated.</comment>
+  </data>
   <data name="VSTHRD011b_MessageFormat" xml:space="preserve">
     <value>Invoking or blocking on async code in a Lazy&lt;T&gt; value factory can deadlock. Use AsyncLazy&lt;T&gt; instead.</value>
   </data>

src/Microsoft.VisualStudio.Threading.Analyzers/Types.cs

@@ -96,6 +96,22 @@ public static class AsyncMethodBuilderAttribute
         public static readonly ImmutableArray<string> Namespace = Namespaces.SystemRuntimeCompilerServices;
     }
 
+    /// <summary>
+    /// Contains descriptors for the convention-based CompletedTaskAttribute type.
+    /// </summary>
+    public static class CompletedTaskAttribute
+    {
+        /// <summary>
+        /// The name of the attribute type.
+        /// </summary>
+        public const string TypeName = nameof(CompletedTaskAttribute);
+
+        /// <summary>
+        /// The namespace containing the attribute type.
+        /// </summary>
+        public static readonly ImmutableArray<string> Namespace = Namespaces.MicrosoftVisualStudioThreading;
+    }
+
     /// <summary>
     /// Contains descriptors for the JoinableTaskFactory type.
     /// </summary>