Create a Razor language server cohosted in Roslyn #70819

davidwengier · 2023-11-15T07:09:54Z

This creates a basic LSP client/server for the "Razor" content type, inheriting from the existing Roslyn language server. Tried to keep the code as minimal as possible, as all of the endpoints will be in the Razor repo, but we need document sync in this repo, in order to access internals on RequestContext, and project context in this repo to match the data the RequestContext expects to receive. Some minor complexity in order to avoid exposing VS Protocol types in the API, to avoid dependency annoyances.

This also includes a very small portion of #70170, which is support for LSP requests against TextDocuments. Thank you @mgoertz-msft!

I've tested locally with local build Roslyn and Razor, but not used it in anger to try to implement a "real" endpoint in the Razor repo yet. Wanted to put this up early-ish in case there is a lot of feedback.

…ndles requests

davidwengier · 2023-11-15T07:12:20Z

...Features/LanguageServer/Protocol/Handler/ProjectContext/GetTextDocumentWithContextHandler.cs

@@ -35,19 +35,20 @@ public GetTextDocumentWithContextHandler()
            Contract.ThrowIfNull(context.Workspace);
            Contract.ThrowIfNull(context.Solution);

-            // We specifically don't use context.Document here because we want multiple
-            var documents = context.Solution.GetDocuments(request.TextDocument.Uri);


Changing this to call GetTextDocuments seemed overly wasteful given only project info is needed, so I changed this handler a bit. Mainly wanted to avoid calling GetDocument and GetAdditionalDocument on every Id (and this endpoint is called a fair bit)

davidwengier · 2023-11-15T07:15:19Z

src/Tools/ExternalAccess/Razor/Cohost/RazorCohostLanguageClient.cs

+    ExportProvider exportProvider,
+    [Import(AllowDefault = true)] IRazorCohostCapabilitiesProvider? razorCapabilitiesProvider = null,
+    [Import(AllowDefault = true)] IRazorCustomMessageTarget? razorCustomMessageTarget = null,
+    [Import(AllowDefault = true)] IRazorCohostLanguageClientActivationService? razorCohostLanguageClientActivationService = null)


Made all of these nullable to avoid dual insertions etc. and so nothing activates if the Razor bits aren't installed. Though hopefully this never activates in that case, due to the content type :)

davidwengier · 2023-11-15T07:16:08Z

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

@@ -31,7 +31,6 @@
    <InternalsVisibleTo Include="Microsoft.VisualStudio.LanguageServices.Razor" Key="$(RazorKey)" />
    <InternalsVisibleTo Include="Microsoft.VisualStudio.LanguageServerClient.Razor" Key="$(RazorKey)" />
    <InternalsVisibleTo Include="Microsoft.VisualStudio.LanguageServerClient.Razor.Test" Key="$(RazorKey)" />
-    <InternalsVisibleTo Include="Microsoft.VisualStudio.Mac.LanguageServices.Razor" Key="$(RazorKey)" />


Snuck this change in here, and the above repo uri change :)

davidwengier · 2023-11-15T07:20:09Z

src/Tools/ExternalAccess/Razor/Cohost/AbstractRazorRequestHandler.cs

+
+namespace Microsoft.CodeAnalysis.ExternalAccess.Razor.Cohost;
+
+internal abstract class AbstractRazorCohostRequestHandler<TRequestType, TResponseType> : ILspServiceRequestHandler<TRequestType, TResponseType>


Endpoints on the Razor side will inherit from one of these two abstract types.

davidwengier · 2023-11-15T07:21:51Z

src/Tools/ExternalAccess/RazorTest/Cohost/RazorCohostTests.cs

+
+        protected override RazorTextDocumentIdentifier? GetRazorTextDocumentIdentifier(TextDocumentPositionParams request)
+        {
+            return new RazorTextDocumentIdentifier(request.TextDocument.Uri, (request.TextDocument as VSTextDocumentIdentifier)?.ProjectContext?.Id);


All of the endpoints in the Razor repo will have to do this, in particular the second parameter, in order to ensure project context flows correctly. Not too bad, and I couldn't see a way around it. Will create a nice extension method on the Razor side for it anyway, so it would be return request.TextDocument.ToRazorTextDocumentIdentifier()

dibarbet

Generally fine with this

dibarbet · 2023-11-15T18:47:44Z

src/Features/LanguageServer/Protocol/WellKnownLspServerKinds.cs

+    /// <summary>
+    /// Razor LSP server for Razor document requests (.razor and .cshtml files)
+    /// </summary>
+    RazorCohostServer,


So trying to understand the steps here

Create this server to handle Razor files (instead of the current Razor server). Migrate Razor support to here.

(maybe) Once the rest of Marco's work goes in, we can unify this Razor server with the RazorCSharp server, and have a single server that handles both

Once dynamic registration goes in we merge this with the normal C# server

Does that sound right? Potentially we can also skip 2) entirely if requests about C# are converted to non-LSP entirely before 3?

Item 2 hadn't really occurred to me, but I guess we could do that. Without dynamic registration though, we probably can't do that behind a feature flag though, as it would just be controlled by the content type attribute on the server. Interesting idea though.

The rest is correct though. Essentially this lets us work on migration, find bugs, maybe get some benefit, but without being blocked on dynamic registration.

src/Tools/ExternalAccess/Razor/Cohost/RazorCohostLanguageClient.cs

dibarbet · 2023-11-15T20:00:48Z

src/Tools/ExternalAccess/Razor/Cohost/RazorLspServiceProvider.cs

+[Export(typeof(RazorLspServiceProvider)), Shared]
+[method: ImportingConstructor]
+[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
+internal sealed class RazorLspServiceProvider(


All of these should go away once we can merge this into the C# server (with dynamic registration) right?

Yep! Thats why I tried to make them do as little as possible

davidwengier · 2023-11-19T20:59:50Z

@CyrusNajmabadi I heard a rumour you wanted to review this too?

I imagine there will be follow ups regardless, as I start implementing the Razor side

davidwengier · 2023-11-24T00:50:01Z

src/Tools/ExternalAccess/Razor/Cohost/RazorCohostLanguageClient.cs

+            return base.ActivateAsync(cancellationToken);
+        }
+
+        return Task.FromResult<Connection?>(null);


Sadly, this doesn't work. The platform helpfully checks this for null and just immediately throws an exception 🤦‍♂️

https://dev.azure.com/devdiv/DevDiv/_git/VSLanguageServerClient?path=/src/product/RemoteLanguage/Impl/Infrastructure/MefClientProvider/RemoteLanguageClientInstance.cs&version=GBdevelop&line=528&lineEnd=529&lineStartColumn=1&lineEndColumn=1&lineStyle=plain&_a=contents

So options are ClientName, or just return no capabilities. Thoughts @dibarbet ?

I'm guessing ClientName, since activating a server to nothing isn't great, but there is a slight downside with that, that we'd be unable to do progressive moves to this server, it would be all or nothing. I don't know how important that will be though.

So I'm not super opposed to having a server with no capabilities. We'll just have to make sure that the server isn't trying to do anything (e.g. sending refresh notifications).

As you mention it would allow you to migrate features one at a time which I think is fairly valuable.

…llow bar

...es/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/AbstractLanguageServer.cs

davidwengier · 2023-12-05T05:59:54Z

I've addressed all of the feedback, and am using this fine from the Razor repo. Just to confirm @dibarbet is this good to merge? Or do you want to hold off for a specific version etc.?

…m the Razor repo

davidwengier · 2023-12-11T01:09:30Z

Made a few more updates to this for issues found when implementing the first endpoint from Razor (in dotnet/razor#9674). Would be good if you can take a look and re-approve @dibarbet, then I'll merge so I can get some packages going.

...es/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/AbstractLanguageServer.cs

src/Features/LanguageServer/Protocol/Extensions/Extensions.cs

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

src/Features/LanguageServer/Protocol/WellKnownLspServerKinds.cs

dibarbet · 2023-12-11T20:56:24Z

src/Tools/ExternalAccess/Razor/Cohost/RazorCohostDidChangeEndpoint.cs

+        context.UpdateTrackedDocument(request.TextDocument.Uri, text);
+
+        // Razor can't handle this request because they don't have access to the RequestContext, but they might want to do something with it
+        if (didChangeHandler is not null)


Wondering here if we want a different mechanism to expose changes to Razor, if Razor needs to do special handling on didChange. For example similar to the IOnInitialized services that get called by the base InitializedHandler.

Especially when we unify with the Roslyn server- I'm not sure we want to allow separate didChange handlers per language.

Does the work Razor needs to do here require the queue to be blocked while its happening?

Same for open/close

All good thoughts, and I see this as definitely a point-in-time thing.

When we have dynamic registration, yes I would expect the "normal" text document sync handlers work for all languages and we'd remove these.

When Razor can pull on the source generator for our .g.cs files then we won't need to be notified about changes or opens.

For now though, we need something here, as the Razor side that responds to this is effectively unchanged from our existing code, we kick off a compile of the razor file as it changes.

Actually, even when we can use the source generator and even use the Roslyn API rather than LSP to ask questions about it etc, we'd still need some way to get notified of open and change of Razor files, and run code while the queue is blocked, in order to make sure the html virtual buffers are up to date. Maybe some way to hook into the LspWorkspaceManager? Haven't really thought about the specifics of that far ahead yet.

Hmm. Probably depends on what needs to happen. I wouldn't be opposed to something like IOnInitialized where we expose a hook inside the handler. I think putting it there makes it more clear that its a blocking hook (than putting in LspWorkspaceManager0

I'm missing something. Isn't that exactly what this is currently doing?

What I meant was putting code inside the main DidChangeHandler that allows for arbitrary additional callers, instead of duplicating teh handler for Razor and having a special Razor hook.
e.g. https://github.com/dotnet/roslyn/blob/main/src/Features/LanguageServer/Protocol/Handler/ServerLifetime/InitializedHandler.cs#L27

We'd have a generic hook that razor could implement. However I think you need to export a separate Razor didChange handler already - so I'm fine if we do that later when we merge the servers.

Oh yep, can definitely follow up with that once we merge. The existing handlers won't activate in this server though, due to the export attribute being used.

davidwengier · 2023-12-12T22:25:38Z

@dibarbet any reason not to merge this?

First part of #9519 Razor side of dotnet/roslyn#70819 This PR consumes the new Cohost server from Roslyn, and when enabled, moves DocumentColor requests from being handled by our existing Razor Language Server, to instead be handled by the new server. There is currently no sharing of project system data or anything like that yet. A few TODOs for follow ups, ~can't be merged, and indeed won't build, until a Roslyn package is available etc. but~ it's reviewable while we keep working, and more important its shippable in its "off" state. ~Ignore the last commit, obviously won't be merged :)~

davidwengier added 6 commits November 15, 2023 18:02

Create a Razor language server cohosted in Roslyn

821420b

Allow handlers to operate on TextDocuments

079c504

Add test to validate that the server has all required services and ha…

223d01b

…ndles requests

More realistic test

342d5de

Add endpoints that need to access Roslyn internals

f1ed068

Allow project context handler to work with any document type

3c3d285

davidwengier requested a review from a team as a code owner November 15, 2023 07:09

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 15, 2023

davidwengier requested review from dibarbet and a team November 15, 2023 07:10

davidwengier commented Nov 15, 2023

View reviewed changes

dibarbet approved these changes Nov 15, 2023

View reviewed changes

davidwengier commented Nov 24, 2023

View reviewed changes

Switch to ClientName to prevent activation, and avoid an immediate ye…

38bffad

…llow bar

build-analysis bot mentioned this pull request Nov 24, 2023

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

ryzngard approved these changes Nov 28, 2023

View reviewed changes

...es/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/AbstractLanguageServer.cs Outdated Show resolved Hide resolved

davidwengier added 2 commits December 4, 2023 16:39

Revert use of Client Name, switch to just reporting no capabilities

1db5ad9

PR feedback

464e873

davidwengier added 4 commits December 6, 2023 16:47

Add IRazorLspService in case any lsp services need to be exported fro…

b93538b

…m the Razor repo

Add version number to didChange extension point

af6b3ab

Add a way for Razor to make calls back to the client

06f72e9

Allow getting services from the RazorRequestContext

bc1768f

runfoapp bot mentioned this pull request Dec 7, 2023

FixAllOccurrencesIgnoresGeneratedCode is flaky #56625

Closed

This was referenced Dec 7, 2023

newintegrationtests.csharp.csharpaddmissingreference.verifyavailablecodeactions (vs2022) #64807

Closed

roslyn.visualstudio.newintegrationtests.csharp.csharpargumentprovider.fullcycle is flaky #66597

Closed

davidwengier added 4 commits December 10, 2023 15:06

Put types in their own files

8c3c4a9

Add more data to extension points, and allow async

0cc3ac8

Add empty base class for service exports, just in case

ad0302f

Make sure we do no work if the Razor cohost server isn't active

789c5cc

davidwengier mentioned this pull request Dec 11, 2023

Initial cohost server implementation dotnet/razor#9674

Merged

dibarbet reviewed Dec 11, 2023

View reviewed changes

davidwengier added 5 commits December 12, 2023 15:12

PR feedback

da42b86

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

c537048

Update after merge

28b837f

Fix capabilities so didOpen doesn't get sent at all

97689b5

Add exception message

64fa385

dibarbet approved these changes Dec 12, 2023

View reviewed changes

davidwengier merged commit aae160a into dotnet:main Dec 12, 2023
22 of 24 checks passed

davidwengier deleted the RazorCohost branch December 12, 2023 22:40

ghost added this to the Next milestone Dec 12, 2023

allisonchou mentioned this pull request Dec 14, 2023

[Automated] PRs inserted in VS build main-34414.81 #71267

Closed

Cosifne modified the milestones: Next, 17.9 P3 Jan 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a Razor language server cohosted in Roslyn #70819

Create a Razor language server cohosted in Roslyn #70819

davidwengier commented Nov 15, 2023 •

edited

Loading

davidwengier Nov 15, 2023

davidwengier Nov 15, 2023

davidwengier Nov 15, 2023

davidwengier Nov 15, 2023

davidwengier Nov 15, 2023 •

edited

Loading

dibarbet left a comment

dibarbet Nov 15, 2023

davidwengier Nov 15, 2023

dibarbet Nov 15, 2023

davidwengier Nov 15, 2023

davidwengier commented Nov 19, 2023

davidwengier Nov 24, 2023

dibarbet Nov 27, 2023

davidwengier commented Dec 5, 2023

davidwengier commented Dec 11, 2023

dibarbet Dec 11, 2023

davidwengier Dec 11, 2023

davidwengier Dec 12, 2023 •

edited

Loading

dibarbet Dec 12, 2023

davidwengier Dec 12, 2023

dibarbet Dec 12, 2023

davidwengier Dec 12, 2023

davidwengier commented Dec 12, 2023


		namespace Microsoft.CodeAnalysis.ExternalAccess.Razor.Cohost;

		internal abstract class AbstractRazorCohostRequestHandler<TRequestType, TResponseType> : ILspServiceRequestHandler<TRequestType, TResponseType>

Create a Razor language server cohosted in Roslyn #70819

Create a Razor language server cohosted in Roslyn #70819

Conversation

davidwengier commented Nov 15, 2023 • 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

davidwengier Nov 15, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier commented Nov 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier commented Dec 5, 2023

davidwengier commented Dec 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier Dec 12, 2023 • 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

davidwengier commented Dec 12, 2023

davidwengier commented Nov 15, 2023 •

edited

Loading

davidwengier Nov 15, 2023 •

edited

Loading

davidwengier Dec 12, 2023 •

edited

Loading