search

dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.85k stars 4.01k forks source link

Workspace diagnostic LSP request when modifying a TypeScript file in VS #73222

Open MariaSolOs opened 4 months ago

MariaSolOs commented 4 months ago

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: https://github.com/dotnet/roslyn/blob/c14515520d206217c509fb05fa667481aa333d9d/src/Features/LanguageServer/Protocol/Handler/AbstractRefreshQueue.cs#L101-L118

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 commented 4 months ago

cc @akhera99 @dibarbet @CyrusNajmabadi

dibarbet commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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).