Fix LSP duplicate error messages#44874
Conversation
src/VisualStudio/Core/Def/Implementation/LanguageClient/InProcLanguageServer.cs
Outdated
Show resolved
Hide resolved
jasonmalinowski
left a comment
There was a problem hiding this comment.
Isn't this a bug on the Razor side then? The original design here was we are responsible for publishing our diagnostics; it was only the Razor diagnostics we need to suppress since that was being reported by the Razor provider and not us. If they're asking for diagnostics for regular files and publishing those...can they not do that?
In any case this should target master and not master-vs-deps; there's nothing using VS APIs here.
@jasonmalinowski Since diagnostics are pushed from the server, no one is 'asking' for them per se - the razor c# language server just hears about diagnostic update events from the diagnostic service and publishes them. So I'd think that the razor C# language server publishes razor diagnostics and only razor diagnostics, and the c# language server publishes diagnostics for only for c# (and not razor c# diagnostics) |
|
Oh, right, forgot this was still push. 😦 So how did this work before? Or did it never properly work? Was Razor filtering the messages before but isn't now? |
ae39283 to
c5f2778
Compare
|
Hmm, I don't have much background knowledge on the original Razor implementation. @dibarbet do you have any idea? |
|
@allisonchou @jasonmalinowski It's very possible it never worked, I don't recall exactly when I started seeing duplicates. Razor couldn't have been filtering, as they cannot intercept push notifications from the server. Could have been some weird interactions where the razor client was not being activated unless a razor file was opened? |
src/VisualStudio/Core/Def/Implementation/LanguageClient/InProcLanguageServer.cs
Outdated
Show resolved
Hide resolved
Sounds like nobody knows if this ever worked.
| var protocol = ((TestWorkspace)workspace).ExportProvider.GetExportedValue<LanguageServerProtocol>(); | ||
|
|
||
| var languageServer = new InProcLanguageServer(inputStream, outputStream, protocol, workspace, mockDiagnosticService, clientName: "RazorCSharp"); | ||
| var languageServer = new InProcLanguageServer(inputStream, outputStream, protocol, workspace, mockDiagnosticService, clientName: null); |
There was a problem hiding this comment.
I talked to David and it's unclear why clientName had a value of RazorCSharp in the first place. I believe these tests should just be using the regular C# language client.
|
Quick update - I talked to Taylor earlier today, and he indicated that the fix shouldn't negatively impact anything on the Razor side. The fix appears to be working properly for both C# and Razor files. The original issue likely stems from the ClientName attribute being ignored by Liveshare, so the Razor client always activates. |
Fixes https://devdiv.visualstudio.com/DevDiv/_queries/edit/1135206/
The Razor language client is publishing diagnostics for non-Razor files on top of the regular C#/VB language client, resulting in duplicate diagnostics.
This is likely because the
DocumentPropertiesService.DiagnosticsLspClientNameproperty is only being set when the diagnostic is within a Razor document.To get around this, if the property is not set, we assume that the diagnostic is within a C#/VB file and thus the diagnostic should only be reported by the C#/VB language client.