Update cancellation token handling behavior for certain analyzer APIs #69108

CyrusNajmabadi · 2023-07-19T15:51:02Z

The old API was busted (and never used) where a cancellation token was provided at creation, and then potentially passed along if you called particular methods. This was highly unsafe as said times had no relation to each other, and you could provide a cancellation token at one point unrelated to the one used to compute diagnostics.

We fixed all our own usages everywhere to do the right thing. Specifically, to not pass a cancellation token to the constructor, and to always pass a correct one to the GetXXXDiagnostics methods. This PR just goes and adds appropriate overloads, obsolete warnings, and editorbrowsable hidings, to ensure that any other consumers do the right thing as well.

This change is entirely binary compatible with existing releases, using our standard patterns for preserving (but hiding) bad apis, and introducing new APIs that will be preferred in source.

CyrusNajmabadi · 2023-07-19T15:54:24Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs

@@ -26,7 +27,6 @@ public class CompilationWithAnalyzers
        private readonly ImmutableArray<DiagnosticAnalyzer> _analyzers;
        private readonly ImmutableArray<DiagnosticSuppressor> _suppressors;
        private readonly CompilationWithAnalyzersOptions _analysisOptions;
-        private readonly CancellationToken _cancellationToken;


bad captured CT.

CyrusNajmabadi · 2023-07-19T15:54:37Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs

-        public CancellationToken CancellationToken => _cancellationToken;
+        [Obsolete("This CancellationToken is always 'default'", error: false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
+        public CancellationToken CancellationToken => default;


kept for binary compat. but if anyone is using this, they're warned to stop.

CyrusNajmabadi · 2023-07-19T15:55:41Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs

+        public CompilationWithAnalyzers(Compilation compilation, ImmutableArray<DiagnosticAnalyzer> analyzers, AnalyzerOptions? options, CancellationToken cancellationToken)
+            : this(compilation, analyzers, options)
+        {
+        }


kept for binary compat. if anyone is using this, they're warning to stop.

CyrusNajmabadi · 2023-07-19T15:57:06Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs

@@ -225,15 +228,16 @@ private void VerifyAdditionalFile(AdditionalText file)
        /// <summary>
        /// Returns diagnostics produced by all <see cref="Analyzers"/>.
        /// </summary>
+        [EditorBrowsable(EditorBrowsableState.Never)]
        public Task<ImmutableArray<Diagnostic>> GetAnalyzerDiagnosticsAsync()


can't warn on this, as we often have APIs where user provided cancellationtokens are optional, and we want to allow users to call those without a CT for simple programming purposes. We do hide this though, so that the user only sees the method below, which follows our standard api pattern of an actually 'optional' parameter.

CyrusNajmabadi · 2023-07-19T15:57:24Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/DiagnosticAnalyzerExtensions.cs

+        /// <inheritdoc cref="WithAnalyzers(Compilation, ImmutableArray{DiagnosticAnalyzer}, AnalyzerOptions?)"/>
+        [Obsolete("Use WithAnalyzers overload without a cancellation token", error: false)]
+        [EditorBrowsable(EditorBrowsableState.Never)]
+        public static CompilationWithAnalyzers WithAnalyzers(this Compilation compilation, ImmutableArray<DiagnosticAnalyzer> analyzers, AnalyzerOptions? options, CancellationToken cancellationToken)


kept for binary compat. if anyone is using this, they're warned to stop.

💡 Consider adding this to the banned APIs in dotnet/roslyn-analyzers so it applies even to users who reference old versions of the compiler in their analyzer projects

CyrusNajmabadi · 2023-07-19T15:58:31Z

@dotnet/roslyn-compiler ptal.

src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs

sharwell · 2023-07-19T17:00:02Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/DiagnosticAnalyzerExtensions.cs

        /// <summary>
        /// Returns a new compilation with attached diagnostic analyzers.
        /// </summary>
        /// <param name="compilation">Compilation to which analyzers are to be added.</param>
        /// <param name="analyzers">The set of analyzers to include in future analyses.</param>
        /// <param name="options">Options that are passed to analyzers.</param>
-        /// <param name="cancellationToken">A cancellation token that can be used to abort analysis.</param>
-        public static CompilationWithAnalyzers WithAnalyzers(this Compilation compilation, ImmutableArray<DiagnosticAnalyzer> analyzers, AnalyzerOptions? options = null, CancellationToken cancellationToken = default)
+#pragma warning disable RS0027 // API with optional parameter(s) should have the most parameters amongst its public overloads


💡 Consider using SuppressMessage for this one since it has a space specifically for the justification

src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs

…Analyzers.cs Co-authored-by: Sam Harwell <[email protected]>

See dotnet/roslyn#69108

sharwell · 2023-07-19T17:31:09Z

I submitted dotnet/roslyn-sdk#1107 as a supporting change

CyrusNajmabadi · 2023-07-24T19:46:34Z

@dotnet/roslyn-compiler ptal.

CyrusNajmabadi · 2023-07-25T20:08:07Z

https://github.com/orgs/dotnet/teams/roslyn-compiler ptal.

CyrusNajmabadi · 2023-07-25T20:08:22Z

@333fred @RikkiGibson can you ptal?

jcouv · 2023-07-26T16:16:08Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/DiagnosticAnalyzerExtensions.cs

 using System.Threading;

 namespace Microsoft.CodeAnalysis.Diagnostics
 {
    public static class DiagnosticAnalyzerExtensions
    {
+        /// <inheritdoc cref="WithAnalyzers(Compilation, ImmutableArray{DiagnosticAnalyzer}, AnalyzerOptions?)"/>
+        [Obsolete("Use WithAnalyzers overload without a cancellation token", error: false)]


We're probably okay since this is just a warning, but let's confirm that we don't need to document this as a break

Tagging @333fred, but i'm virtually certain we don't need to document. There is no actual 'break' here. Both binary and source bulds will continue to be fine. Just that source builds may get a warning. We obsolete things like that all the time without documenting.

jcouv

LGTM Thanks (iteration 6)

Update cancellation token handling behavior for certain analyzer APIs

ba3f4ba

CyrusNajmabadi requested review from a team as code owners July 19, 2023 15:51

dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 19, 2023

CyrusNajmabadi mentioned this pull request Jul 19, 2023

Expose constructor that allows for AnalysisOptions and a CancellationToken. #65051

Closed

CyrusNajmabadi commented Jul 19, 2023

View reviewed changes

Warnings

3fd70d8

CyrusNajmabadi commented Jul 19, 2023

View reviewed changes

pragmas

34d5a92

mavasani approved these changes Jul 19, 2023

View reviewed changes

sharwell reviewed Jul 19, 2023

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs Outdated Show resolved Hide resolved

sharwell reviewed Jul 19, 2023

View reviewed changes

sharwell requested changes Jul 19, 2023

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs Show resolved Hide resolved

sharwell reviewed Jul 19, 2023

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs Outdated Show resolved Hide resolved

sharwell reviewed Jul 19, 2023

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs Outdated Show resolved Hide resolved

sharwell reviewed Jul 19, 2023

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs Show resolved Hide resolved

CyrusNajmabadi and others added 3 commits July 19, 2023 10:21

Update src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWith…

a2d8008

…Analyzers.cs Co-authored-by: Sam Harwell <[email protected]>

Update src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWith…

702a18f

…Analyzers.cs Co-authored-by: Sam Harwell <[email protected]>

Update src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWith…

2ba9e27

…Analyzers.cs Co-authored-by: Sam Harwell <[email protected]>

sharwell added a commit to sharwell/roslyn-sdk that referenced this pull request Jul 19, 2023

Account for new method of cancelling diagnostics

727da98

See dotnet/roslyn#69108

sharwell approved these changes Jul 19, 2023

View reviewed changes

sharwell mentioned this pull request Jul 19, 2023

Account for new method of cancelling diagnostics dotnet/roslyn-sdk#1107

Merged

333fred approved these changes Jul 25, 2023

View reviewed changes

jcouv reviewed Jul 26, 2023

View reviewed changes

jcouv approved these changes Jul 26, 2023

View reviewed changes

jcouv self-assigned this Jul 26, 2023

jcouv added this to the 17.8 milestone Jul 26, 2023

jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Jul 26, 2023

Merge branch 'main' into analyzerCancellation

9ee9e28

CyrusNajmabadi enabled auto-merge (squash) July 26, 2023 16:42

CyrusNajmabadi merged commit 0dd35b3 into dotnet:main Jul 26, 2023
25 checks passed

ghost modified the milestones: 17.8, Next Jul 26, 2023

CyrusNajmabadi deleted the analyzerCancellation branch July 26, 2023 19:15

allisonchou mentioned this pull request Aug 2, 2023

[Automated] PRs inserted in VS build main-34002.289 #69347

Closed

dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update cancellation token handling behavior for certain analyzer APIs #69108

Update cancellation token handling behavior for certain analyzer APIs #69108

CyrusNajmabadi commented Jul 19, 2023 •

edited by sharwell

Loading

CyrusNajmabadi Jul 19, 2023

CyrusNajmabadi Jul 19, 2023

CyrusNajmabadi Jul 19, 2023

CyrusNajmabadi Jul 19, 2023

CyrusNajmabadi Jul 19, 2023 •

edited by sharwell

Loading

sharwell Jul 19, 2023

CyrusNajmabadi commented Jul 19, 2023

sharwell Jul 19, 2023

sharwell commented Jul 19, 2023

CyrusNajmabadi commented Jul 24, 2023

CyrusNajmabadi commented Jul 25, 2023

CyrusNajmabadi commented Jul 25, 2023

jcouv Jul 26, 2023

CyrusNajmabadi Jul 26, 2023

jcouv left a comment

Update cancellation token handling behavior for certain analyzer APIs #69108

Update cancellation token handling behavior for certain analyzer APIs #69108

Conversation

CyrusNajmabadi commented Jul 19, 2023 • edited by sharwell 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

CyrusNajmabadi Jul 19, 2023 • edited by sharwell Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jul 19, 2023

Choose a reason for hiding this comment

sharwell commented Jul 19, 2023

CyrusNajmabadi commented Jul 24, 2023

CyrusNajmabadi commented Jul 25, 2023

CyrusNajmabadi commented Jul 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jul 19, 2023 •

edited by sharwell

Loading

CyrusNajmabadi Jul 19, 2023 •

edited by sharwell

Loading