Switch liveshare to use LSP pull diagnostics and cleanup server name usages #57596
Conversation
ghost
added
the
| private void DiagnosticService_DiagnosticsUpdated(object? _, DiagnosticsUpdatedArgs e) | ||
| => DiagnosticService_DiagnosticsUpdated(e.Solution, e.DocumentId); | ||
|
|
||
| private void DiagnosticService_DiagnosticsUpdated(Solution? solution, DocumentId? documentId) |
dibarbet
added
LSP
src/EditorFeatures/Core/Implementation/LanguageServer/VisualStudioInProcLanguageServer.cs
Show resolved
Hide resolved
| @@ -396,15 +396,15 @@ static LSP.Location ConvertTextSpanWithTextToLocation(TextSpan span, SourceText | |||
| private static RequestDispatcher CreateRequestDispatcher(TestWorkspace workspace) | |||
| { | |||
| var factory = workspace.ExportProvider.GetExportedValue<RequestDispatcherFactory>(); | |||
| return factory.CreateRequestDispatcher(ProtocolConstants.RoslynLspLanguages); | |||
| return factory.CreateRequestDispatcher(ProtocolConstants.RoslynLspLanguages, WellKnownLspServerKinds.AlwaysActiveLspServer); | |||
There was a problem hiding this comment.
i'm a bit nervous about all tests just getting this particular server. should tests indicate which servers to run against so we can test them all?
There was a problem hiding this comment.
So 99% of the time this is just used for telemetry purposes. The functionality of all the servers is essentially the same and are instead gated based on the capabilities they provide. They really shouldn't differ in functionality, it is only the special case of diagnostics that we need to know the server.
I've added tests to verify the server based behavior for diagnostics, but otherwise left the default value to be this.
src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs
Show resolved
Hide resolved
src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs
Outdated
Show resolved
Hide resolved
src/Features/LanguageServer/Protocol/WellKnownLspServerKinds.cs
Outdated
Show resolved
Hide resolved
| /// XAML LSP servers. | ||
| /// </summary> | ||
| XamlLspServer, | ||
| XamlLspServerDisableUX, |
There was a problem hiding this comment.
=-o
we have a xaml lsp server?
There was a problem hiding this comment.
We do in fact, I believe they access solution snapshots and so take advantage of our queueing mechanisms.
There was a problem hiding this comment.
@CyrusNajmabadi Yes, it's used for LiveShare and it's still internally experimental for the regular client.
src/Features/LanguageServer/Protocol/WellKnownLspServerKinds.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Diagnostics/InternalDiagnosticsOptions.cs
Outdated
Show resolved
Hide resolved
| /// Diagnostic mode setting for Live Share. This should always be <see cref="DiagnosticMode.Pull"/> as there is no push support in Live Share. | ||
| /// This option is only for passing to the diagnostics service and can be removed when we switch all of Roslyn to LSP pull. | ||
| /// </summary> | ||
| private static readonly Option2<DiagnosticMode> s_liveShareDiagnosticMode = new(nameof(InternalDiagnosticsOptions), "LiveShareDiagnosticMode", defaultValue: DiagnosticMode.Pull); |
This switches liveshare pre-emptively over to LSP pull (from LSP push) and removes the razor feature flag for LSP push.
This should fix a couple scenarios where liveshare diagnostics were incorrectly placed, as well as allowing us to completely remove our LSP push diagnostics implementation.
Also consolidated some of the usages of server name into an enum to be clearer and allow handlers to switch off server kind.
TODO - verify live share client side works as expected.Verified in 17.0p5, there is currently a liveshare bug breaking everything in int.preview - https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1429983Verified in 17.1p4