Add support for distinct LSP diagnostic categories in LSP Pull Diagnostics. by CyrusNajmabadi · Pull Request #65985 · dotnet/roslyn

CyrusNajmabadi · 2022-12-13T22:13:44Z

This allows us to coordinate with the client to support categories like "Compiler Syntax", "Compiler Semantic", "Task List", etc. categories. This will greatly speed up LSP pull diagnostics as these requests can operate concurrently, and can be displayed to the user once individually complete, as opposed to waiting for us to compute all of them.

CyrusNajmabadi · 2022-12-13T22:16:47Z

        /// </summary>
-        Task<ImmutableArray<DiagnosticData>> GetDiagnosticsForIdsAsync(Solution solution, ProjectId? projectId = null, DocumentId? documentId = null, ImmutableHashSet<string>? diagnosticIds = null, bool includeSuppressedDiagnostics = false, CancellationToken cancellationToken = default);
+        Task<ImmutableArray<DiagnosticData>> GetDiagnosticsForIdsAsync(
+            Solution solution, ProjectId? projectId = null, DocumentId? documentId = null, ImmutableHashSet<string>? diagnosticIds = null, bool includeSuppressedDiagnostics = false, DiagnosticKind diagnosticKind = DiagnosticKind.All, CancellationToken cancellationToken = default);


@mavasani need eyes on htis

CyrusNajmabadi · 2022-12-13T22:17:00Z

+                            AppendDiagnostics(analysisResult.GetDocumentDiagnostics(documentId, AnalysisKind.Semantic));
+
+                        if (includeAll)
+                            AppendDiagnostics(analysisResult.GetDocumentDiagnostics(documentId, AnalysisKind.NonLocal));


need eyes on this.

actually, i just realized there's a simpler way to do this most likely.

…actPullDiagnosticHandler.cs

CyrusNajmabadi · 2022-12-13T22:18:20Z

 internal abstract record AbstractDocumentDiagnosticSource<TDocument>(TDocument Document) : IDiagnosticSource
    where TDocument : TextDocument
 {
-    private static readonly ImmutableArray<string> s_todoCommentCustomTags = ImmutableArray.Create(PullDiagnosticConstants.TaskItemCustomTag);


these types got much simpler as task-list handling was extracted into a dedicated diagnostic source.

CyrusNajmabadi · 2022-12-13T22:18:50Z

+        var allSpanDiagnostics = await diagnosticAnalyzerService.GetDiagnosticsForSpanAsync(
+            Document, range: null, diagnosticKind: this.DiagnosticKind, cancellationToken: cancellationToken).ConfigureAwait(false);
        return allSpanDiagnostics;
    }


this part was extremely clean, and fits onto the current diagnostic service API without any issue.

CyrusNajmabadi · 2022-12-13T22:19:11Z

+
+namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics;
+
+internal sealed record class TaskListDiagnosticSource(Document Document) : AbstractDocumentDiagnosticSource<Document>(Document)


this is just extracting the logic for task list diagnostics into a dedicate source. it is otherwise unchanged.

…/roslyn into lspDiagCategories

CyrusNajmabadi · 2022-12-13T22:41:35Z

            // However we can include them as a part of workspace pull when FSA is on.
-            var documentDiagnostics = await diagnosticAnalyzerService.GetDiagnosticsForIdsAsync(Document.Project.Solution, Document.Project.Id, Document.Id, cancellationToken: cancellationToken).ConfigureAwait(false);
+            var documentDiagnostics = await diagnosticAnalyzerService.GetDiagnosticsForIdsAsync(
+                Document.Project.Solution, Document.Project.Id, Document.Id, cancellationToken: cancellationToken).ConfigureAwait(false);


this part was a little messy. specifically, GetDiagnosticsForIdsAsync doesn't support fine-grained diagnostic requests (@mavasani ). So for workspace-document-pull requests, we just wait on getting all the diagnostics. this means that there's no speedup on getting diagnostics for closed files. However, i think that's ok as you only get those for FSA, and FSA is notoriously slow anyways.

If we do want to support this, it would be possible. We'd just need GetDiagnosticsForIdsAsync to support passing in a diagnostickind. I initially attempted to do that, but i was uncertain about the precise semantics of how that would work in the impl.

If you are interested in adding support for that @mavasani i can update this side of things.

I think it is fine to wait for adding this support as it does not seem to be a mainline scenario.

CyrusNajmabadi · 2022-12-13T22:43:35Z

+        protected override async ValueTask<ImmutableArray<IDiagnosticSource>> GetOrderedDiagnosticSourcesAsync(
+            VSInternalWorkspaceDiagnosticsParams diagnosticsParams,
+            RequestContext context,
+            CancellationToken cancellationToken)


helper to walk projects in priority order. used by both teh normal diagnostic walking code, and the task-list code.

CyrusNajmabadi · 2022-12-13T22:44:09Z

+            // diagnostics will be handled by razor itself, which will operate by calling into Roslyn and asking for
+            // document-diagnostics instead.
+            if (context.ServerKind == WellKnownLspServerKinds.RazorLspServer)
+                return ImmutableArray<IDiagnosticSource>.Empty;


@dibarbet we both check this and below check if (document.IsRazorDocument()) to bail out. Do we need both? Or can we skip the second check if the server is the razor server?

I believe we need both - there are two cases

The razor C# server is being asked for workspace diagnostics (this check here). The razor C# server shouldn't give anything back

The normal C# server is being asked for workspace diagnostics, and it sees a Razor document. It should not return diagnostics for that document because we rely on razor to do a document pull for all razor documents.

CyrusNajmabadi · 2022-12-13T22:44:33Z

+        }
+
+        private static ImmutableArray<IDiagnosticSource> GetTaskListDiagnosticSources(
+            RequestContext context, IGlobalOptionService globalOptions)


dedicated helpers to get the sources for task-list vs diagnsotics.

dibarbet · 2022-12-13T22:36:19Z

            {
                serverCapabilities.SupportsDiagnosticRequests = true;
                serverCapabilities.MultipleContextSupportProvider = new VSInternalMultipleContextFeatures { SupportsMultipleContextsDiagnostics = true };
+                serverCapabilities.DiagnosticProvider ??= new();


could use initializer syntax here? This should always be null here I believe

dibarbet · 2022-12-13T22:40:57Z

+        protected static (DiagnosticKind diagnosticKind, bool taskList, bool isProject) GetDiagnosticKindInfo(VSInternalDiagnosticKind? queryingDiagnosticKind)
+        {
+            if (queryingDiagnosticKind?.Value == PullDiagnosticConstants.Project)
+                return (default, taskList: false, isProject: true);


maybe an explicit kind instead of default?

dibarbet · 2022-12-13T22:53:21Z

        /// </summary>
-        protected abstract ValueTask<ImmutableArray<IDiagnosticSource>> GetOrderedDiagnosticSourcesAsync(RequestContext context, CancellationToken cancellationToken);
+        protected abstract ValueTask<ImmutableArray<IDiagnosticSource>> GetOrderedDiagnosticSourcesAsync(
+            TDiagnosticsParams diagnosticsParams, RequestContext context, CancellationToken cancellationToken);


as discussed offline, we'll need a different cache per kind

yup. has been done.

dibarbet · 2022-12-13T23:01:14Z

+            // diagnostics will be handled by razor itself, which will operate by calling into Roslyn and asking for
+            // document-diagnostics instead.
+            if (context.ServerKind == WellKnownLspServerKinds.RazorLspServer)
+                return ImmutableArray<IDiagnosticSource>.Empty;


I believe we need both - there are two cases

The razor C# server is being asked for workspace diagnostics (this check here). The razor C# server shouldn't give anything back

The normal C# server is being asked for workspace diagnostics, and it sees a Razor document. It should not return diagnostics for that document because we rely on razor to do a document pull for all razor documents.

dibarbet · 2022-12-14T01:07:51Z

-            Assert.Equal("todo: goo", results.Single().Diagnostics.Single().Message);
+            Assert.Empty(results.Single().Diagnostics);
+            //Assert.Equal("TODO", results.Single().Diagnostics.Single().Code);
+            //Assert.Equal("todo: goo", results.Single().Diagnostics.Single().Message);


should be removed

CyrusNajmabadi added 16 commits December 13, 2022 13:18

In progress

a19957e

Dedicated source

2b328e4

In progrss

b1a50f2

In progrss

47d924f

Add diagnostic kind

5c6c194

Flow along

e476e5a

create right kind of source

c40c280

Workspace side

e94e6ca

Simplify

3fc2623

Simplify

4e99c72

Simplify

dd4ac8e

Pass flag down

415545c

Fixing

21c0c5a

project diagnostics

8037c33

Break out project sources

ae542a6

Simplify

b44631d

CyrusNajmabadi requested a review from dibarbet December 13, 2022 22:13

CyrusNajmabadi requested a review from a team as a code owner December 13, 2022 22:13

ghost added the Area-IDE label Dec 13, 2022

Docs

71d789c

CyrusNajmabadi commented Dec 13, 2022

View reviewed changes

Comment thread src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs

Update src/Features/LanguageServer/Protocol/Handler/Diagnostics/Abstr…

f185bde

…actPullDiagnosticHandler.cs

CyrusNajmabadi commented Dec 13, 2022

View reviewed changes

CyrusNajmabadi added 3 commits December 13, 2022 14:21

Revrt this

1bfec1a

Cleanup

441e998

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

97db26b

…/roslyn into lspDiagCategories

Simplify

5e8a362

CyrusNajmabadi commented Dec 13, 2022

View reviewed changes

CyrusNajmabadi added 3 commits December 13, 2022 14:42

add comment

65894a8

Simplify

5f32d69

Simplify

a48c857

CyrusNajmabadi commented Dec 13, 2022

View reviewed changes

Add helper

adb7895

dibarbet reviewed Dec 13, 2022

View reviewed changes

CyrusNajmabadi added 3 commits December 13, 2022 15:30

Update tests

094e6c7

Add tests

d2d248e

Distinct version caches

798e8f5

dibarbet reviewed Dec 14, 2022

View reviewed changes

dibarbet approved these changes Dec 14, 2022

View reviewed changes

CyrusNajmabadi and others added 4 commits December 13, 2022 18:16

Merge branch 'main-vs-deps' into lspDiagCategories

59bfbb1

Delete dead code

0200847

lint

e730f13

Remove unused usings

b169360

CyrusNajmabadi enabled auto-merge December 14, 2022 04:45

CyrusNajmabadi merged commit 38360d1 into dotnet:main-vs-deps Dec 14, 2022

ghost added this to the Next milestone Dec 14, 2022

allisonchou mentioned this pull request Dec 15, 2022

[Automated] PRs inserted in VS build main-33215.136 #66029

Closed

Cosifne modified the milestones: Next, 17.5 P3 Jan 4, 2023

CyrusNajmabadi deleted the lspDiagCategories branch February 2, 2023 18:12


		namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics;

		internal sealed record class TaskListDiagnosticSource(Document Document) : AbstractDocumentDiagnosticSource<Document>(Document)

Conversation

CyrusNajmabadi commented Dec 13, 2022

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Dec 13, 2022 • 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!

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyrusNajmabadi Dec 13, 2022 •

edited

Loading