Suggest CA2007 for await foreach without specified cancellation #6683

CollinAlpert · 2023-06-11T20:00:46Z

This PR extends the DoNotDirectlyAwaitATask analyzer for IAsyncEnumerable. Let me know if there is an easier way to check for an await foreach loop, I found this method to be quite cumbersome.

Fixes #6652.

Youssef1313 · 2023-06-14T21:18:17Z

...nalyzers/Core/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/DoNotDirectlyAwaitATask.cs

+        public virtual (Action<OperationAnalysisContext> Analysis, OperationKind OperationKind) AnalyzeAwaitForEachLoopOperation(INamedTypeSymbol configuredAsyncEnumerable)
+        {
+            return (_ => { }, OperationKind.None);
+        }


Better to make this method return void and do the actual registeration.

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.

Youssef1313 · 2023-06-14T21:23:44Z

...nalyzers/Core/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/DoNotDirectlyAwaitATask.cs

-    [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
-    public sealed class DoNotDirectlyAwaitATaskAnalyzer : DiagnosticAnalyzer
+    [DiagnosticAnalyzer(LanguageNames.VisualBasic)]
+    public class DoNotDirectlyAwaitATaskAnalyzer : DiagnosticAnalyzer


I'd make this abstract, and have VB-derived class.

Youssef1313 · 2023-06-14T21:26:29Z

.../CSharp/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/CSharpDoNotDirectlyAwaitATask.cs

+
+        private static void AnalyzeAwaitForEachLoopOperation(OperationAnalysisContext context, INamedTypeSymbol configuredAsyncEnumerable)
+        {
+            if (context.Operation is IForEachLoopOperation {Syntax: ForEachStatementSyntax {AwaitKeyword.RawKind: not (int)SyntaxKind.None}} forEachOperation


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?

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.

Youssef1313 · 2023-06-14T21:26:53Z

.../CSharp/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/CSharpDoNotDirectlyAwaitATask.cs

+            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));


Suggested change

context.ReportDiagnostic(forEachOperation.Collection.Syntax.CreateDiagnostic(Rule));

context.ReportDiagnostic(forEachOperation.Collection.CreateDiagnostic(Rule));

Youssef1313 · 2023-06-14T21:29:23Z

...nalyzers/Core/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/DoNotDirectlyAwaitATask.cs

@@ -52,7 +53,9 @@ public override void Initialize(AnalysisContext context)
                    return;
                }

-                var configuredAsyncDisposable = context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemRuntimeCompilerServicesConfiguredAsyncDisposable);
+                var wellKnownTypeProvider = WellKnownTypeProvider.GetOrCreate(context.Compilation);


It might be a good idea to move this variable before TryGetTaskTypes, and pass it instead of passing context.Compilation.

sharwell · 2023-06-21T21:36:10Z

My preference is to take earlier pull request #5377 instead of this pull request, provided it remains viable

mavasani · 2023-06-30T09:03:33Z

My preference is to take earlier pull request #5377 instead of this pull request, provided it remains viable

@sharwell Are you able to review #5377 and take it forward?

# Conflicts: # src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.sarif # src/Utilities/Compiler/WellKnownTypeNames.cs

codecov · 2023-07-31T11:50:21Z

Codecov Report

Merging #6683 (ab446be) into main (ac6c703) will decrease coverage by 0.01%.
The diff coverage is 98.90%.

@@            Coverage Diff             @@
##             main    #6683      +/-   ##
==========================================
- Coverage   96.43%   96.42%   -0.01%     
==========================================
  Files        1410     1410              
  Lines      336154   336238      +84     
  Branches    11103    11106       +3     
==========================================
+ Hits       324158   324230      +72     
- Misses       9200     9212      +12     
  Partials     2796     2796

# Conflicts: # src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.sarif

mavasani · 2023-10-11T10:04:52Z

.../CSharp/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/CSharpDoNotDirectlyAwaitATask.cs

+        protected override void RegisterLanguageSpecificChecks(OperationBlockStartAnalysisContext context, INamedTypeSymbol configuredAsyncEnumerable)
+        {
+            context.RegisterOperationAction(ctx => AnalyzeAwaitForEachLoopOperation(ctx, configuredAsyncEnumerable), OperationKind.Loop);
+        }
+
+        private static void AnalyzeAwaitForEachLoopOperation(OperationAnalysisContext context, INamedTypeSymbol configuredAsyncEnumerable)
+        {
+            if (context.Operation is IForEachLoopOperation { IsAsynchronous: true, Collection.Type: not null } forEachOperation
+                && !forEachOperation.Collection.Type.OriginalDefinition.Equals(configuredAsyncEnumerable, SymbolEqualityComparer.Default))
+            {
+                context.ReportDiagnostic(forEachOperation.Collection.CreateDiagnostic(Rule));
+            }
+        }


@CollinAlpert all the code here seems to be compilable at the shared layer. I understand only C# supports async foreach loops, but even having this code in the shared layer will do the right thing by bailing out early for VB. I would move this code down to the shared layer and we can definitely conclude that this PR is preferable over #5377

mavasani · 2023-10-16T04:10:05Z

Thanks!

Suggest CA2007 for await foreach without specified cancellation.

afd83a5

CollinAlpert requested a review from a team as a code owner June 11, 2023 20:00

github-actions bot added the Community label Jun 11, 2023

Youssef1313 reviewed Jun 14, 2023

View reviewed changes

CollinAlpert added 2 commits June 20, 2023 18:46

PR comments.

97c69a2

Merge branch 'master' into issue_6652

3851cb0

CollinAlpert added 3 commits June 23, 2023 18:41

Merge branch 'master' into issue_6652

8d25a62

PR fixes.

30dbd89

Merge branch 'master' into issue_6652

787a519

CollinAlpert added 3 commits July 31, 2023 13:00

Merge branch 'master' into issue_6652

3b58d2f

# Conflicts: # src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.sarif # src/Utilities/Compiler/WellKnownTypeNames.cs

Remove warning.

169dcbb

Add file header

6bfef06

CollinAlpert added 2 commits July 31, 2023 21:10

Use IsAsynchronous property

ccb6a77

Merge branch 'master' into issue_6652

9e0fff8

# Conflicts: # src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.sarif

mavasani reviewed Oct 11, 2023

View reviewed changes

CollinAlpert added 2 commits October 15, 2023 21:28

Merge branch 'master' into issue_6652

231edb2

Apply PR comments

ab446be

mavasani approved these changes Oct 16, 2023

View reviewed changes

mavasani merged commit b87a634 into dotnet:main Oct 16, 2023
11 checks passed

CollinAlpert deleted the issue_6652 branch October 17, 2023 10:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest CA2007 for await foreach without specified cancellation #6683

Suggest CA2007 for await foreach without specified cancellation #6683

CollinAlpert commented Jun 11, 2023

Youssef1313 Jun 14, 2023 •

edited

Loading

Youssef1313 Jun 14, 2023

Youssef1313 Jun 14, 2023

mavasani Jun 19, 2023

CollinAlpert Jul 31, 2023

Youssef1313 Jun 14, 2023

Youssef1313 Jun 14, 2023

sharwell commented Jun 21, 2023

mavasani commented Jun 30, 2023

codecov bot commented Jul 31, 2023 •

edited

Loading

mavasani Oct 11, 2023

CollinAlpert Oct 15, 2023

mavasani commented Oct 16, 2023

	context.ReportDiagnostic(forEachOperation.Collection.Syntax.CreateDiagnostic(Rule));
	context.ReportDiagnostic(forEachOperation.Collection.CreateDiagnostic(Rule));

Suggest CA2007 for await foreach without specified cancellation #6683

Suggest CA2007 for await foreach without specified cancellation #6683

Conversation

CollinAlpert commented Jun 11, 2023

Youssef1313 Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell commented Jun 21, 2023

mavasani commented Jun 30, 2023

codecov bot commented Jul 31, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani commented Oct 16, 2023

Youssef1313 Jun 14, 2023 •

edited

Loading

codecov bot commented Jul 31, 2023 •

edited

Loading