Remove CompilationPair abstraction in diagnostic analyzer subsystem by CyrusNajmabadi · Pull Request #80141 · dotnet/roslyn

CyrusNajmabadi · 2025-09-04T16:33:14Z

Followup to #80200
API change request is here: #80143

Build: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=12380432&view=results
PR: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/670048

CyrusNajmabadi · 2025-09-04T16:34:59Z

src/EditorFeatures/CSharpTest/Formatting/CodeCleanupTests.cs

-
            namespace A
            {
+                using System;


this is the correct behavior. The test here is testing " 'usings' should be INSIDE the namespace, and it is an ERROR otherwise". So it was a bug this wasn't moving in in CodeCleanup.

Note: this differs from teh tests taht validate that nothing change when the option suggestions the change, but the severity is set to 'hidden'. in that case we do not make any change.

Do you have a sense why this changed? (I agree the behavior looks right, but this means we had a bug somewhere...)

When i was debugging through, teh wrong options were coming through, and it meant that the default severity was getting pulled in (which is 'hidden'). Because of this, the codecleanup layer doesn't actually want to make a change due to 'hidden' effectively being equivalent to 'the user didn't actually set anything themselves'.

CyrusNajmabadi · 2025-09-04T16:44:42Z

Draft until API is reviewed and approved.

CyrusNajmabadi · 2025-09-04T19:01:19Z

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

+        {
+            // No specific options factory.  Can use the shared context.
+            if (_analyzerToCachedOptions is null)
+                return context;


note: this is the common case for those not referencing hte roslyn sdk. So there should be almost no overhead in that common case.

CyrusNajmabadi · 2025-09-04T19:03:18Z

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

+        internal SymbolAnalysisContext WithOptions(AnalyzerOptions options)
+            => this.Options == options
+                ? this
+                : new(this.Symbol, this.Compilation, options, _reportDiagnostic, _isSupportedDiagnostic, this.IsGeneratedCode, this.FilterTree, this.FilterSpan, this.CancellationToken);


note: these context types are structs. So this is not actually allocating a new instance on the heap. Just returning a tweaked version with the right options to pass along.

CyrusNajmabadi · 2025-09-04T19:05:58Z

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

+        /// from the shared instance provided in <see cref="Options"/>.  If <see langword="null"/> then <see cref="Options"/>
+        /// will be used for all analyzers.
+        /// </summary>
+        public Func<DiagnosticAnalyzer, AnalyzerOptions>? AnalyzerSpecificOptionsFactory


note: this prop doesn't need to be public (though that matches the reest here). The API review covers if we should expose this (since it was a constructor param) or if we should keep it internally since we hvve no actual public consumers.

CyrusNajmabadi · 2025-09-04T19:06:05Z

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

+           bool logAnalyzerExecutionTime,
+           bool reportSuppressedDiagnostics,
+           Func<Exception, bool>? analyzerExceptionFilter,
+           Func<DiagnosticAnalyzer, AnalyzerOptions>? analyzerSpecificOptionsFactory)


new public API that needs API review.

CyrusNajmabadi · 2025-09-04T19:06:36Z

.../Core/Portable/Diagnostics/Service/DiagnosticAnalyzerService_CompilationWithAnalyzersPair.cs

+                analyzerExceptionFilter: exceptionFilter,
+                analyzerSpecificOptionsFactory));
+
+            (AnalyzerOptions sharedOptions, Func<DiagnosticAnalyzer, AnalyzerOptions>? analyzerSpecificOptionsFactory) GetOptions()


this is hte meat of the change on the IDE side.

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

.../Core/Portable/Diagnostics/Service/DiagnosticAnalyzerService_CompilationWithAnalyzersPair.cs

CyrusNajmabadi · 2025-09-04T21:35:26Z

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

+                var map = new Dictionary<DiagnosticAnalyzer, AnalyzerOptions>(ReferenceEqualityComparer.Instance);
+
+                // Deduping map for the distinct AnalyzerConfigOptionsProvider we get back from analyzerSpecificOptionsFactory
+                var optionsProviderToOptions = new Dictionary<AnalyzerConfigOptionsProvider, AnalyzerOptions>(ReferenceEqualityComparer.Instance);


note: this deduping is tested. we make sure that if two analyzers return the same AnalyzerConfigOptionsProvider that they will then get passed the exact same AnalyzerOptions instance in callbacks.

CyrusNajmabadi · 2025-09-09T18:53:35Z

API change was approved. @jasonmalinowski @dotnet/roslyn-compiler for eyes.

CyrusNajmabadi · 2025-09-09T18:58:37Z

src/Compilers/CSharp/Test/Emit3/Diagnostics/DiagnosticAnalyzerTests.cs

            diagnostics.Verify(Diagnostic("ID0001", "B").WithLocation(1, 8));
        }
+
+        private sealed class OptionsOverrideDiagnosticAnalyzer(AnalyzerConfigOptionsProvider customOptions) : DiagnosticAnalyzer


these tests validate that we see the different options through all the callbacks.

CyrusNajmabadi · 2025-09-09T18:59:47Z

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt

 Microsoft.CodeAnalysis.CommandLineResource.LinkedResourceFileName.get -> string?
 Microsoft.CodeAnalysis.CommandLineResource.ResourceName.get -> string!
 Microsoft.CodeAnalysis.Compilation.EmitDifference(Microsoft.CodeAnalysis.Emit.EmitBaseline! baseline, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.Emit.SemanticEdit>! edits, System.Func<Microsoft.CodeAnalysis.ISymbol!, bool>! isAddedSymbol, System.IO.Stream! metadataStream, System.IO.Stream! ilStream, System.IO.Stream! pdbStream, Microsoft.CodeAnalysis.Emit.EmitDifferenceOptions options, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.Emit.EmitDifferenceResult!
+Microsoft.CodeAnalysis.Diagnostics.CompilationWithAnalyzersOptions.CompilationWithAnalyzersOptions(Microsoft.CodeAnalysis.Diagnostics.AnalyzerOptions? options, System.Action<System.Exception!, Microsoft.CodeAnalysis.Diagnostics.DiagnosticAnalyzer!, Microsoft.CodeAnalysis.Diagnostic!>? onAnalyzerException, bool concurrentAnalysis, bool logAnalyzerExecutionTime, bool reportSuppressedDiagnostics, System.Func<System.Exception!, bool>? analyzerExceptionFilter, System.Func<Microsoft.CodeAnalysis.Diagnostics.DiagnosticAnalyzer!, Microsoft.CodeAnalysis.Diagnostics.AnalyzerConfigOptionsProvider!>? getAnalyzerConfigOptionsProvider) -> void


only public change is this one constructor.

src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.txt

…thApiChange

.../Core/Portable/Diagnostics/Service/DiagnosticAnalyzerService_CompilationWithAnalyzersPair.cs

jasonmalinowski · 2025-09-12T22:24:22Z

.../Core/Portable/Diagnostics/Service/DiagnosticAnalyzerService_CompilationWithAnalyzersPair.cs

I assume you'll be renaming the file as a follow-up.

Yup. I'm even working on making the "make filename match type" fixer able to fix-all. So i can do these in bulk.

.../Core/Portable/Diagnostics/Service/DiagnosticAnalyzerService_CompilationWithAnalyzersPair.cs

src/Features/Core/Portable/Diagnostics/Service/DocumentAnalysisExecutor.cs

jasonmalinowski · 2025-09-12T22:38:05Z

src/Workspaces/Core/Portable/Diagnostics/Extensions.cs

        CancellationToken cancellationToken)
    {
-        var hostAnalyzers = documentAnalysisScope?.HostAnalyzers ?? compilationWithAnalyzers.HostAnalyzers;
+        var hostAnalyzers = documentAnalysisScope?.Analyzers ?? compilationWithAnalyzers.Analyzers;


Not sure if local needs a rename, because it's suspicious it was 'hostAnalyzers' before?

will do. i agree it's suspicious. can you think of a reason we'd only be using hostanalyzers for suppression analyzers? @mavasani do you perhaps knows?

i'm tempted to leave this as is (with the rename). but if there's a sound reason for only using host analyzers, we could always filter these down to those.

Unfortunately, we don't have any tests indicating if this is intentional or errant.

jasonmalinowski · 2025-09-12T22:39:25Z

src/EditorFeatures/CSharpTest/Formatting/CodeCleanupTests.cs

-
            namespace A
            {
+                using System;


Do you have a sense why this changed? (I agree the behavior looks right, but this means we had a bug somewhere...)

…zerService_CompilationWithAnalyzersPair.cs Co-authored-by: Jason Malinowski <jason@jason-m.com>

…yrusNajmabadi/roslyn into noCompilationPairWithApiChange

CyrusNajmabadi

comment

src/Features/Core/Portable/Diagnostics/Service/DocumentAnalysisExecutor.cs

CyrusNajmabadi · 2025-09-12T23:20:12Z

src/Features/Core/Portable/Diagnostics/Service/DocumentAnalysisExecutor.cs

-                RoslynDebug.Assert(AnalysisScope.TextDocument is Document);
+                Contract.ThrowIfFalse(analyzer.IsCompilerAnalyzer());
+                Contract.ThrowIfNull(_compilationWithAnalyzers);
+                Contract.ThrowIfFalse(_compilationBasedAnalyzersInAnalysisScope.Contains(analyzer));


we even already had this assert here. so it massively simplifies the code below.

jasonmalinowski

I sign off; obviously that question is still pending but I'm not sure I can figure that one out...

ToddGrun · 2025-09-13T16:55:31Z

.../Core/Portable/Diagnostics/Service/DiagnosticAnalyzerService_CompilationWithAnalyzersPair.cs

-            filteredHostAnalyzers = filteredHostAnalyzers.AddRange(filteredProjectSuppressors);
+            // Ensure we filter out DocumentDiagnosticAnalyzers (they're used to get diagnostics, without involving a
+            // compilation), and also ensure the list has no duplicates.
+            analyzers = analyzers.WhereAsArray(static a => !a.IsWorkspaceDiagnosticAnalyzer()).Distinct();


Distinct

Huh, I would have thought the IA.Distinct extension method would have used a PooledHashSet (in the default comparer case)

.../Core/Portable/Diagnostics/Service/DiagnosticAnalyzerService_CompilationWithAnalyzersPair.cs

…thApiChange

github-actions bot added the Area-Analyzers label Sep 4, 2025

dotnet-policy-service bot added VSCode Needs API Review Needs to be reviewed by the API review council labels Sep 4, 2025

CyrusNajmabadi commented Sep 4, 2025

View reviewed changes

CyrusNajmabadi mentioned this pull request Sep 4, 2025

Provide a way for specific DiagnosticAnalyzers to get specific AnalyzerOptions when running analyzers #80143

Closed

CyrusNajmabadi changed the title ~~No compilation pair with api change~~ Remove CompilationPair abstraction in diagnostic analyzer subsystem (with api change) Sep 4, 2025

CyrusNajmabadi requested a review from jasonmalinowski September 4, 2025 17:01

CyrusNajmabadi commented Sep 4, 2025

View reviewed changes

jasonmalinowski reviewed Sep 4, 2025

View reviewed changes

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

jasonmalinowski reviewed Sep 4, 2025

View reviewed changes

.../Core/Portable/Diagnostics/Service/DiagnosticAnalyzerService_CompilationWithAnalyzersPair.cs Outdated Show resolved Hide resolved

CyrusNajmabadi requested a review from jasonmalinowski September 4, 2025 21:20

CyrusNajmabadi commented Sep 4, 2025

View reviewed changes

CyrusNajmabadi mentioned this pull request Sep 8, 2025

Remove CompilationPair abstraction in diagnostic analyzer subsystem. #80086

Merged

CyrusNajmabadi marked this pull request as ready for review September 9, 2025 18:53

CyrusNajmabadi requested review from a team as code owners September 9, 2025 18:53

CyrusNajmabadi commented Sep 9, 2025

View reviewed changes

src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.txt Outdated Show resolved Hide resolved

CyrusNajmabadi marked this pull request as draft September 9, 2025 21:11

CyrusNajmabadi added 3 commits September 12, 2025 12:12

Merge branch 'main' into noCompilationPairWithApiChange

b26c7e8

Merge remote-tracking branch 'upstream/main' into noCompilationPairWi…

3d184c9

…thApiChange

Restore

a2fc8dd

CyrusNajmabadi removed request for a team September 12, 2025 22:06

jasonmalinowski reviewed Sep 12, 2025

View reviewed changes

CyrusNajmabadi and others added 4 commits September 13, 2025 01:00

Update src/Features/Core/Portable/Diagnostics/Service/DiagnosticAnaly…

0031ef9

…zerService_CompilationWithAnalyzersPair.cs Co-authored-by: Jason Malinowski <jason@jason-m.com>

Remove unneeded code

f662360

Merge branch 'noCompilationPairWithApiChange' of https://github.com/C…

f03660b

…yrusNajmabadi/roslyn into noCompilationPairWithApiChange

Simplify

217d694

CyrusNajmabadi commented Sep 12, 2025

View reviewed changes

src/Features/Core/Portable/Diagnostics/Service/DocumentAnalysisExecutor.cs Outdated Show resolved Hide resolved

Simplify

0d9b372

CyrusNajmabadi commented Sep 12, 2025

View reviewed changes

jasonmalinowski approved these changes Sep 12, 2025

View reviewed changes

Move helpers

53a29ac

jasonmalinowski approved these changes Sep 12, 2025

View reviewed changes

Filter back down to host only analyzers

defbc18

ToddGrun reviewed Sep 13, 2025

View reviewed changes

.../Core/Portable/Diagnostics/Service/DiagnosticAnalyzerService_CompilationWithAnalyzersPair.cs Outdated Show resolved Hide resolved

CyrusNajmabadi added 3 commits September 13, 2025 23:30

Merge remote-tracking branch 'upstream/main' into noCompilationPairWi…

29c8ff4

…thApiChange

Move inside

36daa2a

Merge branch 'main' into noCompilationPairWithApiChange

fb385c2

CyrusNajmabadi merged commit 95b192d into dotnet:main Sep 16, 2025
25 checks passed

CyrusNajmabadi deleted the noCompilationPairWithApiChange branch September 16, 2025 11:54

dotnet-policy-service bot added this to the Next milestone Sep 16, 2025

akhera99 modified the milestones: Next, 18.0 P1, 18.0 P2 Sep 22, 2025

Conversation

CyrusNajmabadi commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Sep 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Sep 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

ToddGrun Sep 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyrusNajmabadi commented Sep 4, 2025 •

edited

Loading

jasonmalinowski Sep 12, 2025 •

edited

Loading

jasonmalinowski Sep 12, 2025 •

edited

Loading

ToddGrun Sep 13, 2025 •

edited

Loading