Skip to content

Support textDocument/semanticTokens/full#77665

Merged
dibarbet merged 1 commit intodotnet:mainfrom
PaddiM8:semantic-tokens-full
Mar 19, 2025
Merged

Support textDocument/semanticTokens/full#77665
dibarbet merged 1 commit intodotnet:mainfrom
PaddiM8:semantic-tokens-full

Conversation

@PaddiM8
Copy link
Contributor

@PaddiM8 PaddiM8 commented Mar 18, 2025

Solves #70536 (apart from delta). Some editors, such as neovim, don't support textDocument/semanticTokens/range, which means that you currently don't get semantic highlighting for .NET in these editors. Getting semantic tokens for the entire document is of course slower, but the editor can decide what to do anyway.

Tested and working with neovim.

@PaddiM8 PaddiM8 requested a review from a team as a code owner March 18, 2025 22:37
@ghost ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 18, 2025
@dotnet-policy-service dotnet-policy-service bot added Community The pull request was submitted by a contributor who is not a Microsoft employee. VSCode labels Mar 18, 2025
@PaddiM8
Copy link
Contributor Author

PaddiM8 commented Mar 18, 2025

@dotnet-policy-service agree

JoeRobich
JoeRobich approved these changes Mar 18, 2025
View reviewed changes
Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

Thanks @PaddiM8!

CyrusNajmabadi
CyrusNajmabadi reviewed Mar 18, 2025
View reviewed changes
src/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensHelpers.cs
if (ranges.Length == 0)
{
return [];
}
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Mar 18, 2025

Choose a reason for hiding this comment

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

i actually like this. i would just make ranges nullable and say that if it is null that you get the entire doc. but if it is empty you get nothing back.

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Mar 19, 2025

Choose a reason for hiding this comment

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

note: this was not a joke :)

dibarbet
dibarbet requested changes Mar 18, 2025
View reviewed changes
Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

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

Not 100% opposed to allowing others to opt in to full semantic tokens, but we need to make sure this does not accidentally opt vscode into using full.

With Roslyn's semantic model, providing full document semantic tokens is incredibly expensive (but small ranges are very fast). We previously used full and had to disable it due to perf issues, especially on large documents.

dibarbet
dibarbet reviewed Mar 18, 2025
View reviewed changes
src/LanguageServer/Protocol/DefaultCapabilitiesProvider.cs Outdated
capabilities.SemanticTokensOptions = new SemanticTokensOptions
{
Full = false,
Full = true,
Copy link
Member

@dibarbet dibarbet Mar 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We need some kind of mechanism to allow different clients to either enable this or not. We do not want to enable this in VSCode (which is what will happen with this PR IIRC).

Potentially setting this capability dynamically in response to an option change could work. Not 100% sure yet.

Copy link
Contributor Author

@PaddiM8 PaddiM8 Mar 18, 2025

Choose a reason for hiding this comment

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

Hm right, that's true. Could we perhaps check if the client capabilities include semantic token ranges, and if not, enable this? Since we get those straight away

Copy link
Member

@dibarbet dibarbet Mar 19, 2025

Choose a reason for hiding this comment

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

That definitely sounds like a reasonable approach! We would always only use range, unless the client does not support range. Works for me

@PaddiM8 PaddiM8 force-pushed the semantic-tokens-full branch from d560425 to dd7c9ce Compare March 19, 2025 18:47
@PaddiM8 PaddiM8 force-pushed the semantic-tokens-full branch from dd7c9ce to b07f304 Compare March 19, 2025 18:56
@PaddiM8 PaddiM8 requested a review from dibarbet March 19, 2025 18:56
dibarbet
dibarbet approved these changes Mar 19, 2025
View reviewed changes
Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

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

lgtm - thanks!

CyrusNajmabadi
CyrusNajmabadi reviewed Mar 19, 2025
View reviewed changes
src/LanguageServer/Protocol/DefaultCapabilitiesProvider.cs
// potentially run a diff between the old and new tokens. Therefore, we only enable full handling if
// the client does not support ranges.
var rangeCapabilities = clientCapabilities.TextDocument?.SemanticTokens?.Requests?.Range;
var supportsSemanticTokensRange = rangeCapabilities?.Value is not (false or null);
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Mar 19, 2025

Choose a reason for hiding this comment

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

is this just asking is true?

Copy link
Contributor Author

@PaddiM8 PaddiM8 Mar 19, 2025

Choose a reason for hiding this comment

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

Range here is a sum type that can be either bool or object? so if you just did is true it would return false if there is an object instead, in which case the client would actually have the range capability, afaik

CyrusNajmabadi
CyrusNajmabadi reviewed Mar 19, 2025
View reviewed changes
src/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensFullHandler.cs
CancellationToken cancellationToken)
{
Contract.ThrowIfNull(request.TextDocument);
// Passing an empty array of ranges will cause the helper to return tokens for the entire document.
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Mar 19, 2025

Choose a reason for hiding this comment

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

i would still like us to use 'null' for this, and having empty return empty.

@CyrusNajmabadi
Copy link
Contributor

CyrusNajmabadi commented Mar 19, 2025

Note: i won't block on my feedback. But i may just make the change if this merges in as is :)

@dibarbet dibarbet merged commit 259b4d0 into dotnet:main Mar 19, 2025
25 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Mar 19, 2025
@CyrusNajmabadi
Copy link
Contributor

CyrusNajmabadi commented Mar 19, 2025

Thanks @PaddiM8 !

This was referenced Mar 21, 2025
Closed
Closed
Closed
@molostovvs molostovvs mentioned this pull request Apr 1, 2025
Closed
@jjonescz jjonescz modified the milestones: Next, 17.14 P3 Apr 1, 2025
@dibarbet dibarbet mentioned this pull request May 19, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoeRobich JoeRobich JoeRobich approved these changes

@dibarbet dibarbet dibarbet approved these changes

+1 more reviewer

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead VSCode

Projects

None yet

Milestone

17.14 P3

Development

Successfully merging this pull request may close these issues.

5 participants

@PaddiM8 @CyrusNajmabadi @JoeRobich @dibarbet @jjonescz