-
Notifications
You must be signed in to change notification settings - Fork 459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Suggest CA2007 for await foreach without specified cancellation #6683
Changes from 1 commit
afd83a5
97c69a2
3851cb0
8d25a62
30dbd89
787a519
3b58d2f
169dcbb
6bfef06
ccb6a77
9e0fff8
231edb2
ab446be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,31 @@ | ||||||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | ||||||
|
||||||
using System; | ||||||
using Analyzer.Utilities.Extensions; | ||||||
using Microsoft.CodeAnalysis; | ||||||
using Microsoft.CodeAnalysis.CSharp; | ||||||
using Microsoft.CodeAnalysis.CSharp.Syntax; | ||||||
using Microsoft.CodeAnalysis.Diagnostics; | ||||||
using Microsoft.CodeAnalysis.Operations; | ||||||
using Microsoft.CodeQuality.Analyzers.ApiDesignGuidelines; | ||||||
|
||||||
namespace Microsoft.CodeQuality.CSharp.Analyzers.ApiDesignGuidelines | ||||||
{ | ||||||
[DiagnosticAnalyzer(LanguageNames.CSharp)] | ||||||
public sealed class CSharpDoNotDirectlyAwaitATask : DoNotDirectlyAwaitATaskAnalyzer | ||||||
{ | ||||||
public override (Action<OperationAnalysisContext> Analysis, OperationKind OperationKind) AnalyzeAwaitForEachLoopOperation(INamedTypeSymbol configuredAsyncEnumerable) | ||||||
{ | ||||||
return (ctx => AnalyzeAwaitForEachLoopOperation(ctx, configuredAsyncEnumerable), OperationKind.Loop); | ||||||
} | ||||||
|
||||||
private static void AnalyzeAwaitForEachLoopOperation(OperationAnalysisContext context, INamedTypeSymbol configuredAsyncEnumerable) | ||||||
{ | ||||||
if (context.Operation is IForEachLoopOperation {Syntax: ForEachStatementSyntax {AwaitKeyword.RawKind: not (int)SyntaxKind.None}} forEachOperation | ||||||
&& !forEachOperation.Collection.Type.OriginalDefinition.Equals(configuredAsyncEnumerable, SymbolEqualityComparer.Default)) | ||||||
{ | ||||||
context.ReportDiagnostic(forEachOperation.Collection.Syntax.CreateDiagnostic(Rule)); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
} | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Collections.Immutable; | ||
using System.Linq; | ||
using Analyzer.Utilities; | ||
|
@@ -16,8 +17,8 @@ | |
/// <summary> | ||
/// CA2007: <inheritdoc cref="DoNotDirectlyAwaitATaskTitle"/> | ||
/// </summary> | ||
[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)] | ||
public sealed class DoNotDirectlyAwaitATaskAnalyzer : DiagnosticAnalyzer | ||
[DiagnosticAnalyzer(LanguageNames.VisualBasic)] | ||
Check failure on line 20 in src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/DoNotDirectlyAwaitATask.cs
|
||
public class DoNotDirectlyAwaitATaskAnalyzer : DiagnosticAnalyzer | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd make this abstract, and have VB-derived class. |
||
{ | ||
internal const string RuleId = "CA2007"; | ||
|
||
|
@@ -52,7 +53,9 @@ | |
return; | ||
} | ||
|
||
var configuredAsyncDisposable = context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemRuntimeCompilerServicesConfiguredAsyncDisposable); | ||
var wellKnownTypeProvider = WellKnownTypeProvider.GetOrCreate(context.Compilation); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be a good idea to move this variable before |
||
var configuredAsyncDisposable = wellKnownTypeProvider.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemRuntimeCompilerServicesConfiguredAsyncDisposable); | ||
var configuredAsyncEnumerable = wellKnownTypeProvider.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemRuntimeCompilerServicesConfiguredCancelableAsyncEnumerable); | ||
|
||
context.RegisterOperationBlockStartAction(context => | ||
{ | ||
|
@@ -77,11 +80,22 @@ | |
context.RegisterOperationAction(context => AnalyzeUsingOperation(context, configuredAsyncDisposable), OperationKind.Using); | ||
context.RegisterOperationAction(context => AnalyzeUsingDeclarationOperation(context, configuredAsyncDisposable), OperationKindEx.UsingDeclaration); | ||
} | ||
|
||
if (configuredAsyncEnumerable is not null) | ||
{ | ||
var (analysis, operationKind) = AnalyzeAwaitForEachLoopOperation(configuredAsyncEnumerable); | ||
context.RegisterOperationAction(analysis, operationKind); | ||
} | ||
} | ||
}); | ||
}); | ||
} | ||
|
||
public virtual (Action<OperationAnalysisContext> Analysis, OperationKind OperationKind) AnalyzeAwaitForEachLoopOperation(INamedTypeSymbol configuredAsyncEnumerable) | ||
{ | ||
return (_ => { }, OperationKind.None); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better to make this method return protected virtual void RegisterLanguageSpecificChecks(OperationBlockStartAnalysisContext context, INamedTypeSymbol configuredAsyncEnumerable)
{
} and in C# implementation: protected override void RegisterLanguageSpecificChecks(OperationBlockStartAnalysisContext context, INamedTypeSymbol configuredAsyncEnumerable)
{
context.RegisterOperationAction(context => AnalyzeAwaitForEachLoopOperation(context, configuredAsyncEnumerable), OperationKind.Loop);
} Note: I'm writing directly in GitHub, so I could have made an error. |
||
|
||
private static void AnalyzeAwaitOperation(OperationAnalysisContext context, ImmutableArray<INamedTypeSymbol> taskTypes) | ||
{ | ||
var awaitExpression = (IAwaitOperation)context.Operation; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't recall what version of Roslyn we do use in this repo, but is this API not available? https://learn.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.operations.iforeachloopoperation.isasynchronous?view=roslyn-dotnet-3.9.0#microsoft-codeanalysis-operations-iforeachloopoperation-isasynchronous
Would it make sense to use it via lightup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should upgrade to newer version of Microsoft.CodeAnalysis package reference for the NetAnalyzers project. We don't really need to support this analyzer package on very old versions of VS/compiler. I'll create a separate PR for this, and remove any old lightup code that was added to work around this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.