LSP progress reporting support

[estrizhok]

I have read the corresponding page explaining how progress reporting work. Among other things it says:

The progress argument may have any name or position.

and

Its value will be determined by the IJsonRpcMessageFormatter and may be any valid JSON token

After reading this, I was under the impression that it's a matter of configuration, and one can set it up to work with LSP names for those tokens, namely workDoneToken and partialResultToken.

After not finding anything of the sort in the API, I looked into the code and the tests, and it seems that current implementations, which are all derived from FormatterBase, have the name of the token hard-coded to token, type to long and it has to be a positional argument, too.

Implementing a whole IJsonRpcMessageFormatter just to get those details how LSP needs them to be seems daunting. Am I missing something?

[AArnott]

You're mixing up the client vs. server-side docs. The parts of the doc you quote from is from the client section. The client (an LSP client in your case) can make any argument a special progress argument, without regard to position (in the case of positional arguments in the JSON-RPC request) or name (in the case of named arguments). Although it is expected to match whatever the signature of the method on the server specifies.

Its value will be determined by the IJsonRpcMessageFormatter and may be any valid JSON token

I'll grant you this is poorly worded, since it assumes IJsonRpcMessageFormatter is on the client-side, but the client may be a non-StreamJsonRpc implementation of JSON-RPC and therefore this interface does not apply.

But this is intended to mean that the client gets to make up the value of the token that the server will use to provide progress updates.

StreamJsonRpc already works with LSP as-is. No need to implement IJsonRpcMessageFormatter in a custom way.
Are you writing an LSP server or client?

If you're designing the method to be invoked, and using StreamJsonRpc to implement it, then just add an IProgress<T> parameter to the method. Or if you're using the special LSP-supporting attribute that stuffs all arguments into an object passed as the only argument to a method, add an IProgress<T> property to the request object.

If you're writing the client code that calls it, with StreamJsonRpc, just invoke the method, passing in an IProgress<T> instance for the argument where the server is expecting it.

[estrizhok]

I am writing an LSP server.

I tried to write a test verifying that a IProgress<T> is correctly injected when a message's payload has the workDoneToken or the partialResultToken property. And it did not work.

Here is the test for workDoneToken (incomplete, since it did not work, but enough to make the point):

public class ProgressInjectionTests(ITestOutputHelper testOutputHelper)
{
    [Fact]
    public async Task Should_inject_progress_from_WorkDoneToken_property()
    {
        var (serverStream, clientStream) = FullDuplexStream.CreatePair();
        var clientMessageHandler = MessageHandlerFactory.CreateMessageHandler(clientStream, clientStream);
        var serverMessageHandler = MessageHandlerFactory.CreateMessageHandler(serverStream, serverStream);

        using (var client = new JsonRpc(clientMessageHandler))
        {
            client.AllowModificationWhileListening = true;
            client.TraceSource.Switch.Level = SourceLevels.All;
            client.TraceSource.Listeners.Add(new TestTraceListener("client", testOutputHelper));
            client.StartListening();

            using (var server = new JsonRpc(serverMessageHandler))
            {
                server.AllowModificationWhileListening = true;
                server.TraceSource.Switch.Level = SourceLevels.All;
                server.TraceSource.Listeners.Add(new TestTraceListener("server", testOutputHelper));
                server.StartListening();

                var handler = (Data d, IProgress<WorkDoneProgress> pi) =>
                {
                    pi.Report(WorkDoneProgress.Begin());
                    pi.Report(WorkDoneProgress.Report());
                    pi.Report(WorkDoneProgress.End());

                    return 0;
                };

                server.AddLocalRpcMethod(handler.Method, handler.Target, new JsonRpcMethodAttribute("method")
                {
                    ClientRequiresNamedArguments = true,
                    UseSingleObjectParameterDeserialization = true
                });
                
                await client.InvokeWithParameterObjectAsync<int>("method", new Data(Guid.NewGuid().ToString()));
            }
        }
    }
}

public class Data
{
    public string? Prop { get; }
    public string WorkDoneToken { get; set; } = Guid.NewGuid().ToString();

    public Data(string? prop)
    {
        Prop = prop;
    }
}

Here is the log it generated:

[17:04:26.817] [server] JsonRpc Information: 0 : Added local RPC method "method" -> StreamJsonRpc.Tests.ProgressInjectionTests+<>c.<Should_inject_progress_from_WorkDoneToken_property>b__2_0(Data, IProgress`1) (StreamJsonRpc.Tests.ProgressInjectionTests+<>c)
[17:04:26.821] [client] JsonRpc Information: 3 : Listening started.
[17:04:26.821] [server] JsonRpc Information: 3 : Listening started.
[17:04:26.914] [client] JsonRpc Information: 8 : {"id":2,"method":"method"}
[17:04:26.929] [client] JsonRpc Verbose: 8 : Sent: {"jsonrpc":"2.0","id":2,"method":"method","params":{"prop":"f4f13214-f43f-4f0c-9ea7-c7c25c7ffc31","workDoneToken":"fecfef18-c3e8-4ce0-8da9-12b1309ed5f1"}}
[17:04:26.947] [server] JsonRpc Information: 7 : {"id":2,"method":"method"}
[17:04:26.947] [server] JsonRpc Verbose: 7 : Received: {"jsonrpc":"2.0","id":2,"method":"method","params":{"prop":"f4f13214-f43f-4f0c-9ea7-c7c25c7ffc31","workDoneToken":"fecfef18-c3e8-4ce0-8da9-12b1309ed5f1"}}
[17:04:26.949] [server] JsonRpc Information: 6 : Received request "2" for method "method".
[17:04:26.957] [server] JsonRpc Warning: 5 : Invocation of "method" cannot occur because arguments do not match any registered target methods.
[17:04:26.963] [server] JsonRpc Information: 8 : {"id":2,"error":{"code":-32602,"message":"Unable to find method 'method/2' on {no object} for the following reasons: An argument was not supplied for a required parameter."}}
[17:04:26.964] [server] JsonRpc Verbose: 8 : Sent: {"jsonrpc":"2.0","id":2,"error":{"code":-32602,"message":"Unable to find method \u0027method/2\u0027 on {no object} for the following reasons: An argument was not supplied for a required parameter."}}
[17:04:26.967] [client] JsonRpc Information: 7 : {"id":2,"error":{"code":-32602,"message":"Unable to find method 'method/2' on {no object} for the following reasons: An argument was not supplied for a required parameter."}}
[17:04:26.967] [client] JsonRpc Verbose: 7 : Received: {"jsonrpc":"2.0","id":2,"error":{"code":-32602,"message":"Unable to find method \u0027method/2\u0027 on {no object} for the following reasons: An argument was not supplied for a required parameter."}}
[17:04:26.968] [client] JsonRpc Warning: 12 : Received error response for request 2: InvalidParams "Unable to find method 'method/2' on {no object} for the following reasons: An argument was not supplied for a required parameter.": 
[17:04:26.973] [server] JsonRpc Information: 13 : Connection closing (LocallyDisposed: Stream has been disposed). 
[17:04:26.975] [client] JsonRpc Information: 13 : Connection closing (LocallyDisposed: Stream has been disposed). 

Although the message did contain the workDoneToken property exactly as a request from a real LSP client might, the incoming request was not bound to a method that was expecting IProgress parameter.

I also looked through the code of StreamJsonRpc and did not see why it would work.

EDIT: clientMessageHandler and serverMessageHandler and both use SystemTextJsonFormatter.

[AArnott]

You appear to be mixing things up with your UseSingleObjectParameterDeserialization = true setting. This works when the server method takes exactly one parameter. Yours takes two: (Data d, IProgress<WorkDoneProgress> pi).

Your client is sending a message that expects the server to define a method that takes two parameters named Prop and WorkDoneToken. Because your server method sets UseSingleObjectParameterDeserialization = true, the server will apply the typical LSP-deviation from the JSON-RPC spec and consider those named arguments to instead be properties on an object that is passed to the only parameter on the server method. But you have two parameters on it. The pi parameter does not belong.

Once you remove the pi parameter, I expect it will work fine. Although at that point your client and server are both working with just a WorkDoneToken string value and no magical IProgress<T> property. If you change that property to be typed as IProgress<T>, it should work great. Or if you want to mock up the client side so that it has no idea about IProgress<T> and yet the server will apply progress behavior, you'll need two distinct Data classes: one on the client that provides a string for that property, and one for the server that declares the property as an IProgress<T>.

[estrizhok]

Your client is sending a message that expects the server to define a method that takes two parameters named Prop and WorkDoneToken.

Not really. The client sends a message containing a single argument (as all LSP messages are; correct me if I am wrong). This single argument has WorkDoneToken mixed-in. And I expected the server to extract that token and inject it as a separate argument of IProgress<Something> based on registered method's signature, because

1. Nowhere on the page it says it's possible to do that:

one for the server that declares the property as an IProgress

2. It is kind of silly to have a property WorkDoneToken of type IProgress<WorkDoneProgress> on your data. I know, you can have the property named differently on the server side, but still - I wanted data to be separate from control constructs, and everything I read in the documentation suggested that's how it was implemented in StreamJsonRpc.

Anyway, declaring the IProgress<WorkDoneProgress> WorkDoneToken property. I guess that's what I will do with. Thank you!

BTW, StreamJsonRpc spits warnings about the $/progress method not being registered when it is the one receiving them.

[AArnott]

The client sends a message containing a single argument (as all LSP messages are; correct me if I am wrong).

It depends on whether you interpret this as a JSON-RPC request, or by the different LSP rules. This is what you're sending:

"params":{"prop":"f4f13214-f43f-4f0c-9ea7-c7c25c7ffc31","workDoneToken":"fecfef18-c3e8-4ce0-8da9-12b1309ed5f1"}

Per the JSON-RPC spec, that is to be interpreted as two named arguments. LSP however is designed and versioned as if it's one object to be passed as a single argument to the server method. LSP != JSON-RPC, even though they look extremely similar.

Click here for a tangent about how LSP is a bifurcation of JSON-RPC

As LSP is versioned, the spec adds *arguments* to requests as if they were properties on an object and therefore safe to ignore on the server that doesn't yet support them. But a proper JSON-RPC interpretation would see that adding a named argument to a request is a protocol breaking change because the server would have to add another method overload or a parameter to an existing method to match.

(aside, what LSP should have done was define all their requests with this change applied:

-"params":{"prop":"f4f13214-f43f-4f0c-9ea7-c7c25c7ffc31","workDoneToken":"fecfef18-c3e8-4ce0-8da9-12b1309ed5f1"}
+"params":[{"prop":"f4f13214-f43f-4f0c-9ea7-c7c25c7ffc31","workDoneToken":"fecfef18-c3e8-4ce0-8da9-12b1309ed5f1"}]

It's subtle, but it would allow LSP to version as it wishes, but build into the wire format the assumption that the server takes a single argument that is a JSON object.

StreamJsonRpc is first and foremost a JSON-RPC implementation. But because LSP is such a close variant, we include a couple of settings (via attribute or runtime options) to make it emulate LSP behavior so it can be used for LSP scenarios.

I expected the server to extract that token and inject it as a separate argument of IProgress based on registered method's signature

LSP is already bending the rules of JSON-RPC by specifying one object as if its properties were JSON-RPC method arguments. StreamJsonRpc lets you play in that world or the proper JSON-RPC world. But when you mix those two worlds by having both a params object-consuming parameter and another parameter on the same method .... that isn't a defined behavior anywhere.

It is kind of silly to have a property WorkDoneToken of type IProgress<WorkDoneProgress> on your data.

This is an artifact from LSP being designed by a javascript programmer with javascript primarily in mind. In fact so is the LSP-bifurcation of JSON-RPC, since it's impossible for javascript to implement a JSON-RPC server with named arguments support (at least not without specialized code being written for every single server method). In javascript they can't do magic for progress parameter types like we can do on .NET, so all they expected the server to receive is the token. You can receive just the token too by typing for property/parameter as string and then you can do things the same way as javascript. But if you want the .NET/StreamJsonRpc syntax sugar of dealing with IProgress<T>, change the type to match and deal with the poorly named JSON property.

StreamJsonRpc spits warnings about the $/progress method not being registered when it is the one receiving them.

Hmmm... that's unexpected. Looking at the logs in our tests, I do see this now that you mention it:

Client Warning: 5 : No target methods are registered that match "$/progress".

Sounds like a logging bug. I'll file one. Thank you for pointing that out.