Skip to content

Commit 0ae5973

Browse files
authored
Improve LSP conformance in MS.CA.LanguageServer (#81174)
I am proposing fixes for two LSP conformance issues I discovered while integrating Roslyn LSP with Zed: 1. The response from the `textDocument/diagnostic` request cannot be `null`. MS.CA.LanguageServer currently returns a `null` response in the case described here: https://github.com/dotnet/roslyn/blob/76a39c1a97fe5c73549786fc6944dc188a7b7b3b/src/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticSourceProviders/DiagnosticSourceManager.cs#L72-L80 Because the set of diagnostic handlers is empty, a response value is never generated for the request, and we fall through to returning `null`. Instead, explicitly return an empty list of diagnostics. (This is exactly how VS Code's LSP client treats a `null` response.) 2. Methods that take no parameters (e.g. `workspace/diagnostic/refresh`) were being sent with an empty array as the parameter value. This is because the `InvokeWithCancellationAsync` method in StreamJsonRpc normalizes unspecified arguments to an empty list. Use the `InvokeParameterObjectAsync` method instead, which don't do this.
1 parent 0ed9e05 commit 0ae5973

File tree

5 files changed

+32
-20
lines changed

5 files changed

+32
-20
lines changed

src/LanguageServer/Protocol/Handler/Diagnostics/Public/PublicDocumentPullDiagnosticsHandler.cs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ internal sealed partial class PublicDocumentPullDiagnosticsHandler(
2323
IDiagnosticSourceManager diagnosticSourceManager,
2424
IDiagnosticsRefresher diagnosticsRefresher,
2525
IGlobalOptionService globalOptions)
26-
: AbstractDocumentPullDiagnosticHandler<DocumentDiagnosticParams, DocumentDiagnosticPartialReport, DocumentDiagnosticReport?>(
26+
: AbstractDocumentPullDiagnosticHandler<DocumentDiagnosticParams, DocumentDiagnosticPartialReport, DocumentDiagnosticReport>(
2727
diagnosticsRefresher, diagnosticSourceManager, globalOptions)
2828
{
2929
private readonly IClientLanguageServerManager _clientLanguageServerManager = clientLanguageServerManager;
@@ -57,7 +57,7 @@ protected override bool TryCreateUnchangedReport(TextDocumentIdentifier identifi
5757
return true;
5858
}
5959

60-
protected override DocumentDiagnosticReport? CreateReturn(BufferedProgress<DocumentDiagnosticPartialReport> progress)
60+
protected override DocumentDiagnosticReport CreateReturn(BufferedProgress<DocumentDiagnosticPartialReport> progress)
6161
{
6262
// We only ever report one result for document diagnostics, which is the first DocumentDiagnosticReport.
6363
var progressValues = progress.GetValues();
@@ -71,7 +71,10 @@ protected override bool TryCreateUnchangedReport(TextDocumentIdentifier identifi
7171
return progressValues.Single().Second;
7272
}
7373

74-
return null;
74+
return new RelatedFullDocumentDiagnosticReport
75+
{
76+
Items = []
77+
};
7578
}
7679

7780
protected override ImmutableArray<PreviousPullResult>? GetPreviousResults(DocumentDiagnosticParams diagnosticsParams)

src/LanguageServer/Protocol/Handler/LanguageServerNotificationManager.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,13 @@ public Task<TResponse> SendRequestAsync<TParams, TResponse>(string methodName, T
2727
=> _jsonRpc.InvokeWithParameterObjectAsync<TResponse>(methodName, @params, cancellationToken);
2828

2929
public async ValueTask SendRequestAsync(string methodName, CancellationToken cancellationToken)
30-
=> await _jsonRpc.InvokeWithCancellationAsync(methodName, cancellationToken: cancellationToken).ConfigureAwait(false);
30+
=> await _jsonRpc.InvokeWithParameterObjectAsync(methodName, cancellationToken: cancellationToken).ConfigureAwait(false);
3131

3232
public async ValueTask SendRequestAsync<TParams>(string methodName, TParams @params, CancellationToken cancellationToken)
3333
=> await _jsonRpc.InvokeWithParameterObjectAsync(methodName, @params, cancellationToken).ConfigureAwait(false);
3434

3535
public async ValueTask SendNotificationAsync(string methodName, CancellationToken cancellationToken)
36-
=> await _jsonRpc.NotifyAsync(methodName).ConfigureAwait(false);
36+
=> await _jsonRpc.NotifyWithParameterObjectAsync(methodName).ConfigureAwait(false);
3737

3838
public async ValueTask SendNotificationAsync<TParams>(string methodName, TParams @params, CancellationToken cancellationToken)
3939
=> await _jsonRpc.NotifyWithParameterObjectAsync(methodName, @params).ConfigureAwait(false);

src/LanguageServer/ProtocolUnitTests/Diagnostics/AbstractPullDiagnosticTestsBase.cs

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -271,30 +271,26 @@ private protected static async Task<ImmutableArray<TestDiagnosticResult>> RunGet
271271
else
272272
{
273273
BufferedProgress<DocumentDiagnosticPartialReport>? progress = useProgress ? BufferedProgress.Create<DocumentDiagnosticPartialReport>(null) : null;
274-
var diagnostics = await testLspServer.ExecuteRequestAsync<DocumentDiagnosticParams, SumType<FullDocumentDiagnosticReport, UnchangedDocumentDiagnosticReport>?>(
274+
var diagnostics = await testLspServer.ExecuteRequestAsync<DocumentDiagnosticParams, SumType<FullDocumentDiagnosticReport, UnchangedDocumentDiagnosticReport>>(
275275
Methods.TextDocumentDiagnosticName,
276276
CreateProposedDocumentDiagnosticParams(vsTextDocumentIdentifier, previousResultId, category, progress),
277277
CancellationToken.None).ConfigureAwait(false);
278278
if (useProgress)
279279
{
280-
Assert.Null(diagnostics);
280+
Assert.IsType<FullDocumentDiagnosticReport>(diagnostics.Value);
281+
Assert.Empty(diagnostics.First.Items);
281282
AssertEx.NotNull(progress);
282283
diagnostics = progress.Value.GetValues()!.Single().First;
283284
}
284285

285-
if (diagnostics == null)
286-
{
287-
// The public LSP spec returns null when no diagnostics are available for a document wheres VS returns an empty array.
288-
return [];
289-
}
290-
else if (diagnostics.Value.Value is UnchangedDocumentDiagnosticReport)
286+
if (diagnostics.Value is UnchangedDocumentDiagnosticReport)
291287
{
292288
// The public LSP spec returns different types when unchanged in contrast to VS which just returns null diagnostic array.
293-
return [new TestDiagnosticResult(vsTextDocumentIdentifier, diagnostics.Value.Second.ResultId!, null)];
289+
return [new TestDiagnosticResult(vsTextDocumentIdentifier, diagnostics.Second.ResultId!, null)];
294290
}
295291
else
296292
{
297-
return [new TestDiagnosticResult(vsTextDocumentIdentifier, diagnostics.Value.First.ResultId!, diagnostics.Value.First.Items)];
293+
return [new TestDiagnosticResult(vsTextDocumentIdentifier, diagnostics.First.ResultId!, diagnostics.First.Items)];
298294
}
299295
}
300296

src/LanguageServer/ProtocolUnitTests/Diagnostics/NonLocalDiagnosticTests.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,11 @@ internal async Task TestNonLocalDocumentDiagnosticsAreReportedWhenFSAEnabled(boo
5757
}
5858
else
5959
{
60-
Assert.Empty(results);
60+
Assert.Empty(results.Single().Diagnostics!);
6161

6262
// Asking again should give us back unchanged diagnostics.
6363
var results2 = await RunGetDocumentPullDiagnosticsAsync(testLspServer, document.GetURI(), useVSDiagnostics: false, category: PublicDocumentNonLocalDiagnosticSourceProvider.NonLocal);
64-
Assert.Empty(results2);
64+
Assert.Empty(results.Single().Diagnostics!);
6565
}
6666
}
6767

src/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,26 @@ public async Task TestNoDocumentDiagnosticsForClosedFilesWithFSAOff(bool useVSDi
4545
var document = testLspServer.GetCurrentSolution().Projects.Single().Documents.Single();
4646

4747
var results = await RunGetDocumentPullDiagnosticsAsync(testLspServer, document.GetURI(), useVSDiagnostics);
48-
49-
Assert.Empty(results);
48+
if (useVSDiagnostics)
49+
{
50+
Assert.Empty(results);
51+
}
52+
else
53+
{
54+
Assert.Empty(results.Single().Diagnostics!);
55+
}
5056

5157
// Verify document pull diagnostics are unaffected by running code analysis.
5258
await testLspServer.RunCodeAnalysisAsync(document.Project.Id);
5359
results = await RunGetDocumentPullDiagnosticsAsync(testLspServer, document.GetURI(), useVSDiagnostics);
54-
Assert.Empty(results);
60+
if (useVSDiagnostics)
61+
{
62+
Assert.Empty(results);
63+
}
64+
else
65+
{
66+
Assert.Empty(results.Single().Diagnostics!);
67+
}
5568
}
5669

5770
[Theory, CombinatorialData]

0 commit comments

Comments
 (0)