Workspace diagnostic LSP request when modifying a TypeScript file in VS

[MariaSolOs]

TypeScript enabled LSP pull diagnostics and code actions last week, and since then we discovered the following behavior: When a TypeScript file is modified, the server will receive 2 pull diagnostic requests. One of them comes from the document change in the buffer (which makes sense), but the other one comes from a workspace/diagnostic/refresh request from AlwaysActiveInProcLanguageClient.

The latter request is sent because of the following code:

roslyn/src/Features/LanguageServer/Protocol/Handler/AbstractRefreshQueue.cs

Lines 101 to 118 in c145155

	private ValueTask FilterLspTrackedDocumentsAsync(
	LspWorkspaceManager lspWorkspaceManager,
	IClientLanguageServerManager notificationManager,
	ImmutableSegmentedList<Uri?> documentUris,
	CancellationToken cancellationToken)
	{
	var trackedDocuments = lspWorkspaceManager.GetTrackedLspText();
	foreach (var documentUri in documentUris)
	{
	if (documentUri is null || !trackedDocuments.ContainsKey(documentUri))
	{
	return notificationManager.SendRequestAsync(GetWorkspaceRefreshName(), cancellationToken);
	}
	}
	// LSP is already tracking all changed documents so we don't need to send a refresh request.
	return ValueTaskFactory.CompletedTask;
	}

With a TypeScript file, documentUri is not null and it's not a tracked document, hence the refresh.

Shouldn't the above logic filter out documents which aren't relevant to Roslyn?

[MariaSolOs]

cc @akhera99 @dibarbet @CyrusNajmabadi

[dibarbet]

@MariaSolOs I think from the server perspective this is relatively expected behavior? The AlwaysActiveInProcLanguageClient will only have C# files open (it isn't registered for TS files) - so a change to a TS file would be seen as a change to a 'closed' file, hence the refresh.

However - I wouldn't expect a refresh request from the AlwaysActiveInProcLanguageClient to trigger a request for diagnostics to your server? Is that happening?

Potentially we could be smarter here and say that TS files cannot affect C# diagnostics, so we should ignore it, but if the refresh is triggering a request to a different server, that also seems like a client side bug.

[MariaSolOs]

However - I wouldn't expect a refresh request from the AlwaysActiveInProcLanguageClient to trigger a request for diagnostics to your server? Is that happening?

Yes. The editor will clear the diagnostic cache and request diagnostics from all providers when the refresh is requested.

@dibarbet I think that the issue here is that Roslyn expects the refresh to only affect the server who requested, but the LSP says that "this event is global and will force the client to refresh all pulled diagnostics currently shown". One could argue that this requires clarification, but I still think Roslyn needs to be more careful here.

[MariaSolOs]

Yes. The editor will clear the diagnostic cache and request diagnostics from all providers when the refresh is requested.

Moreover, Visual Studio seems to match VSCode's behavior:
https://github.com/microsoft/vscode-languageserver-node/blob/8e625564b531da607859b8cb982abb7cdb2fbe2e/client/src/common/diagnostic.ts#L1122-L1128

[dibarbet]

I think we should get clarification here on if the current behavior is appropriate. IMHO it doesn't make sense for a refresh request to refresh diagnostics from an entirely unrelated server. Seems like a fairly bad side effect if refreshing C# diagnostics also causes typescript diagnostics to refresh.

Even if we fix this particular issue, anytime there is a closed file or project change in C#, we will trigger TS to refresh (and vice versa - you will trigger C# to refresh when TS-only properties change).