Language server's initial response to "initialize" request does not correctly report server capabilities

[tintoy]

Hi.

As noted here the language server is not correctly reporting its capabilities when responding to the initialize request (although if the client supports dynamic registration then the subsequent capability-registration message will correctly report its capabilities).

I believe I've tracked this down to the use of LanguageServer.HasHandler<T>, which is never going to behave as expected because the interface captured in HandlerCollection is going to be IRequestHandler<HoverParams, Hover> not IHoverProvider:

csharp-language-server-protocol/src/Lsp/LanguageServer.cs

Line 130 in 4bc2a15

HoverProvider = HasHandler<IHoverHandler>(),

I believe this could be fixed by either capturing the original interface (IHoverHandler) when adding the handler to the collection, finding the underlying IRequestHandler<HoverParams, Hover> interface and matching on that instead, or simply matching on JSON-RPC method name (personally, this seems like the most robust and easy-to-reason-about option).

For example:

bool HasHandler<T>()
    where T : IJsonRpcHandler
{
    var method = LspHelper.GetMethodName<T>();

    return _collection.Get(method).Any();
}

I'm happy to open a PR if you don't have time to fix this, but if so let me know which solution you'd prefer.

[david-driscoll]

Sorry didn't see this the other day. I've changed it to _collection.Any(z => z.Handler is T); with the next release (shortly) which hopefully will solve the problem. If that doesn't then we can move to use the method names.

[tintoy]

No worries - thanks for having a look at it!

BTW, might it be worth considering a match on JSON-RPC method name? Because (although this is admittedly an edge case) there's nothing to indicate to an API consumer that creating their own handler interface with the correct [Method] attribute will correctly handle the messages but won't result in a correct capability declaration.

[david-driscoll]

That's a fair point. My main goal was to just have things be automatic, and I (generally) dislike strings in C# because they're easy to mess up or feel kludgy to get right.

We'd have to create a const string field for each item we wanted to add, and then use that anywhere the text is used. That's why I was using the interface to basically remove needing the string, it's only marked one place, implement the interface and you're good to go.

That said you make a pull request to change it around I'm willing to bring it in.

[david-driscoll]

Release should show up on nuget shortly as version 0.3.0.

[tintoy]

Yeah, I totally get you - stringly-typed stuff makes me uncomfortable, too :)

If this works for now, I personally think it would be enough to simply document the limitation so people understand what they're getting into if they try it; I'd be happy to sketch out a "development guide" in the form of a simple markdown document that describes the basic approach and common pitfalls / gotchas.

[david-driscoll]

Oh one other note on the topic. My goal is try and make something that adapts dynamically to the host client, instead of enforcing the adaptation onto server implementors.

IE, if eclipse che only supports LSP 2.0, and vscode supports LSP 13.3 ( 🤣 ), that as long as you reference this library with a version that knows what LSP 13.3 is, that you can support all the versions you want.

I don't foresee any drastic changes to LSP, besides the dynamic registration ( ✔️ ), and multiple workspaces ( ❌ ). That said I better knock on wood and shut up, lol.

[tintoy]

Defo like that concept - I'm a big believer (especially for protocol libraries) of being strict in what you send, but relaxed / adaptive / forgiving in what you're willing to receive.

[tintoy]

Hmm - just tried the new package, and the call to LanguageClient.AddHandler now hangs. If I roll back to the old package it works fine 😕

Not certain whether this change is the reason though - I was on 0.1.3, so 0.3.0 probably has a lot of other changes no?

[tintoy]

Simple repro here:

https://github.com/tintoy/dotnet-language-client/blob/a8b04586a8433fd59d14dbd7cca5ffb96070bf11/samples/SingleProcess/Program.cs#L170

Only happens with 0.3.0, works fine with 0.1.3. I'll try the version before 0.3.0 to see if that works.

[tintoy]

Interesting - same thing happens in 0.2.1. Looks like it's getting stuck in an infinite loop somewhere inside AddHandler.

[david-driscoll]

Possibly inside https://github.com/OmniSharp/csharp-language-server-protocol/blob/master/src/Lsp/HandlerCollection.cs somewhere.

[tintoy]

I suspect you're right. Having trouble debugging (can't get symbols so VS keeps blindly stepping over the problematic function even when I'm in the disassembly view!), but I'll keep at it.

[tintoy]

Do you currently build symbol packages BTW?

[david-driscoll]

they build but I don't publish them anywhere yet, I need to wireup symbol source.

[david-driscoll]

@tintoy I'll see if I can't test with your extension tonight, but let me know if you've made progress.

[tintoy]

Hi - will give it another try shortly (it's 8am here). VS debugger is still a little broken for .net core it seems :/

[tintoy]

Will try to build packages locally and use them from there...

[tintoy]

(with symbols in the package)

[david-driscoll]

I will look at symbol source as well... and sorry totally forgot where the timezone sits for that side of the world. I should be used to the timezone dance.

[tintoy]

Heh, no problem - I normally keep odd hours :)

So of course it doesn't hang when I use a package built from source (current tip of master) ☹️

BUT: It also doesn't work - HandlerDescriptor.HandlerType is IRequestHandler<T,Response>, whereas the type being checked in HasHandler<T> is IHoverHandler. I tried just comparing the method names, though, and that works fine.

[tintoy]

It doesn't even happen if I check out 561a96d, which is the commit on which 0.3.0 is based! Seems to only happen with the published packages from NuGet. That's... bizzarre.

[tintoy]

Actually, hang on. Let me check something.

[tintoy]

Found the problem :-)

https://github.com/OmniSharp/csharp-language-server-protocol/blob/v0.3.0/src/Lsp/LanguageServer.cs#L58-L61

That overload should probably go?

[tintoy]

(I missed it before because your repo is not the origin remote for my repo and the checkout for that commit didn't work at first because I was only fetching from my origin)

[david-driscoll]

Wow... I totally missed that because I side step and use the enumerable one! It needs to be changed to call AddHandlers instead of AddHandler. I was trying to avoid breaking the existing methods.

[tintoy]

Thanks, @david-driscoll! Fixed in v0.4.1.