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.

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

Expose semantic tokens functionality to Razor EA #72495

Merged
merged 2 commits into from
Mar 14, 2024

Conversation

davidwengier
Copy link
Contributor

As part of dotnet/razor#9519 we need to be able to call into the guts of the Roslyn LSP handlers via C#, so this PR exposes semantic tokens as the first cab off the rank. In the meeting about this we said we were okay with using LSP types in these methods, but for semantic tokens specifically it seemed easy enough to avoid that, as the only relevant type is Range and it already has a good analog in LinePositionSpan.

There is more work to come with this I'm sure, but this will unblock things in Razor a little.

@davidwengier davidwengier requested a review from a team March 12, 2024 20:35
@davidwengier
Copy link
Contributor Author

ping @CyrusNajmabadi @dibarbet @dotnet/roslyn-ide, reviews would be much appreciated so this can be consumed from Razor.

new LSP.Range { Start = new Position(12, 0), End = new Position(13, 0) },
new LSP.Range { Start = new Position(29, 0), End = new Position(30, 0) }
};
ImmutableArray<LinePositionSpan> spans = [
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, feels like these tests should be calling the actual range handler..., but thats not a new problem in this PR

// off a request to ensure that the OOP side gets a fully up to compilation for this project. Once it does
// we can optionally choose to notify our caller to do a refresh if we computed a compilation for a new
// solution snapshot.
// TODO: await semanticTokensRefreshQueue.TryEnqueueRefreshComputationAsync(project, 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.

why was this commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is on the long list of follow-ups.

This method is called in the OOP process, so the queue has no access to send a refresh notification back to the client from here. We'll have to expose the queue functionality to the actual endpoint, and handle it there. Razor also has code to intercept refreshes, and map them from the generated C# back to the Razor file, and we need to work out a way to do that (if its still relevant? maybe not for semantic tokens)

@davidwengier davidwengier merged commit 711e122 into dotnet:main Mar 14, 2024
27 checks passed
@davidwengier davidwengier deleted the RazorSemanticTokens branch March 14, 2024 20:21
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Mar 14, 2024
@davidwengier
Copy link
Contributor Author

Thanks David! Merging so I can get an updated package flowing. Happy to follow up if anyone has any further comments (that hopefully aren't binary breaking 😛)

davidwengier added a commit to dotnet/razor that referenced this pull request Mar 20, 2024
Part of #9519

This PR feels like the last month of my life, with most of that time
simply fighting ServiceHub.

This adds a new endpoint for the cohosting server to support semantic
tokens. It's the first endpoint in the server. (Almost) the only thing
it does is immediately call our to the OOP process to do the actual
work. Logic is mainly provided by the same service that the existing
semantic tokens endpoint uses, but for a small piece of functionality
that is plugged in, via MEF, to call Roslyn via a new C# API
(dotnet/roslyn#72495) instead of via LSP.
Currently the syntax tree is computed on the fly in OOP, and the
document Roslyn is asked about is the one that the LSP server creates
via IDynamicFile. In future this will change to get the real source
generated document, and data from the source generator for the tree etc.

Reviewing commit-at-a-time is probably best. There are still a bunch of
TODOs, some hard-coded bits, and some hacky bits, but I wanted to ensure
this was actually reviewable and not too big.

Worth nothing this won't build until the Roslyn PR mentioned above is
merged, and a new package is available to update to.
@RikkiGibson RikkiGibson modified the milestones: Next, 17.10 P3 Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants