LSP: Tolerate non-mutating requests where we are unable to determine the language

[RikkiGibson]

Closes #81410

[RikkiGibson]

There might be a more appropriate place for this test to live, happy to move it there if someone wants to point it out.

[RikkiGibson]

This change basically means that we will use the default handler for non-mutating requests where we can't determine a language, which I think generally doesn't give much/any info.

When I stepped through the new test, I found it sent a request to PublicDocumentPullDiagnosticsHandler, which ended up having no sources for diagnostics, so it finally yielded no diagnostics to the client.

While it's possible that if we loaded the document as csharp instead, we would end up getting some meaningful diagnostics, I think our actual answer to this request isn't important, what matters is that we just don't fail, don't show a spurious error popup, and allow things to keep going.

If the client didn't give us the necessary information to see this document as csharp, for example, then it's unlikely that they need csharp-specific info. It's possible that even asking for diagnostics at all in this "closing file" scenario is just a "quirk"/misfire in the client, but, the spec does appear to allow it, so we might as well handle it as gracefully as possible.

IIRC, the previous behavior, where we probably were calling work.FailRequest, not only would show an unwanted error popup, but seemed to bring down the LS anyway. Editor features would entirely stop working even in the "new" .cs document, which was unrelated to the untitled document associated with the failure.

[davidwengier]

Personally, I've long been of the opinion that having our LSP server be really fussy about the shape of requests it will handle makes for a great self-DDOS machine, so I love the fact that predictable actions like this don't bring down the server any more.

Having said that, I think its still appropriate to return from this method, and not attempt to handle the request, if its non-mutating. And probably log something.

ie:

if (!didGetLanguage)
{
	if (handler.MutatesSolutionState)
    {
        throw new ....
    }

    logger.LogWarning("Refusing to handle {method} because we couldn't determine a language for {uri}");
    return;
}

[davidwengier]

To put it another way, I don't think the "default handlers" are currently set up to be generic "try to handle this method about any old file". They're really just "C# handlers". I could totally see an argument to make them more defensive though

[RikkiGibson]

Logging a warning in the non-mutating case seems reasonable here. That way it seems like we will be able to spot this happening in the logs fairly easily, without actually bothering users with something that might not be a problem.

re: not handling the request. I am worried that if we neither give a response nor fail the request, then the client will eventually time out, or could get "stuck" somehow waiting for that response. So it feels reasonable to allow the default language handler to just handle these requests in general, on the assumption that it will usually just give minimal information back quickly and allow the client to move on. ("move along, nothing to see here..")

[davidwengier]

Fair enough on not directly returning, but can we return a completed task? Or perhaps, as you said, its worth checking if FailRequest is working as expected?

My worry is that this simply moves the problem down a level, rather than fixes it. It might work for diagnostics, great, but all it takes is for any other handlers to call context.GetRequiredDocument() and we're back to throwing an exception and bringing down the server. Then again, as I said, perhaps that fine and we just make each handler defensive enough as it comes up. I don't feel super strongly about this aspect, and will immediately fold as soon as David B weighs in :)

[RikkiGibson]

It might be good to use some specialized minimal handler here. Because it does seem we really don't want this to do any meaningful work. Presumably though each request has some schema that the response needs to conform to. If I am understanding this code path right, I do believe that this call needs to take an active step to write JSON to a response stream (like by running a handler) in order for the client to operate properly.

Also curious what @dibarbet thinks here. IIRC his suggestion previously was to basically take requests where we can't determine the language, and essentially just force the language mode to C# and proceed.

I am far from an expert in this area, so it's definitely possible that any assumptions I have about how the current solution will "just work out", are plain wrong.

[RikkiGibson]

If we can just return null and client is always supposed to suspect that as a possibility and carry on gracefully (i.e. not treat it as an error scenario) then great. I looked around a bit and didn't spot a way of doing that, but I may have missed it

[dibarbet]

This is complicated. David is right that other handlers are likely to call GetRequiredDocument and just throw there instead (if its a request for a closed document that we no longer have)

However, I am not sure it is correct to simply return nothing from the request in all cases. The LSP spec does not allow null as a valid response for all requests, for example diagnostics - https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_diagnostic - this was specifically fixed for diagnostics recently for compatibility with another client, so I don't think we should change that (and I prefer the former approach over this)

@RikkiGibson one thing we could experiment with - could we return an exception with a specific error code here (catch it so the queue continues on)? For example, invalid request? I know the LSP client does handle some of the error codes and does not show error popups for them, for example content modified which we use in code lens, but not sure about all of them. Would need to test. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#errorCodes

Alternatively, if none of the built in error codes suppress the toasts, potentially we could return a custom error code that we suppress on the client side here?
https://github.com/dotnet/vscode-csharp/blob/b2132170ffb0d859a0ba24d91e909e53890198f0/src/lsptoolshost/server/roslynLanguageClient.ts#L52

[dibarbet]

Generally, I don't think it is wrong for the server to error in this case, especially when we get asked about an unsaved file that has been closed. We can't analyze just a URI. But we also should not take down the server, and don't need to necessarily display an annoying error since we already know exactly what is happening and why.

[RikkiGibson]

I can see that throwing a LocalRpcException with code LspErrorCodes.ContentModified, will cause the client to not surface the failure in the UI.

I can't tell what in the client is causing the UI to not appear here. If I use a different code like RequestFailed from the spec, then the client does show the toast. Perhaps something from the base vscode LanguageClient. Weird. I thought it might have been related to the logging change in #80549, but I cannot hit a breakpoint there in the case where the toast is popping up, so I don't think so.

I think the most "clean" solution would involve special checking on the client for a new error code for this purpose.

[dibarbet]

Perhaps something from the base vscode LanguageClient

its this, and it looks like there isn't another one we can use from the spec that doesn't show an error (content modified, cancellation are not correct).
https://github.com/microsoft/vscode-languageserver-node/blob/7744dc56aaff88a05deee900b91375791d3bc7d2/client/src/common/client.ts#L2252

I think custom error code is potentially the best option