Make Razor a built in feature of the Roslyn language server#83575
Make Razor a built in feature of the Roslyn language server#83575davidwengier wants to merge 9 commits intodotnet:mainfrom
Conversation
Local testing is not just one setting (dotnet.server.path) so simply enough to not need a skill IMO
There was a problem hiding this comment.
Pull request overview
This PR integrates the Razor VS Code component directly into the Roslyn language server so Razor functionality ships as part of the same tool/package, rather than being provided via externally-supplied component paths.
Changes:
- Removes command-line/config plumbing for supplying Razor design-time inputs via arguments.
- Adds the VS Code Razor extension project as a runtime dependency of the language server and ensures its entry assembly participates in the language server MEF catalog.
- Updates project loading to always set
RazorDesignTimeTargetsbased on the language server deployment layout.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/ServerConfigurationFactory.cs | Removes RazorDesignTimePath from ServerConfiguration. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs | Removes Razor-related CLI options and stops flowing Razor design-time paths into configuration. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Microsoft.CodeAnalysis.LanguageServer.csproj | Adds a project reference to Microsoft.VisualStudioCode.RazorExtension so it’s a runtime dependency. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServerExportProviderBuilder.cs | Adds Razor extension assembly to MEF discovery and excludes specific Razor/remote assemblies from the main MEF catalog. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs | Always sets RazorDesignTimeTargets to a path under AppContext.BaseDirectory. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/Utilities/LanguageServerTestComposition.cs | Updates test server configuration construction to match the new ServerConfiguration shape. |
| .github/skills/run-vscode-local-testing/SKILL.md | Removes the local VS Code testing skill documentation. |
| .github/skills/run-vscode-local-testing/scripts/Start-LocalVsCode.ps1 | Removes the helper script used by the deleted skill. |
| Required = false | ||
| }; | ||
|
|
||
| var razorSourceGeneratorOption = new Option<string?>("--razorSourceGenerator") |
There was a problem hiding this comment.
Was this unused? Not seeing anything removed that actually touched this option
There was a problem hiding this comment.
Yep, it was, the option value stopped getting passed to ServerConfiguration in #78852, and this either didn't get cleaned up, or left as an option so a coordinated change wasn't necessary.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Razor dynamic registration is sometimes coming into play when running multiple tests.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServerExportProviderBuilder.cs:73
- The MEF exclusion list is only applied when enumerating assemblies in the base directory, but extension-provided assemblies are added unfiltered. If an older VS Code extension (or other host) still passes Razor/Remote.ServiceHub DLLs via ExtensionAssemblyPaths, they'll be reintroduced into the MEF catalog and can trigger the same export collisions this exclusion list is trying to prevent. Consider applying the same filtering to extensionManager.ExtensionAssemblyPaths (or explicitly skipping these known-conflicting DLL names) when adding extension assemblies to the catalog.
assemblyPathsBuilder.AddRange(FilterFiles(baseDirectory, "Microsoft.CodeAnalysis*.dll"));
assemblyPathsBuilder.AddRange(FilterFiles(baseDirectory, "Microsoft.ServiceHub*.dll"));
// The Razor extension ships with Roslyn and so needs to be in our MEF composition
assemblyPathsBuilder.Add(Path.Combine(baseDirectory, "Microsoft.VisualStudioCode.RazorExtension.dll"));
// DevKit assemblies are not shipped in the main language server folder
// and not included in ExtensionAssemblyPaths (they get loaded into the default ALC).
// So manually add them to the MEF catalog here.
if (devKitDependencyPath != null)
assemblyPathsBuilder.Add(devKitDependencyPath);
// Add the extension assemblies to the MEF catalog.
assemblyPathsBuilder.AddRange(extensionManager.ExtensionAssemblyPaths);
|
Got a correctness failure for missing runtime identifiers, and while fixing that I realised we don't need to publish the razor package any more, so there have been a couple of smallish updates since last review, FYI |
|
/pr-val |
|
View PR Validation Run triggered by @davidwengier Parameters
|
| { | ||
| foreach (var file in Directory.EnumerateFiles(baseDirectory, filter)) | ||
| { | ||
| if (!s_dllsToExcludeFromMef.Contains(Path.GetFileName(file))) |
There was a problem hiding this comment.
Yeah, good catch, doesn't hurt to be a bit more forgiving here.
|
RPS and Speedometer are happy :) |
|
|
||
| // Get rid of the registration and it should be gone again | ||
| watchedFile.Dispose(); | ||
| context.Dispose(); |
There was a problem hiding this comment.
Disposing this before the assert might be defeating the point this part of the test. Should line 90 just become a using context instead?
There was a problem hiding this comment.
The missing dispose is something copilot noticed, but I did think the same thing. The test above already has the context.Dispose call, and it is before the wait and assert though, so I assumed it to be correct.
| <Nullable>enable</Nullable> | ||
| <IsShippingPackage>true</IsShippingPackage> | ||
| <!-- This assembly ships inside roslyn-language-server, not as its own NuGet package. --> | ||
| <IsShippingAssembly>false</IsShippingAssembly> |
There was a problem hiding this comment.
What does IsShippingAssembly mean? I'd assume want this as true given the comment?
There was a problem hiding this comment.
Shipping in this context means "create and push a package in CI", which we're not doing any more for this. It's in the roslyn package, but just brought in as a normal project reference.
| assemblyPathsBuilder.AddRange(FilterFiles(baseDirectory, "Microsoft.CodeAnalysis*.dll")); | ||
| assemblyPathsBuilder.AddRange(FilterFiles(baseDirectory, "Microsoft.ServiceHub*.dll")); |
There was a problem hiding this comment.
I wonder at what point it's just easier to list the files than keep adding fancier logic here.
Now that we're in the same repo, we can just reference the Razor bits directly and ship everything together in one happy package.
C# extension PR: dotnet/vscode-csharp#9277
Microsoft Reviewers: Open in CodeFlow