Fix for LSP diagnostic bugs#41619
Conversation
dibarbet
left a comment
There was a problem hiding this comment.
Mostly looks good, just a couple async concerns
| DocumentRangeFormattingProvider = true, | ||
| DocumentOnTypeFormattingProvider = new DocumentOnTypeFormattingOptions { FirstTriggerCharacter = "}", MoreTriggerCharacter = new[] { ";", "\n" } }, | ||
| DocumentHighlightProvider = true, | ||
| TextDocumentSync = new TextDocumentSyncOptions { OpenClose = true } |
There was a problem hiding this comment.
Do we need this since the notifications aren't propagated through live share yet?
|
|
||
| #pragma warning disable VSTHRD100 // Avoid async void methods | ||
| private async void DiagnosticService_DiagnosticsUpdated(object sender, DiagnosticsUpdatedArgs e) | ||
| private async void PublishDiagnosticsAsync(Solution solution, Document document) |
There was a problem hiding this comment.
This should just return Task, then you can remove the suppression.
| var document = _workspace.CurrentSolution.GetDocument(documentId); | ||
| if (document != null) | ||
| { | ||
| PublishDiagnosticsAsync(_workspace.CurrentSolution, document); |
There was a problem hiding this comment.
PublishDiagnosticsAsync should return a task, then can return the result of PublishDiagnosticsAsync here. Right now it will trigger PublishDiagnosticsAsync and will return a CompletedTask before PublishDiagnosticsAsync completes.
| return await _protocol.GetWorkspaceSymbolsAsync(_workspace.CurrentSolution, workspaceSymbolParams, _clientCapabilities, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| private void DiagnosticService_DiagnosticsUpdated(object sender, DiagnosticsUpdatedArgs e) |
There was a problem hiding this comment.
Since this is still the one triggered by the event handler, this should still be async void and wrapped in the try catch instead of in PublishDiagnosticsAsync.
| { | ||
| // Since this is an async void method, exceptions here will crash the host VS. We catch exceptions here to make sure that we don't crash the host since | ||
| // the worst outcome here is that guests may not see all diagnostics. | ||
| try |
There was a problem hiding this comment.
This try catch should be in the event handler method.
| DocumentRangeFormattingProvider = true, | ||
| DocumentOnTypeFormattingProvider = new DocumentOnTypeFormattingOptions { FirstTriggerCharacter = "}", MoreTriggerCharacter = new[] { ";", "\n" } }, | ||
| DocumentHighlightProvider = true, | ||
| DocumentHighlightProvider = true |
There was a problem hiding this comment.
| DocumentHighlightProvider = true | |
| DocumentHighlightProvider = true, |
| var document = _workspace.CurrentSolution.GetDocument(documentId); | ||
| if (document != null) | ||
| { | ||
| await PublishDiagnosticsAsync(_workspace.CurrentSolution, document).ConfigureAwait(false); |
There was a problem hiding this comment.
continually referencing _workspace.CurrentSolution is very scary. it could change over time. Instead, pull this out above hte foreach look and always reference the same instance. Passing that instance into PublishDiagnosticsAsync shouldn't be necessary either. it's redundant given that document is passed in.
Fixes bug where IDE info diagnostics are reported as warnings with purple squiggles (https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1062989).
Also partially fixes #41029, pending a bug fix from LSP side (https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1067885).