Fix razor analyzer loading #78116
Fix razor analyzer loading #78116
Conversation
ghost
added
Area-Analyzers
untriaged
chsienki
force-pushed
the
fix_razor_analyzer_loading
branch
from
afcf1aa to
280b420
Compare
- Razor is not going to be listed as an analyzer anymore - Add a version of the extension loading logic that loads the razor specified files in the razor VSIX - Add tests
chsienki
force-pushed
the
fix_razor_analyzer_loading
branch
from
280b420 to
e23e91d
Compare
|
I'll run a val build on this PR and make sure the numbers are what we're expecting. |
|
I don't begin to understand this code, but I will say that cherry-picking the first commit into my local Roslyn clone has fixed a loading issue in VS Code when trying to get cohosting working :) |
That's probably because you don't have the corresponding Razor change, so you've effectively disabled razor redirection :) |
|
@dotnet/roslyn-compiler and @dotnet/razor-compiler for reviews please. |
If you mean the Razor change in the PR you linked, that's not going to be relevant because that only affects VS. But also turns out this isn't working in VS Code, so probably some different changes needed on one side or another there |
src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Diagnostics/VisualStudioDiagnosticAnalyzerProvider.cs
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Workspace/ProjectSystem/IHostDiagnosticAnalyzerProvider.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/TestUtilities2/MockVisualStudioDiagnosticAnalyzerProviderFactory.vb
Show resolved
Hide resolved
src/VisualStudio/Core/Test/Diagnostics/VisualStudioDiagnosticAnalyzerProviderTests.vb
Show resolved
Hide resolved
| } | ||
|
|
||
| public Assembly LoadFromPath(string fullPath) | ||
| if (razorSourceGenerator is not null) |
There was a problem hiding this comment.
The previous code had a File.Exists call, is it okay to remove that here? Also are we sure this points to the generator DLL and not the generator directory?
There was a problem hiding this comment.
Basically the old code, intentionally or accidentally, guarded against a directory being specified with the File.Exists call. That is removed now. So if someone is passing the razor generator directory in a VS code setting what will happen now?
There was a problem hiding this comment.
Ah yeah, good catch, will put the exists back in.
src/VisualStudio/Core/Test/ProjectSystemShim/VisualStudioProjectTests/AnalyzerReferenceTests.vb
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Test/ProjectSystemShim/VisualStudioProjectTests/AnalyzerReferenceTests.vb
Outdated
Show resolved
Hide resolved
|
@jaredpar Any more thoughts on this? |
|
/backport to release/dev17.14 |
|
Started backporting to release/dev17.14: https://github.com/dotnet/roslyn/actions/runs/14600722892 |
|
@chsienki backporting to "release/dev17.14" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Separate out razor redirection: - Razor is not going to be listed as an analyzer anymore - Add a version of the extension loading logic that loads the razor specified files in the razor VSIX - Add tests
Using index info to reconstruct a base tree...
M src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/HostDiagnosticAnalyzerProvider.cs
M src/VisualStudio/Core/Def/Diagnostics/VisualStudioDiagnosticAnalyzerProvider.cs
Falling back to patching base and 3-way merge...
Auto-merging src/VisualStudio/Core/Def/Diagnostics/VisualStudioDiagnosticAnalyzerProvider.cs
Auto-merging src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/HostDiagnosticAnalyzerProvider.cs
CONFLICT (content): Merge conflict in src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/HostDiagnosticAnalyzerProvider.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Separate out razor redirection: - Razor is not going to be listed as an analyzer anymore - Add a version of the extension loading logic that loads the razor specified files in the razor VSIX - Add tests
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
* Seperate out razor redirection: - Razor is not going to be listed as an analyzer anymore - Add a version of the extension loading logic that loads the razor specified files in the razor VSIX - Add tests
Today Razor lists the compiler/source generator as a VSIX analyzer. Roslyn uses this information to find the location of the razor compiler assembly so that it can redirect the SDK copy to the copy shipped in the Razor VSIX.
Previously this was ok, as the assemblies only contained a source generator, not any analyzers, so listing them as VSIX analyzers was essentially a NOP. As of dotnet/razor#11601 however, the assemblies now contain a diagnostic suppressor. Because it's listed as a VSIX analyzer, VS now loads the suppressor in all scenarios, even if razor isn't involved.
This change (along with the matching razor one here dotnet/razor#11731) instead reads razor via a custom asset type. The logic is basically the same but means that the compiler will no longer be listed as a VSIX analyzer, and so the suppressor won't be loaded in scenarios where razor isn't involved.
I don't love making yet more custom Razor stuff, but there is a bigger change here to come, where we can actually remove the razor redirection logic from Roslyn altogether (including the new
GetRazorReferencesInExtensionscall added here), so hopefully this is only a temporary state of affairs.