openlawlibrary / pygls

A pythonic generic language server
https://pygls.readthedocs.io/en/latest/
Apache License 2.0
588 stars 105 forks source link

`exclude_none = True` breaks `shutdown` requests #245

Closed alcarney closed 2 years ago

alcarney commented 2 years ago

Unfortunately, it looks like #236 breaks shutdown requests.

I have a command in Esbonio's VSCode extension that restarts the language server which sends a shutdown request to the server resulting in the following error

Command 'Esbonio: Restart Language Server' resulted in an error 
(The received response has neither a result or an error property.)

With VSCode's devtools open I can see the response received from the server and the result field is in fact missing.

{
    "jsonrpc": "2.0",
    "id": 1
}

Changing exclude_none back to exclude_unset fixes this issue.

I'm not entirely sure what to suggest as a fix as while I didn't follow the discussion around #236 that closely I'm aware it was to resolve some other issues.


Edit: Actually it looks like this is an issue for any request where "result": null is a valid response which includes completions, goto definition and others

dimbleby commented 2 years ago

Aha. I agree, exclude_none was an overshoot, because there are times when you do explicitly want to set a value to None.

In that case I think that we're going to want to come full circle and

(where that latter bit of code says: if a field has a default that is not None, then act as if we set it explicitly. This has the effect that not-None defaults are serialized even though exclude_unset=True is set)

@alcarney perhaps you'd like to submit an MR along those lines

alcarney commented 2 years ago

Sure I'll take a look :)