bug#66144: 29.1; eglot-shutdown request params violate JSONRPC spec

bug#66144: 29.1; eglot-shutdown request params violate JSONRPC spec

[Aaron Zeng]

When using Eglot with ocamllsp (or other language servers built with the
same OCaml LSP server library), M-x eglot-shutdown sends an invalid
JSONRPC request to the language server and causes it to exit with an
error message instead of gracefully.  As an additional side effect of
the error, eglot will restart/reconnect to the language server instead
of staying disconnected.

1. emacs -Q
2. C-x C-f ~/temp.ml
3. M-: (setq major-mode 'tuareg-mode)
4. M-x eglot
5. C-u M-x eglot-shutdown

This produces the following *Messages*:

    [eglot] Connected! Server `ocamllsp' now managing `(tuareg-mode caml-mode reason-mode)' buffers in project `azeng'.
    [eglot] Asking EGLOT (azeng/(tuareg-mode caml-mode reason-mode)) politely to terminate
    [jsonrpc] Server exited with status 1
    jsonrpc-error: "request id=5 failed:", (jsonrpc-error-code . -1), (jsonrpc-error-message . "Server died"), (jsonrpc-error-data)

And from the events buffer:

    [client-request] (id:2) Thu Sep 21 16:48:11 2023:
    (:jsonrpc "2.0" :id 2 :method "shutdown" :params nil)
    [stderr] (monitor.ml.Error
    [stderr]  ("Jsonrpc: json conversion failed: invalid structured value")
    [stderr]  ("Raised at Lsp_json_rpc_types__Import.Json.error in file \"import.ml\" (inlined), line 31, characters 23-50"
    [stderr]   "Called from Lsp_json_rpc_types__Jsonrpc.Structured.t_of_yojson in file \"jsonrpc.ml\" (inlined), line 59, characters 14-56"
    [stderr]   "Called from Lsp_json_rpc_types__Import.Option.map in file \"import.ml\" (inlined), line 7, characters 21-26"
    [stderr]   "Called from Lsp_json_rpc_types__Import.Json.field in file \"import.ml\" (inlined), line 46, characters 31-79"
    [stderr]   "Called from Lsp_json_rpc_types__Jsonrpc.Packet.t_of_fields in file \"jsonrpc.ml\", line 298, characters 21-77"
    [stderr]   "Called from Lsp__Io.Make.read in file \"io.ml\" (inlined), line 108, characters 24-57"
    [stderr]   "Called from Fiber__Core.O.(>>=).(fun) in file \"core.ml\", line 251, characters 34-39"
    [stderr]   "Called from Fiber__Scheduler.exec in file \"scheduler.ml\", line 76, characters 8-11"
    [stderr]   "Re-raised at Stdune__Exn.raise_with_backtrace.(partial) in file \"exn.ml\" (inlined), line 38, characters 27-56"
    [stderr]   "Called from Stdune__Exn_with_backtrace.reraise in file \"exn_with_backtrace.ml\" (inlined), line 20, characters 33-71"
    [stderr]   "Called from Fiber__Scheduler.exec in file \"scheduler.ml\", line 81, characters 30-60"
    [stderr]   "Called from Fiber__Scheduler.advance in file \"scheduler.ml\" (inlined), line 194, characters 2-58"
    [stderr]   "Called from Fiber_async.deferred_of_fiber.loop.(fun) in file \"fiber_async.ml\" (inlined), line 44, characters 11-53"))
    [internal] Thu Sep 21 16:48:11 2023:
    (:message "Connection state changed" :change "exited abnormally with code 1\n")
    
    ----------b---y---e---b---y---e----------

It seems that jsonrpc.el is sending a "null" value for the params field
of a request, when it should instead be omitting the field or sending an
empty JSON object instead:
https://www.jsonrpc.org/specification#parameter_structures

Users of other editors have reported similar issues, and concluded that
this was correct (if pedantic) behavior by ocamllsp and should be fixed
in the editor: https://github.com/helix-editor/helix/issues/5400


In GNU Emacs 29.1 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2023-09-21 built on
 igm-qws-u12685a
Repository revision: a9b28224af0f73d1fe0f422e9b318c5b91af889b
Repository branch: HEAD
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Rocky Linux 8.8 (Green Obsidian)

Configured using:
 'configure --with-x-toolkit=lucid --without-gpm --without-gconf
 --without-selinux --without-imagemagick --with-modules --with-gif=no
 --with-cairo --with-rsvg'

Configured features:
CAIRO DBUS FREETYPE GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND
SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM XINPUT2 XPM LUCID
ZLIB

Important settings:
  value of $LANG: en_US.utf8
  locale-coding-system: utf-8-unix

Major mode: Fundamental

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  blink-cursor-mode: t
  buffer-read-only: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(tabify cus-edit cus-start cus-load wid-edit cl-print mailalias rmail
shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec epa derived epg rfc6068 epg-config
gnus-util mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev
gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils time-date cl-extra eglot external-completion array
filenotify jsonrpc ert pp ewoc debug backtrace find-func xref
flymake-proc flymake warnings icons compile text-property-search comint
ansi-osc ansi-color ring pcase url-util url-parse auth-source cl-seq
eieio eieio-core cl-macs password-cache json subr-x map url-vars project
byte-opt gv bytecomp byte-compile imenu thingatpt help-fns radix-tree
help-mode cl-loaddefs cl-lib rmc iso-transl tooltip cconv eldoc paren
electric uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify
dynamic-setting system-font-setting font-render-setting cairo x-toolkit
xinput2 x multi-tty make-network-process emacs)

Memory information:
((conses 16 114174 16392)
 (symbols 48 11887 5)
 (strings 32 36275 1576)
 (string-bytes 1 1007705)
 (vectors 16 22858)
 (vector-slots 8 309640 13918)
 (floats 8 79 204)
 (intervals 56 351 0)
 (buffers 976 20))

bug#66144: 29.1; eglot-shutdown request params violate JSONRPC

[Javier Olaechea]

[-- Attachment #1: Type: text/plain, Size: 456 bytes --]

It seems that eglot used to send an empty object as the params but it got
replaced by null to appease gopls.

https://github.com/joaotavora/eglot/commit/4f6e152e1c5efc39a888f747ecca313a58f4375e

The fix would be replacing the call
(jsonrpc-request server :shutdown nil :timeout (or timeout 1.5))
with
(jsonrpc-request server :shutdown eglot--{} :timeout (or timeout 1.5))

-- 
"I object to doing things that computers can do." — Olin Shivers

[-- Attachment #2: Type: text/html, Size: 784 bytes --]

bug#66144: 29.1; eglot-shutdown request params violate JSONRPC

[Eli Zaretskii]

> From: Javier Olaechea <pirata@gmail.com>
> Date: Sat, 7 Oct 2023 22:42:47 -0500
> 
> It seems that eglot used to send an empty object as the params but it got replaced by null to appease
> gopls.
> 
> https://github.com/joaotavora/eglot/commit/4f6e152e1c5efc39a888f747ecca313a58f4375e
> 
> The fix would be replacing the call
> (jsonrpc-request server :shutdown nil :timeout (or timeout 1.5))
> with
> (jsonrpc-request server :shutdown eglot--{} :timeout (or timeout 1.5))

João, any comments?

bug#66144: 29.1; eglot-shutdown request params violate JSONRPC

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 669 bytes --]

On Sat, Oct 14, 2023, 09:14 Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Javier Olaechea <pirata@gmail.com>
> > Date: Sat, 7 Oct 2023 22:42:47 -0500
> >
> > It seems that eglot used to send an empty object as the params but it
> got replaced by null to appease
> > gopls.
> >
> >
> https://github.com/joaotavora/eglot/commit/4f6e152e1c5efc39a888f747ecca313a58f4375e
> >
> > The fix would be replacing the call
> > (jsonrpc-request server :shutdown nil :timeout (or timeout 1.5))
> > with
> > (jsonrpc-request server :shutdown eglot--{} :timeout (or timeout 1.5))
>
> João, any comments?
>

Yes. What's the matter? What would the "fix" fix?

>

[-- Attachment #2: Type: text/html, Size: 1429 bytes --]

bug#66144: 29.1; eglot-shutdown request params violate JSONRPC

[Javier Olaechea]

[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]

> Yes. What's the matter? What would the "fix" fix?

That is illegal, as in against the JSON-RPC standard, to send a request
with `params: null.`. Quoting section 4.2 from the JSON RPC spec linked by
OP:

> If present, parameters for the rpc call MUST be provided as a Structured
value. Either by-position through an Array or by-name through an Object.

That means in a request object can not have a param key, but if present it
must be an Array or an Object.

That said, upon further reflection I think the bug is in json-rpc not
eglot.

On Sat, Oct 14, 2023 at 3:59 AM João Távora <joaotavora@gmail.com> wrote:

> On Sat, Oct 14, 2023, 09:14 Eli Zaretskii <eliz@gnu.org> wrote:
>
>> > From: Javier Olaechea <pirata@gmail.com>
>> > Date: Sat, 7 Oct 2023 22:42:47 -0500
>> >
>> > It seems that eglot used to send an empty object as the params but it
>> got replaced by null to appease
>> > gopls.
>> >
>> >
>> https://github.com/joaotavora/eglot/commit/4f6e152e1c5efc39a888f747ecca313a58f4375e
>> >
>> > The fix would be replacing the call
>> > (jsonrpc-request server :shutdown nil :timeout (or timeout 1.5))
>> > with
>> > (jsonrpc-request server :shutdown eglot--{} :timeout (or timeout 1.5))
>>
>> João, any comments?
>>
>
> Yes. What's the matter? What would the "fix" fix?
>
>>

-- 
"I object to doing things that computers can do." — Olin Shivers

[-- Attachment #2: Type: text/html, Size: 2659 bytes --]

bug#66144: 29.1; eglot-shutdown request params violate JSONRPC

[João Távora]

On Sat, Oct 14, 2023 at 5:59 PM Javier Olaechea <pirata@gmail.com> wrote:
>
> > Yes. What's the matter? What would the "fix" fix?
>
> That is illegal,

So is a lot of stuff, like smoking certain plants.  And yet, if it doesn't
hurt anyone...

> That said, upon further reflection I think the bug is in json-rpc not eglot.

Maybe.  Anyway, I think we should close this bug.  If you can find some Eglot
use case that is actually hurt by this, we can reopen.

bug#66144: 29.1; eglot-shutdown request params violate JSONRPC

[Javier Olaechea]

[-- Attachment #1: Type: text/plain, Size: 337 bytes --]

> If you can find some Eglot use case that is actually hurt by this, we can
reopen.

The OP has an Eglot use case that is hurt by this, they cannot shutdown the
ocamllsp server. Because when they call M-x eglot-shutdown, eglot sends an
invalid request. The server dies instead of returning a response and then
eglot restarts the server.

[-- Attachment #2: Type: text/html, Size: 426 bytes --]

bug#66144: 29.1; eglot-shutdown request params violate JSONRPC

[João Távora]

On Sat, Oct 14, 2023 at 6:19 PM Javier Olaechea <pirata@gmail.com> wrote:
>
> > If you can find some Eglot use case that is actually hurt by this, we can reopen.
>
> The OP has an Eglot use case that is hurt by this, they cannot shutdown the ocamllsp server. Because when they call M-x eglot-shutdown, eglot sends an invalid request. The server dies instead of returning a response and then eglot restarts the server.

But what OP?  I don't see this message, can you point to it?

bug#66144: 29.1; eglot-shutdown request params violate JSONRPC

[João Távora]

On Sat, Oct 14, 2023 at 7:02 PM João Távora <joaotavora@gmail.com> wrote:
>
> On Sat, Oct 14, 2023 at 6:19 PM Javier Olaechea <pirata@gmail.com> wrote:
> >
> > > If you can find some Eglot use case that is actually hurt by this, we can reopen.
> >
> > The OP has an Eglot use case that is hurt by this, they cannot shutdown the ocamllsp server. Because when they call M-x eglot-shutdown, eglot sends an invalid request. The server dies instead of returning a response and then eglot restarts the server.
>
> But what OP?  I don't see this message, can you point to it?

OK, I see the message now.  In that case that changes things.
One of the misbehaving servers has to correct itself.  I'd say it
should be gopls.

Unless that has already happened.  Can you test gopls shutdowns
with your patch Javier?

João

bug#66144: 29.1; eglot-shutdown request params violate JSONRPC

[Javier Olaechea]

[-- Attachment #1: Type: text/plain, Size: 1637 bytes --]

> Can you test gopls shutdowns
with your patch Javier?

I have tested locally with gopls v0.11.0 and M-x eglot-shutdown works
successfully afaict. Both w/o and w/ the patch applied.

However I think it might be better to modify jsonrpc-request so that when
the param argument is nil the JSON serialization omits the param key. I'm
thinking of using the `,@(when param (list param)) idiom. jsonrpc has
tests. I'd like to give it a go this weekend to see if I can come up with
an acceptable way to have jsonrpc.el conform with the standard w/o changing
the API. What do you think?

On Sat, Oct 14, 2023 at 1:30 PM João Távora <joaotavora@gmail.com> wrote:

> On Sat, Oct 14, 2023 at 7:02 PM João Távora <joaotavora@gmail.com> wrote:
> >
> > On Sat, Oct 14, 2023 at 6:19 PM Javier Olaechea <pirata@gmail.com>
> wrote:
> > >
> > > > If you can find some Eglot use case that is actually hurt by this,
> we can reopen.
> > >
> > > The OP has an Eglot use case that is hurt by this, they cannot
> shutdown the ocamllsp server. Because when they call M-x eglot-shutdown,
> eglot sends an invalid request. The server dies instead of returning a
> response and then eglot restarts the server.
> >
> > But what OP?  I don't see this message, can you point to it?
>
> OK, I see the message now.  In that case that changes things.
> One of the misbehaving servers has to correct itself.  I'd say it
> should be gopls.
>
> Unless that has already happened.  Can you test gopls shutdowns
> with your patch Javier?
>
> João
>


-- 
"I object to doing things that computers can do." — Olin Shivers

[-- Attachment #2: Type: text/html, Size: 2285 bytes --]

bug#66144: 29.1; eglot-shutdown request params violate JSONRPC

[João Távora]

On Sat, Oct 14, 2023 at 9:14 PM Javier Olaechea <pirata@gmail.com> wrote:
>
> > Can you test gopls shutdowns
> with your patch Javier?
>
> I have tested locally with gopls v0.11.0 and M-x
> eglot-shutdown works successfully afaict. Both w/o and
> w/ the patch applied.

Great, then I'd say we should first apply your patch.

> However I think it might be better to modify jsonrpc-request
> so that when the param argument is nil the JSON serialization
> omits the param key. I'm thinking of using the `,@(when param
> (list param)) idiom. jsonrpc has tests. I'd like to give it
> a go this weekend to see if I can come up with an acceptable
> way to have jsonrpc.el conform with the standard w/o changing
> the API. What do you think?

Maybe, but I don't know if it's worth it.  Especially because we
risk that other servers don't like this new behavior in particular.

It's a very risky change IMO.

Anyway, I would like to confirm some things:

* That the JSONRPC standard actually allows this.  I think you've
  done this already, but please double check.

* That the LSP Base Protocol, which is partially re-described in
  the LSP spec also seems to allow this.

* That the Eglot tests pass after this patch.  And ideally test
  this with some major servers, like rust-analyzer, pylsp,
  pyright, etc.

João

bug#66144: 29.1; eglot-shutdown request params violate JSONRPC

[Javier Olaechea]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

> Great, then I'd say we should first apply your patch.

I've also checked that it fixes the issue with ocamllsp. I agree that we
should apply the change as it fixes the issue that Aaron is facing. I can
work on the jsonrpc regardless and I will take into consideration the
issues that you've mentioned


-- 
"I object to doing things that computers can do." — Olin Shivers

[-- Attachment #2: Type: text/html, Size: 637 bytes --]

bug#66144: 29.1; eglot-shutdown request params violate JSONRPC spec

[Stefan Kangas]

Javier Olaechea <pirata@gmail.com> writes:

>> Great, then I'd say we should first apply your patch.
>
> I've also checked that it fixes the issue with ocamllsp. I agree that we should apply the change
> as it fixes the issue that Aaron is facing. I can work on the jsonrpc regardless and I will take into
> consideration the issues that you've mentioned

Javier, did you make any progress here?

Thanks.

bug#66144: 29.1; eglot-shutdown request params violate JSONRPC spec

[Javier Olaechea]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

Javier, did you make any progress here?
>

I did sit down twice to read the jsonrpc.el code and  understand what I
need to do to make the change I hoped to. But unfortunately I can't even
say I understood jsonrpc--async-request-1 well enough.Or even, if that is
indeed the function we need to change so that we don't seed a params
attribute when params is nil. It seems that we are passing the params down
in multiple arms?

-- 
"I object to doing things that computers can do." — Olin Shivers

[-- Attachment #2: Type: text/html, Size: 875 bytes --]

bug#66144: 29.1; eglot-shutdown request params violate JSONRPC spec

[Stefan Kangas]

Javier Olaechea <pirata@gmail.com> writes:

> I did sit down twice to read the jsonrpc.el code and  understand what I need to do to make the
> change I hoped to. But unfortunately I can't even say I understood jsonrpc--async-request-1 well
> enough.Or even, if that is indeed the function we need to change so that we don't seed a params
> attribute when params is nil. It seems that we are passing the params down in multiple arms?

Daniel, can you help answer Javier's questions in this bug thread?

bug#66144: 29.1; eglot-shutdown request params violate JSONRPC spec

[Daniel Pettersson]

 Javier Olaechea <pirata@gmail.com> writes:

> I did sit down twice to read the jsonrpc.el code and understand what
> I need to do to make the
> change I hoped to. But unfortunately I can't even say I understood
> jsonrpc--async-request-1 well
> enough.Or even, if that is indeed the function we need to change so
> that we don't seed a params

As I see it there are two candidates:

Modify calls to jsonrpc-connection-send (jsonrpc--async-request-1,
jsonrpc-notify, to keep the behavior consistent) such that
jsonrpc-connection-send is not called with keyword :params if params is
nil.

Or make the change in jsonrpc-convert-to-endpoint which makes the
behavior part of jsonrpc.el's API.  We are already doing some
sanitizing here before serialization of a similar character.

I think both options are fine, but I would favor the second one.

> attribute when params is nil. It seems that we are passing the
> params down in multiple arms?

Both solution candidates above should account for this.

This sort of change does come with some extensive regression testing, as
João mentioned.