Remove analyzer cache from public surface area #79808

CyrusNajmabadi · 2025-08-06T13:55:45Z

Followup to #79802.

CyrusNajmabadi · 2025-08-06T13:56:21Z

src/EditorFeatures/Core/EditorConfigSettings/DataProvider/Analyzer/AnalyzerSettingsProvider.cs

-            if (x is null || y is null)
-                return false;
-
-            return x.GetAnalyzerIdAndVersion().GetHashCode() == y.GetAnalyzerIdAndVersion().GetHashCode();


aside: taht this is using HashCodes to determine equality is suppppper sus.

CyrusNajmabadi · 2025-08-06T13:57:03Z

src/EditorFeatures/Core/EditorConfigSettings/DataProvider/Analyzer/AnalyzerSettingsProvider.cs

+    private ImmutableArray<AnalyzerSetting> GetSettings(
+        Solution solution, AnalyzerReference analyzerReference, AnalyzerConfigOptions editorConfigOptions)
    {
-        IEnumerable<DiagnosticAnalyzer> csharpAnalyzers = analyzerReference.GetAnalyzers(LanguageNames.CSharp);


instead of realiing DiagAnalyzers here,e it gets moved into the IDiagService, which can then be the one stop shop for asking these questions in a general fashion. IN the future, IDiagService can then forward thse calls to OOP to do the actual work.

src/EditorFeatures/Core/EditorConfigSettings/DataProvider/Analyzer/AnalyzerSettingsProvider.cs

…yzer/AnalyzerSettingsProvider.cs

…/roslyn into hideAnalyzerCache

CyrusNajmabadi · 2025-08-06T14:00:21Z

src/Features/Core/Portable/Diagnostics/IDiagnosticAnalyzerService.cs

+    // DiagnosticAnalyzerInfoCache AnalyzerInfoCache { get; }
+
+    ImmutableArray<DiagnosticDescriptor> GetDiagnosticDescriptors(
+        Solution solution, AnalyzerReference analyzerReference, string language);


the IDiagService now gets data-oriented apis that were previous based on DiagAnalyzer and exposed off of DiagnosticAnalyzerInfoCache . This will allow these to, in the future, call over to oop.

Note: they will need to be made async first.

CyrusNajmabadi · 2025-08-06T14:00:52Z

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

+            if (x is null || y is null)
+                return false;
+
+            return x.GetAnalyzerIdAndVersion().GetHashCode() == y.GetAnalyzerIdAndVersion().GetHashCode();


if it passes, i'm changing this equality to not use GetHashCode.

CyrusNajmabadi · 2025-08-06T14:01:57Z

src/VisualStudio/Core/Def/TaskList/ProjectExternalErrorReporter.cs

                GetDiagnosticSeverity(error),
                _language,
                new FileLinePositionSpan(project.FilePath ?? "", span: default),
-                AnalyzerInfoCache));


this code used to directly access this cache. now, we just call into IDiagService up front to get the info we used to try to get inside GetDiagnosticData

CyrusNajmabadi · 2025-08-06T14:02:30Z

src/VisualStudio/Core/Def/TaskList/ProjectExternalErrorReporter.cs

        ImmutableArray<string> customTags;

-        if (analyzerInfoCache != null && analyzerInfoCache.TryGetDescriptorForDiagnosticId(errorId, out var descriptor))
+        if (errorIdToDescriptor.TryGetValue(errorId, out var descriptor))


all this was previously trying to do was map from errorid to a DiagDescriptor. so i made an api to do that (in bulk) so this can work in oop as well.

CyrusNajmabadi · 2025-08-06T20:13:28Z

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

+
+        ImmutableArray<DiagnosticDescriptor> GetDiagnosticDescriptors(ImmutableArray<DiagnosticAnalyzer> analyzers)
+            => analyzers.SelectManyAsArray(this._analyzerInfoCache.GetDiagnosticDescriptors);
+    }


all the acutal logic was moved here from the consuming client code on the outside.

CyrusNajmabadi · 2025-08-06T20:15:46Z

@jasonmalinowski this is ready for review.

CyrusNajmabadi · 2025-08-06T20:23:55Z

src/EditorFeatures/Core/EditorConfigSettings/DataProvider/Analyzer/AnalyzerSettingsProvider.cs

-        IEnumerable<DiagnosticAnalyzer> visualBasicAnalyzers = analyzerReference.GetAnalyzers(LanguageNames.VisualBasic);
-        var dotnetAnalyzers = csharpAnalyzers.Intersect(visualBasicAnalyzers, DiagnosticAnalyzerComparer.Instance);
-        csharpAnalyzers = csharpAnalyzers.Except(dotnetAnalyzers, DiagnosticAnalyzerComparer.Instance);
-        visualBasicAnalyzers = visualBasicAnalyzers.Except(dotnetAnalyzers, DiagnosticAnalyzerComparer.Instance);


this logic moved into DiagAnalService

CyrusNajmabadi · 2025-08-06T20:24:03Z

src/Features/Core/Portable/Diagnostics/DiagnosticAnalyzerExtensions.cs

-
-        var descriptors = analyzerReference
-            .GetAnalyzers(project.Language)
-            .SelectManyAsArray(a => diagnosticAnalyzerService.AnalyzerInfoCache.GetDiagnosticDescriptors(a));


this logic moved into DiagAnalService

CyrusNajmabadi · 2025-08-08T12:20:21Z

@jasonmalinowski ptal.

CyrusNajmabadi · 2025-08-08T12:32:21Z

@jasonmalinowski Also, if the feedback is non-critical, let me know and ideally make non-blocking. There are multiple followup PRs, so it would be nicer to just roll the feedback into those.

CyrusNajmabadi · 2025-08-08T12:33:25Z

src/EditorFeatures/Core/EditorConfigSettings/DataProvider/Analyzer/AnalyzerSettingsProvider.cs

-        csharpAnalyzers = csharpAnalyzers.Except(dotnetAnalyzers, DiagnosticAnalyzerComparer.Instance);
-        visualBasicAnalyzers = visualBasicAnalyzers.Except(dotnetAnalyzers, DiagnosticAnalyzerComparer.Instance);
+        var service = solution.Services.GetRequiredService<IDiagnosticAnalyzerService>();
+        var map = service.GetDiagnosticDescriptors(solution, analyzerReference);


general pattern is takign the code that actually operates on DiagAnalyzers and push that into the IDiagnosticAnalyzerService impl. Then the code that processes the computed data stays at the feature level.

jasonmalinowski

This all looks good except for the race being introduced into the external error reporting. Now that we're processing stuff asynchronously and adding it after the call, it would mean a later call to the Clear() methods won't clear them, and worse they might live around once they complete later.

I think a quick use of CancellationTokenSeries should fix up the issue though.

jasonmalinowski · 2025-08-08T23:34:15Z

src/Features/Core/Portable/Diagnostics/IDiagnosticAnalyzerService.cs

 internal interface IDiagnosticAnalyzerService : IWorkspaceService
 {
+    ImmutableArray<DiagnosticDescriptor> GetDiagnosticDescriptors(
+        Solution solution, AnalyzerReference analyzerReference, string language);


Should this be taking a Project rather than Solution + language name?

actually no. this is called from clients who take the same AnalyzerRef (which might be in multiple projects) and try to get both the C# and VB descriptors. Those could pass projects, but it would take more effort on that side to keep track of the originating project. so i didn't do that for now.

jasonmalinowski · 2025-08-08T23:43:08Z

src/EditorFeatures/Core/EditorConfigSettings/DataProvider/Analyzer/AnalyzerSettingsProvider.cs

+
+        return allSettings.ToImmutableAndClear();
+
+        Language ConvertToLanguage(ImmutableArray<string> languages)


Can we do static extension methods on the enum? (OK to not actually do this -- this code might be going away soon enough)

we can. but i also don't mind this just being local here as it is the only place needed.

jasonmalinowski · 2025-08-08T23:46:00Z

src/Features/Core/Portable/Diagnostics/IDiagnosticAnalyzerService.cs

+    /// Returns all the descriptors for all <see cref="DiagnosticAnalyzer"/>s defined within <paramref name="analyzerReference"/>.
+    /// The results are returned in a dictionary where the key is an <see cref="ImmutableArray{T}"/> of languages that descriptor
+    /// is defined for.  This can be <c>[<see cref="LanguageNames.CSharp"/>]</c>, <c>[<see cref="LanguageNames.VisualBasic"/>]</c>,
+    /// or an array containing both languages if the descriptor is defined for both languages.


If the .editorconfig editor was the only thing that needed this crazy form, I'm content to say maybe we just tweak it to use this other form. I appreciate the comment but it's still a bit mind-bending!

If we do need to keep it around, maybe switching it to be a DiagnosticDescriptor -> array of languages it applies to) might make it clearer.

i'm ok doing that later :)
my goals here were: get this OOPable, without changing semantics :)

i def thing later cleanup is a good idea.

Agreed fully.

jasonmalinowski · 2025-08-08T23:47:39Z

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

+    public ImmutableDictionary<string, ImmutableArray<DiagnosticDescriptor>> GetDiagnosticDescriptorsPerReference(Project project)
+        => project.Solution.SolutionState.Analyzers.GetDiagnosticDescriptorsPerReference(this._analyzerInfoCache, project);
+
+    private sealed class DiagnosticAnalyzerComparer : IEqualityComparer<DiagnosticAnalyzer>


Did you get any sense why this was being done this way versus just object identity?

yeah. the more i think about it, the more this seems like nonsense.

src/VisualStudio/Core/Def/TaskList/ProjectExternalErrorReporter.cs

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

…/roslyn into hideAnalyzerCache

src/VisualStudio/Core/Def/TaskList/ProjectExternalErrorReporter.cs

CyrusNajmabadi added 8 commits August 6, 2025 12:24

Remove info cache from diagnostic analyzer surface

a7d732d

Remove analyzer cache from public surface area

05bbdcb

Make private

696c3bd

in progress

d1801ed

Primary constructor

1bcb0ed

Move more code

e0bbc92

cleanup

5166753

Do not expose info cache

7a02598

github-actions bot added the Area-IDE label Aug 6, 2025

dotnet-policy-service bot added the VSCode label Aug 6, 2025

CyrusNajmabadi commented Aug 6, 2025

View reviewed changes

src/EditorFeatures/Core/EditorConfigSettings/DataProvider/Analyzer/AnalyzerSettingsProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi added 4 commits August 6, 2025 15:57

Update src/EditorFeatures/Core/EditorConfigSettings/DataProvider/Anal…

3984720

…yzer/AnalyzerSettingsProvider.cs

Delete

c75a043

Merge branch 'hideAnalyzerCache' of https://github.com/CyrusNajmabadi…

e5da77f

…/roslyn into hideAnalyzerCache

Docs

3bc4d77

CyrusNajmabadi commented Aug 6, 2025

View reviewed changes

DeletE

0e7c0bd

CyrusNajmabadi commented Aug 6, 2025

View reviewed changes

CyrusNajmabadi added 2 commits August 6, 2025 16:02

Cleanup

af64f42

Simplify

6b809de

CyrusNajmabadi mentioned this pull request Aug 6, 2025

Update new IDiagnosticAnalyzerService methods to be async. #79809

Merged

CyrusNajmabadi requested review from AbhitejJohn and jasonmalinowski and removed request for AbhitejJohn August 6, 2025 15:38

CyrusNajmabadi marked this pull request as ready for review August 6, 2025 15:40

CyrusNajmabadi requested a review from a team as a code owner August 6, 2025 15:40

CyrusNajmabadi commented Aug 6, 2025

View reviewed changes

build-analysis bot mentioned this pull request Aug 6, 2025

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Closed

3 tasks

CyrusNajmabadi commented Aug 8, 2025

View reviewed changes

CyrusNajmabadi added 2 commits August 8, 2025 10:00

Merge branch 'equalsNotHashCode' into hideAnalyzerCache

4c29357

Merge branch 'main' into hideAnalyzerCache

866c6b7

jasonmalinowski requested changes Aug 9, 2025

View reviewed changes

CyrusNajmabadi and others added 5 commits August 8, 2025 20:08

Merge remote-tracking branch 'upstream/main' into hideAnalyzerCache

04b6c76

Update src/VisualStudio/Core/Def/TaskList/ProjectExternalErrorReporte…

e9575df

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

Merge branch 'hideAnalyzerCache' of https://github.com/CyrusNajmabadi…

46957f6

…/roslyn into hideAnalyzerCache

Ensure we're actually on the BG

fb0a2b2

Fix race condition

1755482

CyrusNajmabadi requested a review from jasonmalinowski August 9, 2025 00:24

Simplify

96e8e96

jasonmalinowski reviewed Aug 9, 2025

View reviewed changes

src/VisualStudio/Core/Def/TaskList/ProjectExternalErrorReporter.cs Show resolved Hide resolved

jasonmalinowski approved these changes Aug 9, 2025

View reviewed changes

Cancel in flight work

0f1e9db

CyrusNajmabadi enabled auto-merge August 9, 2025 00:38

CyrusNajmabadi merged commit ff3da04 into dotnet:main Aug 9, 2025
24 of 25 checks passed

CyrusNajmabadi deleted the hideAnalyzerCache branch August 9, 2025 01:43

dotnet-policy-service bot added this to the Next milestone Aug 9, 2025

This was referenced Aug 19, 2025

[Automated] PRs inserted in VS build main-10918.105 #79952

Closed

[Automated] PRs inserted in VS build feature.debugger.main-10918.21 #79956

Closed

RikkiGibson modified the milestones: Next, 18.0 P1 Aug 19, 2025

dotnet-bot mentioned this pull request Aug 22, 2025

[Automated] PRs inserted in VS build feature.Wix5-10921.95 #80011

Closed


		return allSettings.ToImmutableAndClear();

		Language ConvertToLanguage(ImmutableArray<string> languages)

Remove analyzer cache from public surface area #79808

Remove analyzer cache from public surface area #79808

Uh oh!

Conversation

CyrusNajmabadi commented Aug 6, 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!

Choose a reason for hiding this comment

Uh oh!

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!

CyrusNajmabadi commented Aug 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Aug 8, 2025

Uh oh!

CyrusNajmabadi commented Aug 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski left a comment

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!

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!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CyrusNajmabadi commented Aug 6, 2025 •

edited

Loading