Reenable LSP pull diagnostics #48328

CyrusNajmabadi · 2020-10-05T15:13:51Z

This reverts commit 02a9e20.

…spDiagnostics"" This reverts commit 02a9e20.

dibarbet · 2020-10-08T03:31:05Z

@CyrusNajmabadi when you have time, this will need latest master merged in to fix the failures (or let me know and I can update your branch)

edit - nvm it isn't merged in yet - spoke too soon!

CyrusNajmabadi · 2020-10-15T19:15:01Z

src/EditorFeatures/Core/Implementation/Diagnostics/AbstractDiagnosticsTaggerProvider.cs

@@ -151,7 +151,7 @@ private void ProduceTags(TaggerContext<TTag> context, DocumentSnapshotSpan spanT
            buffer?.Properties.TryGetProperty(PredefinedPreviewTaggerKeys.SuppressDiagnosticsSpansKey, out suppressedDiagnosticsSpans);

            var buckets = _diagnosticService.GetDiagnosticBuckets(
-                workspace, document.Project.Id, document.Id, context.CancellationToken);
+                workspace, document.Project.Id, document.Id, forPullDiagnostics: false, context.CancellationToken);


an intermediary concept this PR introduces is that the two major clients of the diagnostics subsystem now pass in what behavior they want when calling into the DiagnosticService singleton.

i.e. the normal push features call in saying forPullDiagnostics: false and the lsp system calls in saying forPullDiagnostics: true. Internally we see which mode we're actually in and we either pass along the diagnostics, or pass along empty. in essence only one side will work, and the other will see absolutely no diagnostics ever.

This is not pretty, but it was:

the cleanest way i could find to keep both systems around

required minimal and mechanical changes to both systems.

ensured that every entrypoint was guarded so diagnostics wouldn't accidentally sneak into the wrong client.

Once we are entirely on pull, all the push components can go away (as can tehse booleans).

src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs

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

CyrusNajmabadi · 2020-10-15T19:21:59Z

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

+            // If this is a pull client, but pull diagnostics is not on, then they get nothing.  Similarly, if this is a
+            // push client and pull diagnostics are on, they get nothing.
+            if (forPullDiagnostics != workspace.Options.GetOption(InternalDiagnosticsOptions.LspPullDiagnostics))
+                return ImmutableArray<DiagnosticData>.Empty;


this is one of 3 main entrypoints that now ensures the right data flows out depending on the caller's needs. If the caller doesn't match the current diagnostics-mode we're in (either 'push' or 'pull') they get nothing back. This allows DiagnosticService to still be the centralized authority that everyone uses.

CyrusNajmabadi · 2020-10-15T19:22:26Z

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

+            // If this is a pull client, but pull diagnostics is not on, then they get nothing.  Similarly, if this is a
+            // push client and pull diagnostics are on, they get nothing.
+            if (forPullDiagnostics != workspace.Options.GetOption(InternalDiagnosticsOptions.LspPullDiagnostics))
+                return ImmutableArray<DiagnosticBucket>.Empty;


second (of three) entrypoint that is updated.

CyrusNajmabadi · 2020-10-15T19:22:49Z

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

+            // If this is a pull client, but pull diagnostics is not on, then they get nothing.  Similarly, if this is a
+            // push client and pull diagnostics are on, they get nothing.
+            if (forPullDiagnostics != workspace.Options.GetOption(InternalDiagnosticsOptions.LspPullDiagnostics))
+                return ImmutableArray<DiagnosticData>.Empty;


third (of three) entrypoint that was updated.

CyrusNajmabadi · 2020-10-15T19:38:52Z

src/VisualStudio/Core/Def/Implementation/LanguageClient/AbstractLanguageServerClient.cs

@@ -20,7 +20,6 @@ namespace Microsoft.VisualStudio.LanguageServices.Implementation.LanguageService
    internal abstract class AbstractLanguageServerClient : ILanguageClient
    {
        private readonly string? _diagnosticsClientName;
-        private readonly IDiagnosticService _diagnosticService;


all the lsp push diagnostic code is removed with this PR.

The client is now sending a capability that tells us whether they support pull diagnostics (https://devdiv.visualstudio.com/DevDiv/_git/VSLanguageServerClient/pullrequest/279454?_a=files) so if that is false, we should keep this code running.

CyrusNajmabadi · 2020-10-15T19:39:10Z

src/VisualStudio/Core/Def/Implementation/LanguageClient/InProcLanguageServer.cs

-                }
-
-                return ImmutableArray<LSP.Diagnostic>.Empty;
-            }


yaay deleted code.

Super happy to delete this code - but this means that in order for razor to get diagnostics, they will have to set the feature flag to LSP diagnostics for regular c# files as well right? I'm OK with that but wanted to make sure that is the goal

CyrusNajmabadi · 2020-10-15T19:51:24Z

...ef/Implementation/TableDataSource/VisualStudioBaseDiagnosticListTable.LiveTableDataSource.cs

@@ -159,7 +159,10 @@ private object CreateAggregationKey(DiagnosticsUpdatedArgs data)

            private void PopulateInitialData(Workspace workspace, IDiagnosticService diagnosticService)
            {
-                foreach (var bucket in diagnosticService.GetDiagnosticBuckets(workspace, projectId: null, documentId: null, cancellationToken: CancellationToken.None))
+                var diagnostics = diagnosticService.GetDiagnosticBuckets(
+                    workspace, projectId: null, documentId: null, forPullDiagnostics: false, cancellationToken: CancellationToken.None);


All existing code calls forPullDiagnostics: false.

CyrusNajmabadi · 2020-10-15T19:52:29Z

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

+            var text = await document.GetTextAsync(cancellationToken).ConfigureAwait(false);
+            using var _ = ArrayBuilder<VSDiagnostic>.GetInstance(out var result);
+
+            var diagnostics = _diagnosticService.GetDiagnostics(document, includeSuppressedDiagnostics: false, forPullDiagnostics: true, cancellationToken);


this is new code. it calls forPullDiagnostics: true as it wants to only run if we're explicitly opted-into pull mode.

…ming them in LSP scenarios.

dibarbet

a few comments, but nothing blocking!

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

dibarbet · 2020-10-19T17:43:26Z

src/VisualStudio/Core/Def/Implementation/LanguageClient/PullDiagnosticsInProcLanguageClient.cs

+                SupportsDiagnosticRequests = true,
+            };
+    }
+}


Yep this looks good!
Do we want to conditionally set this capability based on the feature flag? That way we can avoid having the client even call into us if it's off. Might be too minor of an optimization though.

dibarbet · 2020-10-19T18:25:49Z

src/VisualStudio/Core/Def/Implementation/LanguageClient/InProcLanguageServer.cs

-                }
-
-                return ImmutableArray<LSP.Diagnostic>.Empty;
-            }


Super happy to delete this code - but this means that in order for razor to get diagnostics, they will have to set the feature flag to LSP diagnostics for regular c# files as well right? I'm OK with that but wanted to make sure that is the goal

Co-authored-by: David <dibarbet@gmail.com>

…oslyn into lspDiagnostics2

CyrusNajmabadi · 2020-10-19T19:34:41Z

Do we want to conditionally set this capability based on the feature flag?

Yes. That seems reasonable :)

CyrusNajmabadi · 2020-10-19T19:36:25Z

Super happy to delete this code - but this means that in order for razor to get diagnostics, they will have to set the feature flag to LSP diagnostics for regular c# files as well right? I'm OK with that but wanted to make sure that is the goal

Yeah, i'm ok with that :)

Revert "Revert "Merge pull request dotnet#47914 from CyrusNajmabadi/l…

1cadaee

…spDiagnostics"" This reverts commit 02a9e20.

CyrusNajmabadi requested a review from a team as a code owner October 5, 2020 15:13

allisonchou approved these changes Oct 5, 2020

View reviewed changes

davidwengier approved these changes Oct 6, 2020

View reviewed changes

dibarbet changed the base branch from master-vs-deps to master October 7, 2020 19:05

davidwengier mentioned this pull request Oct 7, 2020

[LSP] Enable push or pull diagnostics but not both #48414

Closed

jinujoseph added the Area-IDE label Oct 8, 2020

CyrusNajmabadi added 8 commits October 8, 2020 12:18

Merge remote-tracking branch 'upstream/master' into lspDiagnostics2

a7c6236

Merge remote-tracking branch 'upstream/master' into lspDiagnostics2

af83f65

Remove unnecessary fields/methods for lsp push diagnostics

55cb2e1

Remove unnecessary fields/methods for lsp push diagnostics

88bfa0d

Add experimental flag

b35167c

Remove

54502f5

Add option to switch between push/pull diagnostics.

0b0cbf0

Reanme

e4e5f89

CyrusNajmabadi changed the title ~~Revert "Revert "Merge pull request #47914 from CyrusNajmabadi/lspDiagnostics""~~ Reenable LSP pull diagnostics Oct 15, 2020

CyrusNajmabadi commented Oct 15, 2020

View reviewed changes

src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs Show resolved Hide resolved

add ocmments

650e11a

mavasani reviewed Oct 15, 2020

View reviewed changes

src/Features/Core/Portable/Diagnostics/IDiagnosticService.cs Show resolved Hide resolved

CyrusNajmabadi commented Oct 15, 2020

View reviewed changes

Add tests

b25608c

CyrusNajmabadi commented Oct 15, 2020

View reviewed changes

UI option

8ea42bd

CyrusNajmabadi added 16 commits October 18, 2020 16:33

Add comments

4737fcd

Revert

c1fdd37

Add an abstraction for either reporting an array or results, or strea…

fbe9c65

…ming them in LSP scenarios.

Make threadsafe

7cba654

Remove #nullable enable

67111a1

Docs

f422ad5

Merge branch 'bufferedProgress' into lspDiagnostics2

162fd21

Tweak the progress type to better support arrays and scalars.

455a733

Merge branch 'bufferedProgress' into lspDiagnostics2

c00d729

Merge branch 'extractMore' into lspDiagnostics2

b084ea7

Merge remote-tracking branch 'upstream/master' into lspDiagnostics2

21d4761

Remove

a0ed107

Update

0a78828

Simplify

9a82574

Fixup tests

573e342

Merge remote-tracking branch 'upstream/master' into lspDiagnostics2

2338c37

dibarbet approved these changes Oct 19, 2020

View reviewed changes

CyrusNajmabadi and others added 5 commits October 19, 2020 15:20

Update src/Features/Core/Portable/Diagnostics/DiagnosticsUpdatedArgs.cs

06b8ea0

Co-authored-by: David <dibarbet@gmail.com>

Fix test

07bbaae

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

130c913

…oslyn into lspDiagnostics2

Fix test

3c7fdb6

Fix test

c90b3ac

only enable diagnostics based on option

5e0ee01

CyrusNajmabadi merged commit 242160f into dotnet:master Oct 20, 2020

ghost added this to the Next milestone Oct 20, 2020

CyrusNajmabadi deleted the lspDiagnostics2 branch October 20, 2020 01:56

allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reenable LSP pull diagnostics #48328

Reenable LSP pull diagnostics #48328

CyrusNajmabadi commented Oct 5, 2020

dibarbet commented Oct 8, 2020 •

edited

Loading

CyrusNajmabadi Oct 15, 2020

CyrusNajmabadi Oct 15, 2020

CyrusNajmabadi Oct 15, 2020

CyrusNajmabadi Oct 15, 2020

CyrusNajmabadi Oct 15, 2020

davidwengier Oct 15, 2020

CyrusNajmabadi Oct 15, 2020

dibarbet Oct 19, 2020

CyrusNajmabadi Oct 15, 2020

CyrusNajmabadi Oct 15, 2020

dibarbet left a comment

dibarbet Oct 19, 2020

dibarbet Oct 19, 2020

CyrusNajmabadi commented Oct 19, 2020

CyrusNajmabadi commented Oct 19, 2020

Reenable LSP pull diagnostics #48328

Reenable LSP pull diagnostics #48328

Conversation

CyrusNajmabadi commented Oct 5, 2020

dibarbet commented Oct 8, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibarbet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Oct 19, 2020

CyrusNajmabadi commented Oct 19, 2020

dibarbet commented Oct 8, 2020 •

edited

Loading