[LSP] Implement support for `workspace/semanticTokens/refresh` notifications by allisonchou · Pull Request #60824 · dotnet/roslyn

allisonchou · 2022-04-18T22:40:57Z

This needs to wait for the LSP side PR to be merged + inserted first.
Implements workspace/semanticTokens/refresh support, which should improve perf by making polling obsolete.
There is an existing bug in the LSP client with middle layer interceptors that makes testing with Razor not possible. Will wait on merging this PR until that bug is fixed + I've manually tested the scenarios.
There is also a Razor side PR since they need to intercept Roslyn's messages: Intercept C# semantic tokens refresh messages razor#6298

allisonchou · 2022-04-18T22:41:31Z

src/EditorFeatures/Core/LanguageServer/AbstractInProcLanguageClient.cs

+        /// <summary>
+        /// Gets a value indicating whether a notification bubble show be shown when the language server fails to initialize.
+        /// </summary>
+        public abstract bool ShowNotificationOnInitializeFailed { get; }


Just moved this property up to fit in with the rest of the properties

allisonchou · 2022-04-18T22:46:34Z

src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.cs

-            // differentiate between the different C# LSP servers that have the same client name.
-            // We also don't use the language client's name property as it is a localized user facing string
-            // which is difficult to write telemetry queries for.
-            _requestTelemetryLogger = new RequestTelemetryLogger(_serverKind.ToTelemetryString());


Moved these two values up to LanguageServerTarget since they're needed by the semantic tokens refresh listener

allisonchou · 2022-04-18T22:47:37Z

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRefreshListener.cs

+            _workQueue = new AsyncBatchingWorkQueue(
+                delay: TimeSpan.FromMilliseconds(2000),
+                processBatchAsync: SendSemanticTokensRefreshNotificationAsync,
+                asyncListener: listener,


I wasn't sure what the correct value of asyncListener should be here - currently it's FeatureAttribute.LanguageServer, but not sure if we want that or FeatureAttribute.Workspace instead

allisonchou · 2022-04-18T22:48:36Z

src/Features/LanguageServer/Protocol/LanguageServerTarget.cs

                Contract.ThrowIfTrue(_clientCapabilities != null, $"{nameof(InitializeAsync)} called multiple times");
                _clientCapabilities = initializeParams.Capabilities;
+
+                // TO-DO: Add client capability check below once LSP side is merged


Before merging this PR, will add a client capability check for refresh once LSP side is merged - https://devdiv.visualstudio.com/DevDiv/_git/VSLanguageServerClient/pullrequest/388715

allisonchou · 2022-04-18T22:49:48Z

src/Tools/ExternalAccess/Razor/Microsoft.CodeAnalysis.ExternalAccess.Razor.csproj

  </ItemGroup>

+  <ItemGroup>
+    <!-- No warn on NU1701 until the LSP client targets netstandard2.0 https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1369985/ -->


This was adapted from EditorFeatures.csproj in order to include the VS LSP client package reference, which is needed for access to the ILanguageClientMiddleLayer type.

@dibarbet it looks like the underlying issue here was marked as "completed" but I'm still seeing the same errors mentioned in the internal issue. Would you have any context here?

@allisonchou depending on when the bug was completed, we may need to depend on an updated version of the VS language client
https://github.com/dotnet/roslyn/blob/main/eng/Versions.props#L155

It looks like the bug was completed in mid-January, however I attempted updating to a client version from February and the problem persists. I'll comment on the original bug to ask

src/Tools/ExternalAccess/Razor/RazorLanguageServerFactoryWrapper.cs

dibarbet · 2022-04-18T23:05:28Z

src/EditorFeatures/Core/LanguageServer/AbstractInProcLanguageClient.cs

+        /// <remarks>
+        /// Currently utilized by Razor to intercept Roslyn's workspace/semanticTokens/refresh requests.
+        /// </remarks>
+        public object? MiddleLayer => _middleLayer;


Interesting - why do we need to support a middle layer here? Shouldn't that be possible entirely on the client side?

We can specify the middle layer as part of the ILanguageClientCustomMessage2 interface, which the client then uses to send messages through the interceptors. I'm thinking it's meant to be specified on the server side so we can specify the middle layer we want, otherwise the client could send requests to all the potential middle layers.

Interesting. It seems like it could be done via content type or lsp server name. But I dont generally have a problem with this, was just wondering

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRefreshListener.cs

src/Features/LanguageServer/Protocol/LanguageServerTarget.cs

src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

dibarbet · 2022-04-18T23:19:57Z

src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

        }

+        // Send a workspace changed notification to anyone subscribed. For example, this is important for semantic tokens refresh.
+        LspWorkspaceChanged?.Invoke(sender, e);


We may want to document that the event handler implementation for this should not do basically any synchronous work, as that will block here.

Right now the only implementation basically does nothing (good), but I'm wondering if there is a way to ensure this cannot block even if the implementation of the event does something bad. Async callback function is an obvious option, but doesn't allow for easy registration of listeners (@CyrusNajmabadi for ideas)

I added a huge bolded XML warning above the event handler. Would be curious if Cyrus has any ideas on how to improve this too though

src/Tools/ExternalAccess/Razor/RazorCSharpInterceptionMiddleLayer.cs

src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRefreshListener.cs

dibarbet · 2022-04-19T19:16:28Z

src/Features/LanguageServer/Protocol/IRefreshListener.cs

+
+namespace Microsoft.CodeAnalysis.LanguageServer
+{
+    internal interface IRefreshListener : IDisposable


I think this was probably my mistake for not being clear - what I meant was a wrapper interface around the jsonrpc object that has a SendNotification method that takes in the params and method name.

Then you import that wrapper interface into the semantic tokens notification thing

Done! Hopefully the abstraction makes things easier in the future. I wasn't sure about naming, if you have any suggestions feel free to lmk

src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRefreshListener.cs

allisonchou added 5 commits April 12, 2022 14:29

Initial changes

5a7d45d

Initial changes

9cbd6c3

Add message interceptor

b8acc42

Simplify middle layer logic

940c7a2

Cleanup

42616d8

allisonchou added the LSP issues related to the roslyn language server protocol implementation label Apr 18, 2022

allisonchou requested review from a team as code owners April 18, 2022 22:40

ghost added the Area-IDE label Apr 18, 2022

allisonchou added 2 commits April 18, 2022 15:43

Update comment

c32ac9f

remove unnecessary null check

9ebdc97

allisonchou commented Apr 18, 2022

View reviewed changes

allisonchou assigned davidwengier, NTaylorMullen, dibarbet and ryanbrandenburg Apr 18, 2022

dibarbet reviewed Apr 18, 2022

View reviewed changes

NTaylorMullen approved these changes Apr 18, 2022

View reviewed changes

allisonchou added 2 commits April 19, 2022 10:56

Partial feedback

79c7a7a

Further PR feedback

0d703a9

dibarbet reviewed Apr 19, 2022

View reviewed changes

allisonchou mentioned this pull request Apr 19, 2022

Intercept C# semantic tokens refresh messages dotnet/razor#6298

Merged

partial review feedback

5c46d9a

CyrusNajmabadi reviewed Apr 19, 2022

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRefreshListener.cs Outdated Show resolved Hide resolved

allisonchou added 2 commits April 19, 2022 17:16

Abstract away jsonrpc

7effd84

misplaced usings

b8cd8bf

dibarbet approved these changes Apr 20, 2022

View reviewed changes

allisonchou added 3 commits April 20, 2022 16:07

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

196d240

Temporarily make import optional

e75e179

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

a8bac57

Bump protocol version

70160d9

allisonchou merged commit 084365c into dotnet:main Apr 22, 2022

ghost added this to the Next milestone Apr 22, 2022

allisonchou deleted the SemanticTokensRefresh branch April 22, 2022 19:21

dibarbet modified the milestones: Next, 17.3.P1 Apr 25, 2022

allisonchou mentioned this pull request May 2, 2022

[LSP] Implement workspace/semanticTokens/refresh #60441

Closed

Conversation

allisonchou commented Apr 18, 2022 • 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dibarbet Apr 19, 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!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

allisonchou commented Apr 18, 2022 •

edited

Loading

dibarbet Apr 19, 2022 •

edited

Loading