Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LSP] Send semantic tokens refresh notifications when compilations are available #60991

Closed
wants to merge 1 commit into from
Closed

[LSP] Send semantic tokens refresh notifications when compilations are available #60991

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRefreshListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@

using System;
using System.Collections.Immutable;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Shared.TestHooks;
using Microsoft.VisualStudio.LanguageServer.Protocol;
using Microsoft.VisualStudio.Threading;
using Newtonsoft.Json.Linq;
using Roslyn.Utilities;
using StreamJsonRpc;
Expand Down Expand Up @@ -45,9 +47,27 @@ public SemanticTokensRefreshListener(

private void OnLspWorkspaceChanged(object? sender, WorkspaceChangeEventArgs e) => _workQueue.AddWork();

// TO-DO: Replace hardcoded string with const once LSP side is merged.
public ValueTask SendSemanticTokensNotificationAsync(CancellationToken cancellationToken)
=> _languageServerNotificationManager.SendNotificationAsync(Methods.WorkspaceSemanticTokensRefreshName, cancellationToken);
public async ValueTask SendSemanticTokensNotificationAsync(CancellationToken cancellationToken)
{
// Currently, we send a refresh notification to the client whenever there are any workspace changes.
// However, we also need to send notifications whenever the compilations finish processing since
// compilations impact semantic tokens. We put this logic here rather than in the semantic tokens
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder - can this still go into the range handler itself? Basically when we get a semantic tokens range request we trigger the compilation request (if not available) which triggers an event of some kind that feeds here?

That way we get de-bouncing as well on our side.

// range handler so that the client still gets initial colorization on file open.
var trackedDocuments = _lspWorkspaceManager.GetTrackedLspText();
var projects = trackedDocuments.Select(
d => _lspWorkspaceManager.GetLspDocument(new TextDocumentIdentifier { Uri = d.Key })?.Project).Distinct();

foreach (var project in projects)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially problematic as we'll be trying to get a lot more compilations because we're now calculating compilations for all changes to all the open projects instead of just the snapshot of the project that is being requested by the semantic tokens range handling.

I think we definitely only want to trigger the compilation for the project requested by the range handler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we definitely only want to trigger the compilation for the project requested by the range handler.

This is my thinking too, but I'd like to hear the thought process to build all projects in case I'm missing something.

{
if (project is null)
continue;

cancellationToken.ThrowIfCancellationRequested();
await project.GetCompilationAsync(cancellationToken).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely needs to check capabilities - we don't want to do this if the semantic tokens capabilities are not on

await _languageServerNotificationManager.SendNotificationAsync(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Razor debounces refresh notifications so sending more notifications shouldn't be an issue. @gundermanc just to confirm, does the LSP client do the same?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gundermanc just to confirm, does the LSP client do the same?

I don't think we do that currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would debouncing on the client be an option for M2? Maybe batching all the requests within a 1-2 second window?

Copy link
Member

@dibarbet dibarbet Apr 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should de-bounce on our side regardless of what the client does, either with feeding this as an event from range (as my other comments) or via queuing this compilation available event into the work queue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would debouncing on the client be an option for M2? Maybe batching all the requests within a 1-2 second window?

I would vote we not take that to the Client for M2 to avoid churn. It's an optimization rather than a "bug fix" (unless I'm missing something) and I would project that while the impact would be measurable it probably wouldn't cause high-level Watsons or anything (famous last words).

Methods.WorkspaceSemanticTokensRefreshName, cancellationToken).ConfigureAwait(false);
}
}

public void Dispose()
{
Expand Down