Fix issues with pull diagnostics in VSCode #73248
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -120,16 +120,6 @@ public ServerCapabilities GetCapabilities(ClientCapabilities clientCapabilities) | |||
| // Using VS server capabilities because we have our own custom client. | |||
| capabilities.OnAutoInsertProvider = new VSInternalDocumentOnAutoInsertOptions { TriggerCharacters = ["'", "/", "\n"] }; | |||
|
|
|||
| if (!supportsVsExtensions) | |||
| { | |||
| capabilities.DiagnosticOptions = new DiagnosticOptions | |||
There was a problem hiding this comment.
this was the root cause of duplicate diagnostics in vscode - we were registering for requests with no identifier and also for requests with an identifier later on in dynamic registration
| @@ -45,11 +45,11 @@ public DiagnosticSourceManager([ImportMany] IEnumerable<IDiagnosticSourceProvide | |||
| .ToImmutableDictionary(kvp => kvp.Name, kvp => kvp); | |||
| } | |||
|
|
|||
| public ImmutableArray<string> GetDocumentSourceProviderNames() | |||
| => _nameToDocumentProviderMap.Keys.ToImmutableArray(); | |||
| public ImmutableArray<string> GetDocumentSourceProviderNames(ClientCapabilities clientCapabilities) | |||
There was a problem hiding this comment.
Allows us to filter the diagnostic sources based on the client (e.g. no task diagnostics in vscode).
| // that also sets the workspace pull option. | ||
| // | ||
| // So we build up a unique set of source names and mark if each one is also a workspace source. | ||
| var allSources = documentSources |
There was a problem hiding this comment.
previously the doc handler registered doc sources and the workspace handler registered workspace sources. However this meant that sources using the same identifer for doc and workspace would overwrite each other - because vscode requires us to register all identifiers as document sources (even if they are workspace only).
now we register them all once (and set the workspace option appropriately if it also participates in workspace requests).
| @@ -114,14 +114,15 @@ private protected override TestAnalyzerReferenceByLanguage CreateTestAnalyzersRe | |||
| TestLspServer testLspServer, | |||
| ImmutableArray<(string resultId, TextDocumentIdentifier identifier)>? previousResults, | |||
| bool useProgress, | |||
| string? category, | |||
There was a problem hiding this comment.
fix tests to pass category in public requests.
|
|
||
| return results.Select(r => (r.ResultId, r.TextDocument)).ToImmutableArray(); | ||
| // If there was no resultId provided in the response, we cannot create previous results for it. | ||
| return results.Where(r => r.ResultId != null).Select(r => (r.ResultId!, r.TextDocument)).ToImmutableArray(); |
There was a problem hiding this comment.
fixes a bug in the tests discovered when enabling categories for public workspace diagnoistics - we incorrectly sent back a non-null PreviousResultId object with a null resultId (not allowed)
| @@ -73,14 +73,14 @@ public async Task TestDocumentDiagnosticsForOpenFilesWithFSAOff(bool useVSDiagno | |||
| } | |||
|
|
|||
| [Theory, CombinatorialData, WorkItem("https://github.com/dotnet/fsharp/issues/15972")] | |||
| public async Task TestDocumentDiagnosticsForOpenFilesWithFSAOff_Categories(bool mutatingLspWorkspace) | |||
| public async Task TestDocumentDiagnosticsForOpenFilesWithFSAOff_Categories(bool useVSDiagnostics, bool mutatingLspWorkspace) | |||
There was a problem hiding this comment.
Modified the tests to test categories in the public version as well.
| @@ -181,37 +181,14 @@ static void Main(string[] args) | |||
| } | |||
|
|
|||
| [Theory, CombinatorialData] | |||
| public async Task TestDocumentTodoCommentsDiagnosticsForOpenFile_NoCategory(bool useVSDiagnostics, bool mutatingLspWorkspace) | |||
| public async Task TestDocumentTodoCommentsDiagnosticsForOpenFile_Category(bool mutatingLspWorkspace) | |||
There was a problem hiding this comment.
modified the tests to correctly only test todo comments in the VS version.
src/Features/LanguageServer/ProtocolUnitTests/Diagnostics/DiagnosticRegistrationTests.cs
Show resolved
Hide resolved
…nosticRegistrationTests.cs Co-authored-by: Cyrus Najmabadi <[email protected]>
VSCode integration tests caught an issue with the mef based pull diagnostics support in the insertion - dotnet/vscode-csharp#7069
Essentially diagnostics were getting duplicated due to the registration of source names and the static registration (of no source name).
This fixes the issue and also cleans up a few bits and pieces around the source handling in vscode.