Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixing missing params and proper end brackets to message registerCapability #299

Closed
wants to merge 1 commit into from

Conversation

vushu
Copy link
Contributor

@vushu vushu commented Jan 18, 2023

I have painstakingly debugged serve-d for quite some time,
since coc didn't work after some major changes, and I have finally discovered why (see in the PR),
It is quite error prone to manually build these messages, so I am guessing we need some tests to ensure the
expected message.

I think why other lsp clients works is because they just ignore invalid messages, whereas coc is way more strict and just shutdowns the server when it receives something invalid.

@vushu vushu changed the title missin params to message missing params to message registerCapability Jan 18, 2023
id.escapeJsonStringContent,
`","method":"`,
method.escapeJsonStringContent,
`","registerOptions":`,
options.serializeJson,
`]}`
Copy link
Contributor

@monkoose monkoose Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me just closing curly bracket is enough }]} to make coc.nvim work. Because it is what is definitely a bug, does params really required?

Copy link
Contributor Author

@vushu vushu Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just compared the result of registerCapability on:

4fb7a55 (last working version with coc)
{"id":"ed99ff5c-eab0-405d-835f-5452bb7394be","jsonrpc":"2.0","method":"client/registerCapability","params":{"registrations":[{"id":"profilegc.watchfiles","method":"workspace/didChangeWatchedFiles","registerOptions":{"watchers":[{"globPattern":"**/profilegc.log"}]}}]}}

and

current master:
{"jsonrpc":"2.0","method":"client/registerCapability","registrations":[{"id":"profilegc.watchfiles","method":"workspace/didChangeWatchedFiles","registerOptions":{"watchers":[{"globPattern":"**/profilegc.log"}]}]}

I will say that it's like the old behavior with params, but not required as you have detected also id doesn't seem to be needed either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the last closing bracket is definitely required here, as done in the PR right now. If it works without it, there is probably a bug in the parsing code on the client, because that would still be invalid JSON.

@vushu vushu changed the title missing params to message registerCapability fixing missing params and proper end brackets to message registerCapability Jan 18, 2023
Copy link
Member

@WebFreak001 WebFreak001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you so much! This method has only been called after startup when capabilities.workspace.didChangeWatchedFiles.dynamicRegistration was on, so it may also be possible that not all clients supported that. VSCode did not help me here because it didn't even log any warning or error.

I think it's also a little bit broken on coc.nvim side that they shutdown the server when an invalid JSON is received (the JSON-RPC spec defines to handle parsing errors you send the parse error error code)

Investigating this code now it looks like it's still missing an id field to be able to receive a response. I would personally rather want to throw out the manual JSON string stuff here and make a struct to send it like anywhere else. Just here I would define the struct in this function.

@WebFreak001
Copy link
Member

I rewrote the code here to be more readable and not fail due to wrong string handling, can you try out this patch and see if it works instead of this PR?

diff --git a/lsp/source/served/lsp/jsonrpc.d b/lsp/source/served/lsp/jsonrpc.d
index 44bbad3..e10bf5d 100644
--- a/lsp/source/served/lsp/jsonrpc.d
+++ b/lsp/source/served/lsp/jsonrpc.d
@@ -224,16 +224,33 @@ class RPCProcessor : Fiber
 
 	void registerCapability(T)(scope const(char)[] id, scope const(char)[] method, T options)
 	{
-		const(char)[][7] parts = [
-			`{"jsonrpc":"2.0","method":"client/registerCapability","registrations":[{"id":"`,
-			id.escapeJsonStringContent,
-			`","method":"`,
-			method.escapeJsonStringContent,
-			`","registerOptions":`,
-			options.serializeJson,
-			`]}`
+		import mir.serde;
+
+		@serdeFallbackStruct
+		struct RegistrationT
+		{
+			const(char)[] id;
+			const(char)[] method;
+			T registerOptions;
+		}
+
+		@serdeFallbackStruct
+		struct RegistrationParamsT
+		{
+			RegistrationT[] registrations;
+		}
+
+		static assert(RegistrationParamsT.tupleof.stringof == RegistrationParams.tupleof.stringof,
+			"Fields of templated `RegistrationParams` differ from regular struct, please verify correct field names in LSP spec!");
+		static assert(RegistrationT.tupleof.stringof == Registration.tupleof.stringof,
+			"Fields of templated `Registration` differ from regular struct, please verify correct field names in LSP spec!");
+
+		scope RegistrationParamsT params;
+		params.registrations = [
+			RegistrationT(id, method, options)
 		];
-		sendRawPacket(parts[]);
+
+		sendMethod("client/registerCapability", params);
 	}
 
 	/// Sends a request with the given `method` name to the other RPC side without any parameters.

if it does, I could either commit it with you co-authoring it or you can just apply it to the PR here and I will merge it then.

This fixes that there was also no id parameter allocated for the request, so it never got a response. (although the response is void, it may throw an exception if the editor responds with an error)

@vushu
Copy link
Contributor Author

vushu commented Jan 18, 2023

@WebFreak001 I can confirm that your implementation works :)
Just commit it with co-authoring

@monkoose
Copy link
Contributor

Works for me too.

@WebFreak001
Copy link
Member

thanks, committed it there, nightly is building

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants