textDocument/rename mixes "user entered invalid name" and "client violates protocol" errors

[matklad]

In rust-analyzer, if you try to rename a thing using invalid name, you get the following result:

The first message makes sense -- the user typed a bad name.

The second message doesn't make sense -- there's no reason to look at the server's log, that's just confusing and wrong.

What happens here is that the requested name, '92 isn't a valid identifier, so we need to return some error. The protocol currently specifies that we need to return an JSON RPC level error, for which we need to pick an error type. We pick InvalidParams, as there doesn't seem to be a better choice.

But I think InvalidParams usually means something different -- that the server and the client disagree about protocol (ie, use different type definitions/serialization formats). So the client, correctly, asks the user to look into the error log for the server.

That is, I think the issue here is that we can't (or at least I don't know how to do it) distinguish between user errors and protocol implementation errors. A plausible solution here is to introduce a dedicated error code for "user provided invalid input".

I got curious what's TypeScript take here. Turns out, you can just rename something to 111!!! lol :-)

[dbaeumer]

To mitigate this I don't show the LSP notification if the rename fails since VS Code itself will already show a notification of a rejected promise.

I do agree that we should define an error code as well to better support this case.

[matklad]

To mitigate this I don't show the LSP notification if the rename fails

Could you expand on this? In the above screenshots, I see both notifications -- one LSP level, and one VS Code level. Haven't traced the origin of those -- maybe it's some kind of our middleware that showes the redundant error?

[dbaeumer]

The LSP client shows the A request has failed error and it is easy for me to suppress that notification in the rename case.

[dbaeumer]

I would propose a generic RequestFailed like this

	/**
	 * A request failed but it was syntactically correct, e.g the
	 * method name was known and the parameters were valid. The error
	 * message should contain human readable information about why
	 * the request failed.
	 */
	export const RequestFailed: integer = -32803

[dbaeumer]

Comment welcome.

[dbaeumer]

A RequestFailed error code got added to the 3.17 version of the spec.