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

don't send params for requests/notifications that don't expect them #2240

Merged
merged 2 commits into from
Apr 24, 2023

Conversation

rchl
Copy link
Member

@rchl rchl commented Apr 23, 2023

Rome returns an error response if we include params: null in the shutdown request:

:: [21:44:43.399] --> LSP-rome shutdown (6): None
:: [21:44:50.131] <~~ LSP-rome (6) (duration: 6732ms): {'message': 'Unexpected params: null', 'code': -32602}

Spec says that the shutdown request and the exit notification have params: void which I guess means that the property should not be set. params is an optional property in the spec so this is valid per spec:

interface RequestMessage extends Message {

	/**
	 * The request id.
	 */
	id: integer | string;

	/**
	 * The method to be invoked.
	 */
	method: string;

	/**
	 * The method's params.
	 */
	params?: array | object;
}

In the spec there are some server-initiated requests like workspace/codeLens/refresh that have params: none specified. I wonder if that means that server is expected to set null for those or it's fine to also not set the property at all. But that's not strictly related to our issue.

@jwortmann
Copy link
Member

have params: void which I guess means that the property should not be set

Not sure about that; there are also many other occurrences in the specs where

  • result: void

is used for a response description. And in this case it means that "result" must be set to null in the JSON (because it is a required field for succesful responses). But I could imagine that your assumption for this particular case (shutdown) is right and it should not be set here, however then we should probably open a bug report in Microsoft's LSP repo and they should fix their inconsistent usage of void in the specs.

@rchl
Copy link
Member Author

rchl commented Apr 23, 2023

There is this schema generated from the LSP spec that seems to confirm that those should not have params because it's not specified for those:

The client/registerCapability request, for example, has result: void in the spec but in the type has it specified:

https://github.com/sublimelsp/lsp-python-types/blob/42c0e5b603f36e2f47c324825bd8e21e8f3671ae/lsprotocol/lsp.json#L978-L990

So yeah, it looks like this change is correct and the textual spec is just not very precise and consistent.

@rchl
Copy link
Member Author

rchl commented Apr 23, 2023

Copy link
Member

@rwols rwols left a comment

Choose a reason for hiding this comment

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

I think the ones in the shutdown request and exit notification aren’t supposed to be there. The ones in the diagnostic-related requests, I don’t know, and may need more clarification in the spec.

@rchl
Copy link
Member Author

rchl commented Apr 24, 2023

It was clarified in microsoft/language-server-protocol#1725 (comment)

Not sure which diagnostic-related requests do you mean though. The one I've mentioned was for example the server-initiated workspace/codeLens/refresh. That one also specifies params: none which seems correct.

@rchl rchl changed the title don't include params in requests/notifications when None don't send params for requests/notifications that don't expect them Apr 24, 2023
@rchl rchl merged commit f13b8ee into main Apr 24, 2023
@rchl rchl deleted the fix/shutdown-params branch April 24, 2023 21:50
@rchl
Copy link
Member Author

rchl commented Apr 24, 2023

I guess this could be problematic for requests/notifications that we initiate and that are allowed to take a null value. In that case we would not send params at all which stricter servers might reject. But as far as I can see, there are no requests/notifications like that currently and if there would be any in the future, we would have to validate the behavior against the schema as we wouldn't know otherwise what to do.

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