Razor assembly redirector #78852
Razor assembly redirector #78852
Conversation
- Remove the special casing for razor in projectsystemproject - Remove HostDiagnosticAnalyzerProvider as it is no longer needed - Add an EA redirector that razor can implement - MEF import the redirectors where needed - Remove RazorSourceGenerator option from language server
chsienki
force-pushed
the
razor_assembly_redirector
branch
from
ad2e8df to
bd3fb8f
Compare
- The VS Code C# extension is still passing the argument and we error if we get an unrecognized option. We can remove this once we update the C# side.
| var sessionId = parseResult.GetValue(sessionIdOption); | ||
| var extensionAssemblyPaths = parseResult.GetValue(extensionAssemblyPathsOption) ?? []; | ||
| var devKitDependencyPath = parseResult.GetValue(devKitDependencyPathOption); | ||
| var razorSourceGenerator = parseResult.GetValue(razorSourceGeneratorOption); |
There was a problem hiding this comment.
I assume this is the option we need to keep as you say in the PR's description. Consider adding a comment near the option definition.
| [Export(typeof(IAnalyzerAssemblyRedirector)), Shared] | ||
| [method: ImportingConstructor] | ||
| [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| internal sealed class RazorAnalyzerAssemblyRedirector([Import(AllowDefault = true)] RazorAnalyzerAssemblyRedirector.IRazorAnalyzerAssemblyRedirector? razorRedirector = null) : IAnalyzerAssemblyRedirector |
There was a problem hiding this comment.
Nit: Line is getting a little long here and wrapping, consider breaking it up a bit.
| Path.Combine(TempRoot.Root, "Dir", "File.dll") | ||
| }, environment.Workspace.CurrentSolution.Projects.Single().AnalyzerReferences.Select(Function(r) r.FullPath)) | ||
| End Using | ||
| End Function |
There was a problem hiding this comment.
A bit worried about the drop in test coverage. Will we get this covered in the razor side of the change?
There was a problem hiding this comment.
Yes. This test is literally checking that if we pass "razorcompiler.dll" to the project system we redirect it. We no longer have that logic hard coded into the project system, so can't really test it.
I did start making a test with a TestRazorRedirector injected, but then all its really testing is the test redirector, so it doesn't actually provide any value.
Testing that redirection works in general is covered already by RedirectedAnalyzers_CSharp
dibarbet
left a comment
dibarbet
left a comment
There was a problem hiding this comment.
one part got deleted that I don't think should be
| await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(alwaysYield: true, cancellationToken); | ||
| _visualStudioWorkspaceImpl.Services.GetRequiredService<VisualStudioMetadataReferenceManager>(); | ||
|
|
||
| _visualStudioWorkspaceImpl.SubscribeExternalErrorDiagnosticUpdateSourceToSolutionBuildEvents(); |
There was a problem hiding this comment.
I don't think these should be deleted. This is for listening to build events for legacy project system build diagnostics so we can push them to the error list (long story).
I'm not sure about VisualStudioMetadataReferenceManager, but I also don't think it should necessarily be deleted either.
There was a problem hiding this comment.
Ah, I hadn't realized that there was other stuff going on in that initializer other than getting the analyzer provider factory. I've restored the original logic and just removed the return for the provider.
Not sure if CreateAndAddToWorkspaceAsync needs to wait until the init task is finished so I've added it back just in case.
There was a problem hiding this comment.
I didn't quite catch the history of this, but this final form looks correct. We do expect the other stuff to run before the first load of a project or otherwise I think metadata reference reading might fail in bad ways.
|
|
||
| // HACK: Fetch this service to ensure it's still created on the UI thread; once this is | ||
| // moved off we'll need to fix up it's constructor to be free-threaded. | ||
| _threadingContext.JoinableTaskFactory.RunAsync( |
There was a problem hiding this comment.
tagging @jasonmalinowski - do we still need to ensure that CreateAndAddToWorkspace awaits the result of this? e..g for the VisualStudioMetadataReferenceManager part?
If we don't need to await, this should likely be tracked with a ReportNonFatal continuation
There was a problem hiding this comment.
Apologies, you reviewed quicker than I could update the code and force pushed with the await also replaced.
chsienki
force-pushed
the
razor_assembly_redirector
branch
from
e750196 to
420044c
Compare
chsienki
force-pushed
the
razor_assembly_redirector
branch
from
420044c to
fe5a33a
Compare
|
Val build here https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/644653 passing since I restored the deleted code. |
7 participants
Part of #76868
This removes the special casing in
ProjectSystemProjectfor the razor compiler. Instead, we make use of theIAnalyzerAssemblyRedirectorinterface to provide a component via MEF that handles the redirection. That interface isn't public, so we expose an interface via the Razor EA that allows razor to provide the redirection.This works for both VS and VS Code, which means it is no longer necessary to pass in the path the razor source generator in VS Code, nor to provide a
HostDiagnosticAnalyzerProviderthat passes the razor compiler to the project system.The razor side is waiting on this PR to be able to make use of the new EA interface, but a dual insertion isn't strictly necessary. For the period when this goes in, but razor hasn't yet updated, there will be no redirection (the generator will load from the SDK instead), but things will still work. The plan is to get the razor side in before this change ships to any customers, so there shouldn't be any time where a publicly available build has no redirection.
There is also a small piece in the C# extension where we pass the razor source generator path. We need to keep the argument here until we update that codebase not to pass it.