Update Razor EA #77194

ryzngard · 2025-02-13T00:02:52Z

~~Commit by commit tells a story~~

The story started strong but is in shambles now.

The goal of this is to start separating the Razor EA from things requiring EditorFeatures and things that do not. Some things are left clunky due to not wanting to break/dual insert into VS.

For VS Code a dual insertion will be needed. Razor will now be loaded into the Roslyn LSP server through a new binary "Microsoft.VisualStudioCode.RazorExtension" which will ship in the .razor folder in vs code. This will provide all of the hooks necessary to provide dynamic file info as well as any LSP endpoints needed in MS.CA.LanguageServer for razor to function.

/Shared : will eventually be the area for all source items shared between the current EA and the new EA.Razor.Features /Features : will have an EA that is restricted to depending only on the features layer. This will help provide a way to ship a more sane EA structure for VS Code as well as reduce the likelyhood and work required for a dual insertion /EditorFeatures : contains the current EA. The project name is kept the same for now but the folder is moved. This denotes that the binary does depend on the EditorFeatures layer in Roslyn Code changes limited to updating references and pathing only. No new/edited/removed code.

… razor in LSP that is based on reflection. Initialize and Dynamic file will use this to handle razor logic.

Shared contains mostly contracts and a few data classes/wrappers that are common between VS/VS Code. The layering itself is not strictly for those clients but instead is meant to be a split to remove the dependency upon EditorFeatures. Prepare for Features layer to load an IRazorServiceProvider to be used in Microsoft.CodeAnalysis.LanguageServer Remove a lot of the implementation logic from Microsoft.CodeAnalysis.LanguageServer. This should now be provided from the razor assembly that is loaded in the LanguageServer via RazorLSPServiceProvider Remove shared code from the existing EA.

Publish MSCA.EA.Razor.Features Remove IsExternalInit

davidwengier

The layout makes sense, had a couple of questions, but probably just be not seeing the bigger picture

davidwengier · 2025-02-13T02:18:39Z

...ls/ExternalAccess/Razor/Features/Microsoft.CodeAnalysis.ExternalAccess.Razor.Features.csproj

+    <!--
+      This is used to load in the language server so it gets special treatment. Otherwise only razor assemblies should be added.
+    -->
+    <InternalsVisibleTo Include="Microsoft.CodeAnalysis.LanguageServer" />


Isn't this going to cause a problem later, when we need the language server to have IVT to this project, so we can put LSP endpoints in it? Or is that going to go through the IRazorServiceProvider?

Might need to chat with you. My understanding of how the lsp endpoints got hooked up is just a normal export with a razor class in the middle. That's what I thought co-hosting was doing.

Can we hook up endpoints through the service provider? Might make things easier and cleaner

It's an export, yeah, but the type being exported has to inherit from something like ILspServiceRequestHandler which we expose via our EA.

BUT I just looked it up to be sure, and actually I'm wrong, and I think this will be fine, because that type is in MS.CA.LS.Protocol, so maybe we can get away with never needing to expose anything from the language server itself through our EA.

davidwengier · 2025-02-13T02:20:50Z

.../ExternalAccess/Razor/EditorFeatures/Cohost/RazorCohostClientLanguageServerManagerFactory.cs

 using Microsoft.CodeAnalysis.Host.Mef;
 using Microsoft.CodeAnalysis.LanguageServer;

 namespace Microsoft.CodeAnalysis.ExternalAccess.Razor.Cohost;

 [Shared]
-[ExportCohostLspServiceFactory(typeof(IRazorCohostClientLanguageServerManager))]
+[ExportCohostLspServiceFactory(typeof(IRazorClientLanguageServerManager))]


FYI, this makes this PR binary breaking so we need a dual insertion. Not necessarily the end of the world since this would be the 3rd in line, but wanted to note it at least. If this is the only break, it could be pretty trivially avoided if that was preferred.

Ah yea. I can do without this change for now to avoid a binary break

To be clear though, the new name makes sense, so perhaps export this twice, once with the old and once with the new name, so we can move over to it in time.

src/VisualStudio/Setup/Roslyn.VisualStudio.Setup.csproj

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

ryzngard · 2025-02-19T20:37:36Z

This is at least in a state where VS shouldn't break or require a dual insertion. Marking ready for review.

VS Code will need a dual insertion. Hopefully one of the last ones....

davidwengier

Nit: Should remove "[DRAFT]" from the PR title 😛

davidwengier · 2025-02-19T21:14:36Z

Roslyn.sln

+Project("{D954291E-2A0B-460D-934E-DC6B0785DB48}") = "Microsoft.CodeAnalysis.ExtnernalAccess.Razor.Shared", "src\Tools\ExternalAccess\Razor\Shared\Microsoft.CodeAnalysis.ExtnernalAccess.Razor.Shared.shproj", "{4853A78A-4EC4-4D86-9F02-D0DDEAE85520}"
+EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Microsoft.CodeAnalysis.ExternalAccess.Razor.Features", "src\Tools\ExternalAccess\Razor\Features\Microsoft.CodeAnalysis.ExternalAccess.Razor.Features.csproj", "{D5A8E20C-E8D2-4A57-906A-263994D8731D}"
+EndProject
 Global


The external access projects will continue until morale improves!

I asked co-pilot when that would be and...

...geServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/Razor/RazorInitializeHandler.cs

davidwengier · 2025-02-19T22:07:21Z

...er/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/Razor/RazorDynamicFileInfoProvider.cs

+        await dynamicFileInfoProvider.EnsureInitializedAsync(
+            static ct =>
+            {
+                return RazorLSPServiceProvider.GetRequiredServiceAsync<IRazorClientLanguageServerManager>(ct);


Can you add a comment explaining this indirection? Seems odd to initialize a razor service with a delegate to get a different razor service from a razor service provider. Can the initialize method not take the IRazorClientLanguageServerManager directly? Or even better, the IRazorServiceProvider in case its dependencies change?

davidwengier · 2025-02-19T22:09:09Z

src/Tools/ExternalAccess/Razor/Features/IRazorLSPDynamicFileInfoProvider.cs

+
+namespace Microsoft.CodeAnalysis.ExternalAccess.Razor.Features;
+
+internal interface IRazorLSPDynamicFileInfoProvider


"LSP" in the name here seems odd. Isn't this just an IRazorDynamicFileInfoProvider, and the implementation happens to use LSP in VS Code, and not in VS?

The only thing that is "LSP-y" about this seems to be the IRazorClientLanguageServerManager bit on the initialize method, so maybe my above comment about passing in a IRazorServiceProvider would resolve that?

Yea, let me rethink this. The main thing is that the LSP portion = needs a way to communicate using IRazorClientLanguageServerManager. Let me see if I can get rid of the initialize method and change the name

src/Tools/ExternalAccess/Razor/Features/RazorDocumentServiceProviderWrapper.cs

src/Tools/ExternalAccess/Razor/Shared/RazorMappedSpanResult.cs

…where needed

ryzngard added 4 commits February 12, 2025 15:59

Move services to common location. Start using a service provider from…

8e90e75

… razor in LSP that is based on reflection. Initialize and Dynamic file will use this to handle razor logic.

Fix package.csproj

77a9181

Publish MSCA.EA.Razor.Features Remove IsExternalInit

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 13, 2025

dotnet-policy-service bot added VSCode Needs API Review Needs to be reviewed by the API review council labels Feb 13, 2025

ryzngard changed the title ~~Update Razor EA~~ [DRAFT] Update Razor EA Feb 13, 2025

davidwengier reviewed Feb 13, 2025

View reviewed changes

ryzngard added 4 commits February 13, 2025 15:00

Fix IsExternalInit path

004e973

Restore project references

73a6d80

PR Feedback

a7fb1bd

Merge branch 'main' into update_razor_ea

9ea28e9

ryzngard mentioned this pull request Feb 19, 2025

Remove Razor EA For Roslyn dotnet/razor#11510

Draft

ryzngard added 2 commits February 19, 2025 11:01

Merge branch 'main' into update_razor_ea

bb610d4

Fix breaking changes to avoid dual insertion

a6910e6

ryzngard marked this pull request as ready for review February 19, 2025 20:37

ryzngard requested review from a team as code owners February 19, 2025 20:37

davidwengier reviewed Feb 19, 2025

View reviewed changes

ryzngard changed the title ~~[DRAFT] Update Razor EA~~ Update Razor EA Feb 19, 2025

ryzngard added 6 commits February 19, 2025 17:40

Remove custom service provider. Use ILspServer and initialize method …

f2a633d

…where needed

Add an EndpointAttribute usable for VS Code

03f213b

Final changes needed to expose needed services and endpoint hookup

35d000d

Hook up to the updated event

3338b6d

Make ABWQ an implementation detail for the razor side

41ccc70

Cleanup

e390b66

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Razor EA #77194

Update Razor EA #77194

ryzngard commented Feb 13, 2025 •

edited

Loading

davidwengier left a comment

davidwengier Feb 13, 2025

ryzngard Feb 13, 2025

davidwengier Feb 13, 2025

davidwengier Feb 13, 2025

ryzngard Feb 13, 2025

davidwengier Feb 13, 2025

ryzngard commented Feb 19, 2025

davidwengier left a comment

davidwengier Feb 19, 2025

ryzngard Feb 19, 2025

davidwengier Feb 19, 2025

davidwengier Feb 19, 2025

ryzngard Feb 19, 2025


		namespace Microsoft.CodeAnalysis.ExternalAccess.Razor.Features;

		internal interface IRazorLSPDynamicFileInfoProvider

Update Razor EA #77194

Are you sure you want to change the base?

Update Razor EA #77194

Conversation

ryzngard commented Feb 13, 2025 • edited Loading

davidwengier 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzngard commented Feb 19, 2025

davidwengier 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

Choose a reason for hiding this comment

ryzngard commented Feb 13, 2025 •

edited

Loading