diff --git a/README.md b/README.md index cabe45111..575ba3d16 100644 --- a/README.md +++ b/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 diff --git a/docfx/analyzers/VSTHRD003.md b/docfx/analyzers/VSTHRD003.md index 49d8ad87b..2027bba2b 100644 --- a/docfx/analyzers/VSTHRD003.md +++ b/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 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, diff --git a/docfx/analyzers/VSTHRD013.md b/docfx/analyzers/VSTHRD013.md new file mode 100644 index 000000000..5762777f2 --- /dev/null +++ b/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. diff --git a/docfx/analyzers/index.md b/docfx/analyzers/index.md index 4d2077c25..bf0c27b74 100644 --- a/docfx/analyzers/index.md +++ b/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` | 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 diff --git a/docfx/analyzers/toc.yml b/docfx/analyzers/toc.yml index cc7bee4ff..c4efe0be8 100644 --- a/docfx/analyzers/toc.yml +++ b/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 diff --git a/docfx/docs/threading_rules.md b/docfx/docs/threading_rules.md index cf6115457..15dea4e30 100644 --- a/docfx/docs/threading_rules.md +++ b/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. diff --git a/src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/VSTHRD003UseJtfRunAsyncAnalyzer.cs b/src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/VSTHRD003UseJtfRunAsyncAnalyzer.cs index 19864c651..62a83284b 100644 --- a/src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/VSTHRD003UseJtfRunAsyncAnalyzer.cs +++ b/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); + /// public override ImmutableArray 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) diff --git a/src/Microsoft.VisualStudio.Threading.Analyzers/Strings.resx b/src/Microsoft.VisualStudio.Threading.Analyzers/Strings.resx index 97e248e4d..c53cc7113 100644 --- a/src/Microsoft.VisualStudio.Threading.Analyzers/Strings.resx +++ b/src/Microsoft.VisualStudio.Threading.Analyzers/Strings.resx @@ -172,6 +172,14 @@ Start the work within this context, or use JoinableTaskFactory.RunAsync to start Avoid awaiting foreign Tasks + + CompletedTaskAttribute cannot be applied to mutable member "{0}". Apply it only to methods, readonly fields, or non-ref get-only properties. + CompletedTaskAttribute is a type name and should not be translated. {0} is the name of a field, property, or indexer. + + + Apply CompletedTaskAttribute only to immutable members + CompletedTaskAttribute is a type name and should not be translated. + Invoking or blocking on async code in a Lazy<T> value factory can deadlock. Use AsyncLazy<T> instead. diff --git a/src/Microsoft.VisualStudio.Threading.Analyzers/Types.cs b/src/Microsoft.VisualStudio.Threading.Analyzers/Types.cs index f6d24b8db..44c87baaf 100644 --- a/src/Microsoft.VisualStudio.Threading.Analyzers/Types.cs +++ b/src/Microsoft.VisualStudio.Threading.Analyzers/Types.cs @@ -96,6 +96,22 @@ public static class AsyncMethodBuilderAttribute public static readonly ImmutableArray Namespace = Namespaces.SystemRuntimeCompilerServices; } + /// + /// Contains descriptors for the convention-based CompletedTaskAttribute type. + /// + public static class CompletedTaskAttribute + { + /// + /// The name of the attribute type. + /// + public const string TypeName = nameof(CompletedTaskAttribute); + + /// + /// The namespace containing the attribute type. + /// + public static readonly ImmutableArray Namespace = Namespaces.MicrosoftVisualStudioThreading; + } + /// /// Contains descriptors for the JoinableTaskFactory type. /// diff --git a/test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD003UseJtfRunAsyncAnalyzerTests.cs b/test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD003UseJtfRunAsyncAnalyzerTests.cs index ce64483d5..05d32e912 100644 --- a/test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD003UseJtfRunAsyncAnalyzerTests.cs +++ b/test/Microsoft.VisualStudio.Threading.Analyzers.Tests/VSTHRD003UseJtfRunAsyncAnalyzerTests.cs @@ -37,8 +37,8 @@ public async Task SomeOperationAsync() "; DiagnosticResult[] expected = { - CSVerify.Diagnostic().WithLocation(15, 19), - CSVerify.Diagnostic().WithLocation(16, 19), + Diagnostic().WithLocation(15, 19), + Diagnostic().WithLocation(16, 19), }; await CSVerify.VerifyAnalyzerAsync(test, expected); } @@ -71,7 +71,7 @@ public async Task SomeOperationAsync() } } "; - DiagnosticResult expected = CSVerify.Diagnostic().WithLocation(14, 19); + DiagnosticResult expected = Diagnostic().WithLocation(14, 19); await CSVerify.VerifyAnalyzerAsync(test, expected); } @@ -91,7 +91,7 @@ public static T WaitAndGetResult(Task task) } } "; - DiagnosticResult expected = CSVerify.Diagnostic().WithLocation(10, 59); + DiagnosticResult expected = Diagnostic().WithLocation(10, 59); await CSVerify.VerifyAnalyzerAsync(test, expected); } @@ -111,10 +111,33 @@ public static T WaitAndGetResult(Task task) } } "; - DiagnosticResult expected = CSVerify.Diagnostic().WithLocation(10, 68); + DiagnosticResult expected = Diagnostic().WithLocation(10, 68); await CSVerify.VerifyAnalyzerAsync(test, expected); } + [Fact] + public async Task ReportWarningWhenTaskFieldAssignedOutsideExpressionLambdaIsReturnedFromJtfRun() + { + string test = """ + using System.Threading.Tasks; + using Microsoft.VisualStudio.Threading; + + class Tests + { + private Task task; + private JoinableTaskFactory jtf; + + public void Test() + { + this.task = this.jtf.RunAsync(async () => await Task.Yield()).Task; + this.jtf.Run(() => {|VSTHRD003:this.task|}); + } + } + """; + + await CSVerify.VerifyAnalyzerAsync(test); + } + [Fact] public async Task ReportWarningWhenTaskIsReturnedDirectlyFromMethod() { @@ -201,7 +224,7 @@ class Tests public async Task AwaitAndGetResult() {{ - await [|task|].ConfigureAwait({(continueOnCapturedContext ? "true" : "false")}); + await {{|VSTHRD003:task|}}.ConfigureAwait({(continueOnCapturedContext ? "true" : "false")}); }} }} "; @@ -222,7 +245,7 @@ class Tests public async Task AwaitAndGetResult() {{ - return await [|task|].ConfigureAwait({(continueOnCapturedContext ? "true" : "false")}); + return await {{|VSTHRD003:task|}}.ConfigureAwait({(continueOnCapturedContext ? "true" : "false")}); }} }} "; @@ -242,7 +265,7 @@ class Tests public async Task AwaitAndGetResult() { - await [|task|].ConfigureAwaitRunInline(); + await {|VSTHRD003:task|}.ConfigureAwaitRunInline(); } } "; @@ -262,7 +285,7 @@ class Tests public async Task AwaitAndGetResult() { - return await [|task|].ConfigureAwaitRunInline(); + return await {|VSTHRD003:task|}.ConfigureAwaitRunInline(); } } "; @@ -311,7 +334,7 @@ public static T WaitAndGetResult(Task task, CancellationToken cancellation } } "; - DiagnosticResult expected = CSVerify.Diagnostic().WithLocation(11, 59); + DiagnosticResult expected = Diagnostic().WithLocation(11, 59); await CSVerify.VerifyAnalyzerAsync(test, expected); } @@ -390,7 +413,7 @@ public async Task SomeOperationAsync() } } "; - DiagnosticResult expected = CSVerify.Diagnostic().WithLocation(14, 19); + DiagnosticResult expected = Diagnostic().WithLocation(14, 19); await CSVerify.VerifyAnalyzerAsync(test, expected); } @@ -423,7 +446,7 @@ public async Task SomeOperationAsync() } } "; - DiagnosticResult expected = CSVerify.Diagnostic().WithLocation(14, 19); + DiagnosticResult expected = Diagnostic().WithLocation(14, 19); await CSVerify.VerifyAnalyzerAsync(test, expected); } @@ -663,7 +686,7 @@ public async Task SomeOperationAsync() } } "; - DiagnosticResult expected = CSVerify.Diagnostic().WithLocation(14, 19); + DiagnosticResult expected = Diagnostic().WithLocation(14, 19); await CSVerify.VerifyAnalyzerAsync(test, expected); } @@ -697,7 +720,7 @@ public async Task SomeOperationAsync() } } "; - DiagnosticResult expected = CSVerify.Diagnostic().WithLocation(16, 19); + DiagnosticResult expected = Diagnostic().WithLocation(16, 19); await CSVerify.VerifyAnalyzerAsync(test, expected); } @@ -732,7 +755,7 @@ public async Task SomeOperationAsync() } } "; - DiagnosticResult expected = CSVerify.Diagnostic().WithLocation(16, 23); + DiagnosticResult expected = Diagnostic().WithLocation(16, 23); await CSVerify.VerifyAnalyzerAsync(test, expected); } @@ -768,9 +791,9 @@ public async Task SomeOperationAsync() "; DiagnosticResult[] expected = { - CSVerify.Diagnostic().WithLocation(14, 19), - CSVerify.Diagnostic().WithLocation(15, 19), - CSVerify.Diagnostic().WithLocation(16, 19), + Diagnostic().WithLocation(14, 19), + Diagnostic().WithLocation(15, 19), + Diagnostic().WithLocation(16, 19), }; await CSVerify.VerifyAnalyzerAsync(test, expected); @@ -896,7 +919,7 @@ public async Task SomeOperationAsync() } } "; - DiagnosticResult expected = CSVerify.Diagnostic().WithLocation(17, 23); + DiagnosticResult expected = Diagnostic().WithLocation(17, 23); await CSVerify.VerifyAnalyzerAsync(test, expected); } @@ -938,7 +961,7 @@ public async Task SomeOperationAsync() } } "; - DiagnosticResult expected = CSVerify.Diagnostic().WithLocation(24, 19); + DiagnosticResult expected = Diagnostic().WithLocation(24, 19); await CSVerify.VerifyAnalyzerAsync(test, expected); } @@ -1058,6 +1081,195 @@ public Task GetTask(int i) await CSVerify.VerifyAnalyzerAsync(test); } + [Fact] + public async Task DoNotReportWarningForMembersMarkedAsCompletedTasks() + { + string test = """ + using System; + using System.Threading.Tasks; + using Microsoft.VisualStudio.Threading; + + namespace Microsoft.VisualStudio.Threading + { + [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property | AttributeTargets.Method)] + internal sealed class CompletedTaskAttribute : Attribute + { + } + } + + class Tests + { + private static Task task = Task.WhenAll(Task.CompletedTask); + + [CompletedTask] + private static readonly Task CompletedField = Task.WhenAll(Task.CompletedTask); + + [CompletedTask] + private static Task CompletedProperty { get; } = Task.WhenAll(Task.CompletedTask); + + [CompletedTask] + private static Task CompletedExpressionProperty => task; + + [CompletedTask] + private static Task CompletedExpressionGetter + { + get => task; + } + + [CompletedTask] + private static Task CompletedBlockProperty + { + get + { + return task; + } + } + + [CompletedTask] + private static Task ReturnCompletedTask(Task task) + { + return Task.WhenAll(Task.CompletedTask); + } + + [CompletedTask] + private static Task ReturnCompletedTaskExpression(Task task) => Task.WhenAll(Task.CompletedTask); + + public Task GetField() => CompletedField; + + public Task GetProperty() => CompletedProperty; + + public Task GetExpressionProperty() => CompletedExpressionProperty; + + public Task GetExpressionGetter() => CompletedExpressionGetter; + + public Task GetBlockProperty() => CompletedBlockProperty; + + public Task GetMethodResult(Task task) => ReturnCompletedTask(task); + + public Task GetExpressionMethodResult(Task task) => ReturnCompletedTaskExpression(task); + + public Task GetLocalFunctionResult(Task task) + { + [CompletedTask] + static Task ReturnCompletedTaskLocal(Task task) + { + return Task.WhenAll(Task.CompletedTask); + } + + return ReturnCompletedTaskLocal(task); + } + } + """; + + await CSVerify.VerifyAnalyzerAsync(test); + } + + [Fact] + public async Task ReportWarningForMutableMembersMarkedAsCompletedTasks() + { + string test = """ + using System; + using System.Threading.Tasks; + using Microsoft.VisualStudio.Threading; + + namespace Microsoft.VisualStudio.Threading + { + [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property | AttributeTargets.Method)] + internal sealed class CompletedTaskAttribute : Attribute + { + } + } + + class Tests + { + [{|#0:CompletedTask|}] + private static Task CompletedField1 = Task.WhenAll(Task.CompletedTask), CompletedField2 = Task.WhenAll(Task.CompletedTask); + + [{|#1:CompletedTask|}] + private static Task CompletedProperty { get; set; } = Task.WhenAll(Task.CompletedTask); + + private static Task task = Task.WhenAll(Task.CompletedTask); + + [{|#2:CompletedTask|}] + private static ref Task CompletedRefProperty => ref task; + + public Task GetField1() => CompletedField1; + + public Task GetField2() => CompletedField2; + + public Task GetProperty() => CompletedProperty; + + public Task GetRefProperty() => CompletedRefProperty; + } + """; + + DiagnosticResult[] expected = + { + InvalidCompletedTaskAttributeDiagnostic().WithLocation(0).WithArguments("CompletedField1, CompletedField2"), + InvalidCompletedTaskAttributeDiagnostic().WithLocation(1).WithArguments("CompletedProperty"), + InvalidCompletedTaskAttributeDiagnostic().WithLocation(2).WithArguments("CompletedRefProperty"), + }; + await CSVerify.VerifyAnalyzerAsync(test, expected); + } + + [Fact] + public async Task ReportWarningForForeignTasksReturnedFromExpressionBodiedMembers() + { + string test = """ + using System.Threading.Tasks; + + class Tests + { + private static Task task = Task.WhenAll(Task.CompletedTask); + + private static Task ExpressionProperty => {|VSTHRD003:task|}; + + private static Task ExpressionGetter + { + get => {|VSTHRD003:task|}; + } + + private static Task GetTask() + { + Task LocalFunction() => {|VSTHRD003:task|}; + return LocalFunction(); + } + } + """; + + await CSVerify.VerifyAnalyzerAsync(test); + } + + [Fact] + public async Task ReportWarningForNestedCompletedTaskAttributeLookalike() + { + string test = """ + using System; + using System.Threading.Tasks; + + namespace Microsoft.VisualStudio.Threading + { + internal static class Container + { + [AttributeUsage(AttributeTargets.Field)] + internal sealed class CompletedTaskAttribute : Attribute + { + } + } + } + + class Tests + { + [Microsoft.VisualStudio.Threading.Container.CompletedTask] + private static readonly Task CompletedField = Task.Delay(1); + + public Task GetField() => {|VSTHRD003:CompletedField|}; + } + """; + + await CSVerify.VerifyAnalyzerAsync(test); + } + [Fact] public async Task DoNotReportWarningWhenTaskFromResultIsReturnedDirectlyFromMethod() { @@ -1249,7 +1461,7 @@ class Tests { async Task GetTask(TaskCompletionSource tcs) { - await [|tcs.Task|]; + await {|VSTHRD003:tcs.Task|}; } } "; @@ -1303,7 +1515,7 @@ static async Task GetTask() // Assigned, but not to a newly created object. TaskCompletionSource tcs3 = tcs2; - await [|tcs3.Task|]; + await {|VSTHRD003:tcs3.Task|}; } } "; @@ -1419,8 +1631,8 @@ class Tests async Task GetTask() { - await [|this.MyTaskProperty|]; - await [|MyTaskProperty|]; + await {|VSTHRD003:this.MyTaskProperty|}; + await {|VSTHRD003:MyTaskProperty|}; } } "; @@ -1453,6 +1665,60 @@ static async Task ListenAndWait() await CSVerify.VerifyAnalyzerAsync(test); } + [Fact] + public async Task DoNotReportWarningForTopLevelLocals() + { + string test = """ + using System; + using System.Threading; + using System.Threading.Tasks; + + using var cts = new CancellationTokenSource(); + Task loopTask = Task.Run(() => Console.WriteLine("loop"), cts.Token); + Task serverTask = Task.Run(() => Console.WriteLine("server"), cts.Token); + Task exitTask = Task.Run(() => cts.Cancel(), cts.Token); + + await Task.WhenAny(loopTask, serverTask, exitTask); + """; + + await new CSVerify.Test + { + TestState = + { + Sources = { test }, + OutputKind = OutputKind.ConsoleApplication, + }, + }.RunAsync(); + } + + [Fact] + public async Task ReportWarningForForeignFieldInTopLevelStatements() + { + string test = """ + using System.Threading.Tasks; + + await {|VSTHRD003:State.Task|}; + + static class State + { + internal static Task Task = System.Threading.Tasks.Task.Delay(1); + } + """; + + await new CSVerify.Test + { + TestState = + { + Sources = { test }, + OutputKind = OutputKind.ConsoleApplication, + }, + }.RunAsync(); + } + + private static DiagnosticResult Diagnostic() => new("VSTHRD003", DiagnosticSeverity.Warning); + + private static DiagnosticResult InvalidCompletedTaskAttributeDiagnostic() => new("VSTHRD013", DiagnosticSeverity.Warning); + private DiagnosticResult CreateDiagnostic(int line, int column, int length) => - CSVerify.Diagnostic().WithSpan(line, column, line, column + length); + Diagnostic().WithSpan(line, column, line, column + length); }