[ELPA] New package: eglot

[ELPA] New package: eglot

[João Távora]

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

Hello,

I'd like to add Eglot, my new package, to GNU ELPA.

Eglot, for Emacs Polyglot, is an Emacs client for the LSP, or Language
Server Protocol (https://microsoft.github.io/language-server-protocol/).

For those unfamilar with LSP, here's a quick primer. M-x eglot connects
to LSP subprocesses locally or through the network. Thereafter, via a
JSON-based RPC protocol, information about the source code is exchanged
with the server: the client tells is about the (unsaved) buffer's
contents, and the server provides information to feed xref.el,
flymake.el, eldoc.el, auto-completion, and other IDE-like functionality.

Because Eglot is language-agnostic, the big idea is that this gives
Emacs an uniform IDE UI for any language one can find a suitable server
for.

There is a prominent existing Emacs package, emacs-lsp, that shares
largely the same goals. I have contacted its author and explained why I
started from scratch. This is summarized in a list of user-visible and
under-the-hood differences between the two projects in Eglot's home
page:

    https://github.com/joaotavora/eglot#differences-to-lsp-modeel

emacs-lsp and its many plugins live at:

    https://github.com/emacs-lsp

One notable difference is that Eglot requires Emacs 26 and integrates
with the new flymake.el redesign, whereas emacs-lsp uses
flycheck.el for that. Another design goal is a single M-x eglot entry
point, instead of different eglot-<language>.el and M-x eglot-<language>
commands for each language.

If the package is accepted, I think I already have commit rights to the
GNU ELPA repo and would like to develop this as a Git subtree, just like
yasnippet.

Thank you for your attention,
João


[-- Attachment #2: Eglot, Emacs Polyglot --]
[-- Type: application/emacs-lisp, Size: 65837 bytes --]

Re: [ELPA] New package: eglot

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 10 May 2018 23:34:40 +0100
> 
> I'd like to add Eglot, my new package, to GNU ELPA.
> 
> Eglot, for Emacs Polyglot, is an Emacs client for the LSP, or Language
> Server Protocol (https://microsoft.github.io/language-server-protocol/).

FWIW, I think LSP support should be part of the core Emacs
distribution, as it's an infrastructure necessary for modern support
of programming modes.  (I didn't yet look at the package, so if it has
separate infrastructure and application levels, perhaps only the
infrastructure layers should be in Emacs.)

Thanks.

Re: [ELPA] New package: eglot

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Date: Thu, 10 May 2018 23:34:40 +0100
>> 
>> I'd like to add Eglot, my new package, to GNU ELPA.
>> 
>> Eglot, for Emacs Polyglot, is an Emacs client for the LSP, or Language
>> Server Protocol (https://microsoft.github.io/language-server-protocol/).
>
> FWIW, I think LSP support should be part of the core Emacs
> distribution, as it's an infrastructure necessary for modern support
> of programming modes.

It'd be great if eglot.el went into core, of course, and I'd welcome the
additional level of scrutiny that usually brings. However, if it does
come to that, and since Emacs 27 is reasonably distant, I'd like to keep
distributing for Emacs 26 and developing outside Emacs or in parallel
with it for at least some time.

> (I didn't yet look at the package, so if it has separate
> infrastructure and application levels, perhaps only the infrastructure
> layers should be in Emacs.)

Depends on where you draw the line, but if you really do want LSP to be
in core, then you need the full eglot.el, which has (almost) no
language-specific code. If you just want JSON-RPC support in Emacs (for
whichever other applications that might use it), then you only need a
part of eglot.el. Regardless of which parts make it into core, a
json-rpc.el could be easily be extracted out of eglot.el if it is deemed
useful.

João

Re: [ELPA] New package: eglot

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Fri, 11 May 2018 12:29:03 +0100
> 
> > FWIW, I think LSP support should be part of the core Emacs
> > distribution, as it's an infrastructure necessary for modern support
> > of programming modes.
> 
> It'd be great if eglot.el went into core, of course, and I'd welcome the
> additional level of scrutiny that usually brings. However, if it does
> come to that, and since Emacs 27 is reasonably distant, I'd like to keep
> distributing for Emacs 26 and developing outside Emacs or in parallel
> with it for at least some time.

New packages could be added in Emacs 26.2, IMO.

Re: [ELPA] New package: eglot

[Stefan Monnier]

> If the package is accepted, I think I already have commit rights to the
> GNU ELPA repo and would like to develop this as a Git subtree, just like
> yasnippet.

Nowadays I recommend ":external"s to ":subtree"s, but it's your call.
I'd welcome this package into GNU ELPA, so feel free to push it there.


        Stefan

Re: [ELPA] New package: eglot

[Bozhidar Batsov]

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

I think that until a package matures it's better for it to live in ELPA, so
it can evolve at its own pace.

I agree that eventually Emacs should probably feature some built-in support
for LSP, though.

On 12 May 2018 at 18:47, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> > If the package is accepted, I think I already have commit rights to the
> > GNU ELPA repo and would like to develop this as a Git subtree, just like
> > yasnippet.
>
> Nowadays I recommend ":external"s to ":subtree"s, but it's your call.
> I'd welcome this package into GNU ELPA, so feel free to push it there.
>
>
>         Stefan
>
>
>

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

Re: [ELPA] New package: eglot

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> If the package is accepted, I think I already have commit rights to the
>> GNU ELPA repo and would like to develop this as a Git subtree, just like
>> yasnippet.
>
> Nowadays I recommend ":external"s to ":subtree"s, but it's your call.
> I'd welcome this package into GNU ELPA, so feel free to push it there.

I just did, with :external, which does indeed sound simpler.  (I did
mess up slightly and pushed an "externals/elpa" branch to the repo
instead of the intended "externals/eglot". I deleted the wrong branch,
so apart from a few extra emails, everything should be fine).

My package builds fine, but "make" for some of the others didn't go so
well.

Anyway, here's hoping to see it built tomorrow,
João

New jrpc.el JSONRPC library (Was: [ELPA] New package: eglot)

[João Távora]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Date: Thu, 10 May 2018 23:34:40 +0100
>> 
>> I'd like to add Eglot, my new package, to GNU ELPA.
>> 
>> Eglot, for Emacs Polyglot, is an Emacs client for the LSP, or Language
>> Server Protocol (https://microsoft.github.io/language-server-protocol/).
>
> FWIW, I think LSP support should be part of the core Emacs
> distribution, as it's an infrastructure necessary for modern support
> of programming modes.  (I didn't yet look at the package, so if it has
> separate infrastructure and application levels, perhaps only the
> infrastructure layers should be in Emacs.)

Hi again, Eli

So, off-list Stefan likewise suggested that some level of LSP
"infrastructure" is integrated into Emacs, which could lead to merging
eglot.el and other LSP clients like lsp-mode.el in the future.

As JSONRPC support is a mandatory component of LSP, I've extracted a
library implementing the JSONRPC spec (see www.jsonrpc.org) out of
eglot.el.

I propose this library, jrpc.el, for Emacs core (or GNU ELPA, or
both). I hope people can review it, comment on it, and propose changes,
particularly developers of other existing LSP-modes.  I attach to the
end of this mail with a reasonably complete ";;; Commentary" header,
including a simple usage example.

JSONRPC is apparently also used for other non-LSP applications. See
http://json-rpc.info/.

There's a jsonrpc-refactor branch of eglot.el using jrpc.el, for those
who want to test it with a real application.

Thanks in advance,
João


[-- Attachment #2: jrpc.el --]
[-- Type: application/emacs-lisp, Size: 29295 bytes --]

Re: New jrpc.el JSONRPC library (Was: [ELPA] New package: eglot)

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca, vibhavp@gmail.com
> Date: Fri, 18 May 2018 17:27:54 +0100
> 
> So, off-list Stefan likewise suggested that some level of LSP
> "infrastructure" is integrated into Emacs, which could lead to merging
> eglot.el and other LSP clients like lsp-mode.el in the future.
> 
> As JSONRPC support is a mandatory component of LSP, I've extracted a
> library implementing the JSONRPC spec (see www.jsonrpc.org) out of
> eglot.el.
> 
> I propose this library, jrpc.el, for Emacs core (or GNU ELPA, or
> both). I hope people can review it, comment on it, and propose changes,
> particularly developers of other existing LSP-modes.  I attach to the
> end of this mail with a reasonably complete ";;; Commentary" header,
> including a simple usage example.
> 
> JSONRPC is apparently also used for other non-LSP applications. See
> http://json-rpc.info/.

Thanks, this sounds like a good addition.  Does it (or can it) work
with the built-in JSON support we have on the master branch?

And I still wonder whether more of eglot's LSP support should be in
core.  After all, only customizing LSP for a specific language should
be left for the major modes, the rest of interaction with LSP is
probably language-agnostic, right?

Re: New jrpc.el JSONRPC library (Was: [ELPA] New package: eglot)

[Philipp Stephani]

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

João Távora <joaotavora@gmail.com> schrieb am Fr., 18. Mai 2018 um
18:28 Uhr:

> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> From: João Távora <joaotavora@gmail.com>
> >> Date: Thu, 10 May 2018 23:34:40 +0100
> >>
> >> I'd like to add Eglot, my new package, to GNU ELPA.
> >>
> >> Eglot, for Emacs Polyglot, is an Emacs client for the LSP, or Language
> >> Server Protocol (https://microsoft.github.io/language-server-protocol/
> ).
> >
> > FWIW, I think LSP support should be part of the core Emacs
> > distribution, as it's an infrastructure necessary for modern support
> > of programming modes.  (I didn't yet look at the package, so if it has
> > separate infrastructure and application levels, perhaps only the
> > infrastructure layers should be in Emacs.)
>
> Hi again, Eli
>
> So, off-list Stefan likewise suggested that some level of LSP
> "infrastructure" is integrated into Emacs, which could lead to merging
> eglot.el and other LSP clients like lsp-mode.el in the future.
>
> As JSONRPC support is a mandatory component of LSP, I've extracted a
> library implementing the JSONRPC spec (see www.jsonrpc.org) out of
> eglot.el.
>
> I propose this library, jrpc.el, for Emacs core (or GNU ELPA, or
> both). I hope people can review it, comment on it, and propose changes,
> particularly developers of other existing LSP-modes.  I attach to the
> end of this mail with a reasonably complete ";;; Commentary" header,
> including a simple usage example.
>

Thanks. I think that separating out JSONRPC from LSP and having a JSONRPC
library in core are highly valuable additions that I'd strongly support.
Some review comments follow; sorry that there are so many, but I think as a
foundational library jrpc.el should be as high-quality as possible:

- Please add extensive unit and integration tests. There are lots of corner
cases (what happens when the server times out, sends an invalid or
incomplete response, sends responses in the wrong order, etc.) that should
all be tested.

- Please remove all defcustoms and defvars. jrpc.el is a low-level library,
it doesn't make sense for it to maintain global state or be globally
customizable. Rather, the timeout could either be a mandatory argument, or
a reasonable constant default could be selected, and the other variables
should be per-process.

- Also please remove `jrpc-current-process'. There is no general notion of
a "current" process; clients should maintain process state themselves and
pass process objects explictly. It seems this is only used as completion
helper anyway, so I think you can remove it entirely and replace it with
something based on `process-list'.

- Please don't use the `error' symbol directly. Instead, define your own
error symbols for each error category using `define-error'. Only then
clients have the chance to catch and handle errors.

- `jrpc-define-process-var' isn't JSONRPC-specific, it should be moved
somewhere else (subr.el)?

- Instead of defining process variables, consider using a structure or
EIEIO class to encapsulate all needed state for the process client.

- Please make it possible to run servers remotely. Unfortunately this isn't
possible with `make-process', so you have to use `start-file-process'
instead if `default-directory' is remote. As an alternative to using
`default-directory', consider passing a REMOTE-DIRECTORY argument to
`jrpc-connect' so that the behavior doesn't depend on ambient context.

- I think many of the names can be internal (i.e. contain a double dash)
unless they are explicitly part of the public API.

- Probably jrpc-async-request should just return nil. Or is there a
situation where callers have to know about IDs or timers? I think those
should be treated as implementation details and not be exposed to clients.

- Please use hashtables or alists instead of plists. There are many more
functions to operate on hashtables and alists, and json.c doesn't support
plists.

- The HTTP-style enveloping (using HTTP headers to separate responses) is
technically not part of JSONRPC. You could either add support for other
enveloping methods (though I'm not aware of any), or at least call this out
in the documentation.

- I think `string-bytes' isn't guaranteed to return the number of UTF-8
bytes. Rather you should encode the string explicitly to a unibyte string
using `encode-coding-string', and then use `no-conversion' for the process
coding systems.

- Consider also sending (and parsing) a Content-Type header: Content-Type:
application/vscode-jsonrpc; charset=utf-8.

- I don't quite understand how `jrpc-ready-predicates' is supposed to be
used. Are clients supposed to have some form of server-specific sidechannel
to detect when the server is ready? It might be simpler to solve this
problem in Emacs core, e.g. by adding a timeout to `process-send-string'.

- I'd call the library jsonrpc.el (or json-rpc.el or jsonrpc2.el or...) if
that name isn't already taken.

Thanks again for working on this!

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

Re: New jrpc.el JSONRPC library (Was: [ELPA] New package: eglot)

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 19 May 2018 13:34:45 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, monnier@iro.umontreal.ca, vibhavp@gmail.com, 
> 	emacs-devel@gnu.org
> 
> - I think `string-bytes' isn't guaranteed to return the number of UTF-8
> bytes.

Yes, it is guaranteed.  See its implementation.

There could be a problem if the original string includes raw bytes --
then the value returned by string-bytes will not match the length of
the bytestream after encoding by UTF-8.  But since JSON requires UTF-8
without any raw bytes, I'm not sure it matters that we will send an
incorrect byte count in that case, because it means there's a bug in
the Lisp program which produced the string.

> Rather you should encode the string explicitly to a unibyte string
> using `encode-coding-string', and then use `no-conversion' for the process
> coding systems.

That's possible, but since process-send-string encodes the string, it
should be enough to bind coding-system-for-write to utf-8, and let
process-send-string encode it.  Assuming we trust the caller to
produce a string without raw bytes, that is.

Re: New jrpc.el JSONRPC library

[João Távora]

Philipp Stephani <p.stephani2@gmail.com> writes:

> João Távora <joaotavora@gmail.com> schrieb am Fr., 18. Mai 2018 um 18:28 Uhr:
>
>>  I propose this library, jrpc.el, for Emacs core (or GNU ELPA, or

> Thanks. I think that separating out JSONRPC from LSP and having a
> JSONRPC library in core are highly valuable additions that I'd
> strongly support.  Some review comments follow; sorry that there are
> so many, but I think as a foundational library jrpc.el should be as
> high-quality as possible:

Apology *not accepted* :-). Having reviewers provide so many insightful
comments is a previlige and a pleasure. The high standards of the Emacs
project are one of the reasons I like to contribute to it.

I'll triaged your comments into "No", "Maybe", "Ok". The former require
some more convincing on your part :-)

I've already applied some of your suggestions (and some that Stefan sent
off-list, too). See the jsonrpc-refactor branch of Eglot's github
repository. The latest version of jsonrpc.el (I changed the name) lives
in:

https://raw.githubusercontent.com/joaotavora/eglot/jsonrpc-refactor/jsonrpc.el

> - Please add extensive unit and integration tests. There are lots of
> corner cases (what happens when the server times out, sends an invalid
> or incomplete response, sends responses in the wrong order, etc.) that
> should all be tested.

OK. It's in the plans, if not in the stars. Help appreciated. :-) We can
convert some of those examples to tests.  Eglot's jsonrpc-refactor
branch, where I'm currently developing jsonrpc.el, already has
integration tests, so at least a part of jsonrpc.el is already covered.

> - Please remove all defcustoms and defvars. jrpc.el is a low-level
> library, it doesn't make sense for it to maintain global state or be
> globally customizable. Rather, the timeout could either be a mandatory
> argument, or a reasonable constant default could be selected, and the
> other variables should be per-process.

OK. Makes sense. Done.

> - Also please remove `jrpc-current-process'. There is no general
> notion of a "current" process;

Maybe. Not convinced. It's opininated, sure. It supposes that just most
JSONRPC applications would have the notion of a "session". So jsonrpc.el
makes this suggestion to its users, allowing them to configure what a
session means for them. They would also get access to M-x
jsonrpc-events-buffer. This is entirely optional on the client's part,
but I see no harm in JSONRPC providing this convenience. Perhaps the
name is off and should mention "session".

> - Please don't use the `error' symbol directly. Instead, define your
> own error symbols for each error category using `define-error'. Only
> then clients have the chance to catch and handle errors.

OK. Done. Though technically they can already catch and handle
them. What they can't do is catch *only* JSONRPC errors and let the
others thru. I happen to think that's a bad idea because a JSONRPC error
is an error in your program. But there's no reason to deny this, too.

> - `jrpc-define-process-var' isn't JSONRPC-specific, it should be moved
> somewhere else (subr.el)?

Yes, but if we do this it'll be harder for clients like eglot.el to keep
supporting Emacs 26.1 (assuming jsonrpc.el would be in GNU ELPA and
master simultaneously).

> - Instead of defining process variables, consider using a structure or
> EIEIO class to encapsulate all needed state for the process client.
>
Maybe. Structures is a bad idea, but defclass isn't at all.

> - Please make it possible to run servers remotely. Unfortunately this
> isn't possible with `make-process', so you have to use
> `start-file-process' instead if `default-directory' is remote. As an
> alternative to using `default-directory', consider passing a
> REMOTE-DIRECTORY argument to `jrpc-connect' so that the behavior
> doesn't depend on ambient context.

Maybe. I don't fully understand the example. But jsonrpc-connect allows you to
pass any pre-connected process object. It will replace its buffer,
sentinel and filter. In the "autostart-server" branch of eglot.el, for
example, there is a function to I start a local process with special
arguments to that it open a server, then connect to it, then return that
connected process. This is for Windows LSP servers that can't to stdio
comms.

> - I think many of the names can be internal (i.e. contain a double
> dash) unless they are explicitly part of the public API.

Maybe. What names? The names that I want to make public are already
public, and all the others aren't.

> - Probably jrpc-async-request should just return nil. Or is there a
> situation where callers have to know about IDs or timers? I think
> those should be treated as implementation details and not be exposed
> to clients.

OK. Makes sense.

> - Please use hashtables or alists instead of plists. There are many
> more functions to operate on hashtables and alists,

Maybe. Hmm, I really really don't know about this. Have you tried
cl-destructuring-bind with plists? It really is the SH&T for JSON.

> and json.c doesn't support plists.

That's the superior argument. I didn't know about json.c until very
recently. I'll play along, but can you help me rewrite jsonrpc-lambda to
something less consing perhaps?

(cl-defmacro jsonrpc-lambda (cl-lambda-list &body body)
  (declare (indent 1) (debug (sexp &rest form)))
  (let ((jsonobj (gensym "jsonrpc-lambda-elem")))
    `(lambda (,jsonobj)
       (apply (cl-function (lambda ,cl-lambda-list ,@body))
              (let (plist)
                (maphash (lambda (k v)
                           (push v plist)
                           (push (if (keywordp k) k
                                   (intern (format ":%s" k)))
                                 plist))
                         ,jsonobj)
                plist)))))

> - The HTTP-style enveloping (using HTTP headers to separate responses)
> is technically not part of JSONRPC. You could either add support for
> other enveloping methods (though I'm not aware of any), or at least
> call this out in the documentation.

OK. Yes, it's most definetly not JSONRPC. It's an LSP thing. This is the
reason I need to rework the "process" object into a proper defclass and
make generic functions that only apply this enveloping to the proper
LSPlike objects (which could/should be the default object types returned
by jsonrpc-connect).

> - I think `string-bytes' isn't guaranteed to return the number of
> UTF-8 bytes. Rather you should encode the string explicitly to a
> unibyte string using `encode-coding-string', and then use
> `no-conversion' for the process coding systems.
>
I think Eli has already answered this authoritatively.

> - Consider also sending (and parsing) a Content-Type header:
> Content-Type: application/vscode-jsonrpc; charset=utf-8.

OK.

> - I don't quite understand how `jrpc-ready-predicates' is supposed to
> be used. Are clients supposed to have some form of server-specific
> sidechannel to detect when the server is ready?

Yes that's precisely it (though it's entirely optional on the client
part).

> It might be simpler to solve this problem in Emacs core, e.g. by
> adding a timeout to `process-send-string'.

No, the "ready" here is for the application, not the transport.

I understand it's obscure for a reader, so let me explain and then this
might go into the doc.  Obviously, this came out of necessity in
eglot.el, but I think its good functionality to keep in jsonrpc.el
refactor (and it'd be really hard to implement outside it.)

The problem arises in the ordering of sending multiple asynchronous
JSONRPC requests. So, for example in LSP, it doesn't make sense to send
most requests if there are outstanding changes in the buffer that you
haven't told the server about. But because the latter can come in
through idle timers, it's useful to have this generic :deferred
mechanism: you just put in `jsonrpc-ready-predicates' a function that
returns nil if there are outstanding changes and then write code without
worrying about whether the server is ready in that particular
situation. This is better than sprinkling the code with
"send-outstanding-changes-just-to-be-sure" before you make requests.

RLS (Rust's Language Server) has a more complicated limitation, so
eglot.el puts an additional predicate there for rust-mode.el

> - I'd call the library jsonrpc.el (or json-rpc.el or jsonrpc2.el
> or...) if that name isn't already taken.

OK. I don't think it is. Done

João

Re: New jrpc.el JSONRPC library

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca, vibhavp@gmail.com
>> Date: Fri, 18 May 2018 17:27:54 +0100
>> 
>> So, off-list Stefan likewise suggested that some level of LSP
>> "infrastructure" is integrated into Emacs, which could lead to merging
>> eglot.el and other LSP clients like lsp-mode.el in the future.
>> 
>> As JSONRPC support is a mandatory component of LSP, I've extracted a
>> library implementing the JSONRPC spec (see www.jsonrpc.org) out of
>> eglot.el.
>> 
>> I propose this library, jrpc.el, for Emacs core (or GNU ELPA, or
>> both). I hope people can review it, comment on it, and propose changes,
>> particularly developers of other existing LSP-modes.  I attach to the
>> end of this mail with a reasonably complete ";;; Commentary" header,
>> including a simple usage example.
>> 
>> JSONRPC is apparently also used for other non-LSP applications. See
>> http://json-rpc.info/.
>
> Thanks, this sounds like a good addition.  Does it (or can it) work
> with the built-in JSON support we have on the master branch?

Not yet. I didn't know about that, but indeed it has to be on the
roadmap, so I'll start looking into it.

> And I still wonder whether more of eglot's LSP support should be in
> core.  After all, only customizing LSP for a specific language should
> be left for the major modes, the rest of interaction with LSP is
> probably language-agnostic, right?

In fact, no. At least not ideally. *All* of LSP is
language-agnostic. The only two things eglot.el does mode-locally are:

1. a very small tweak for `rust-mode', whose not-entirely-compliant RLS
   server has some trouble that eglot works around and

2. a global `eglot-server-programs' variable mapping major-modes to
   server-launching commands.

The first should go into rust-mode.el (not currently in Emacs/ELPA). The
second one could be replaced by a (setq eglot-server-program <program>)
int he mode hook.

So again, if LSP support is wanted, eglot.el fits that bill (and better
that others IMNSHO :-) But I'd let it mature out of core for a little
while more.

João

Re: New jrpc.el JSONRPC library

[Eli Zaretskii]

> From: joaotavora@gmail.com (João Távora)
> Cc: emacs-devel@gnu.org,  monnier@iro.umontreal.ca,  vibhavp@gmail.com
> Date: Sun, 20 May 2018 16:54:42 +0100
> 
> > Thanks, this sounds like a good addition.  Does it (or can it) work
> > with the built-in JSON support we have on the master branch?
> 
> Not yet. I didn't know about that, but indeed it has to be on the
> roadmap, so I'll start looking into it.

Thanks, I think this is an important development direction.  We added
JSON implementation in C for a good reason, so we'd like to have other
core package be able to use that.

> So again, if LSP support is wanted, eglot.el fits that bill (and better
> that others IMNSHO :-) But I'd let it mature out of core for a little
> while more.

Fair enough, thanks.

Re: New jrpc.el JSONRPC library

[Eli Zaretskii]

> From: joaotavora@gmail.com (João Távora)
> Date: Sun, 20 May 2018 16:43:59 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, monnier@iro.umontreal.ca, vibhavp@gmail.com,
> 	emacs-devel@gnu.org
> 
> > - I think `string-bytes' isn't guaranteed to return the number of
> > UTF-8 bytes. Rather you should encode the string explicitly to a
> > unibyte string using `encode-coding-string', and then use
> > `no-conversion' for the process coding systems.
> >
> I think Eli has already answered this authoritatively.

Note that my response did assume you will bind coding-system-for-write
to utf-8 around the call to process-send-string, because relying on it
being so by default is asking for trouble.

Re: New jrpc.el JSONRPC library

[João Távora]

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

Ok, will do.

On Sun, May 20, 2018, 17:06 Eli Zaretskii <eliz@gnu.org> wrote:

> > From: joaotavora@gmail.com (João Távora)
> > Date: Sun, 20 May 2018 16:43:59 +0100
> > Cc: Eli Zaretskii <eliz@gnu.org>, monnier@iro.umontreal.ca,
> vibhavp@gmail.com,
> >       emacs-devel@gnu.org
> >
> > > - I think `string-bytes' isn't guaranteed to return the number of
> > > UTF-8 bytes. Rather you should encode the string explicitly to a
> > > unibyte string using `encode-coding-string', and then use
> > > `no-conversion' for the process coding systems.
> > >
> > I think Eli has already answered this authoritatively.
>
> Note that my response did assume you will bind coding-system-for-write
> to utf-8 around the call to process-send-string, because relying on it
> being so by default is asking for trouble.
>

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

Re: New jrpc.el JSONRPC library

[Clément Pit-Claudel]

On 2018-05-20 11:54, João Távora wrote:
> In fact, no. At least not ideally. *All* of LSP is
> language-agnostic.

Doesn't LSP-mode let you use language-specific additions, for modes that require more complex interaction than the default style?

Re: New jrpc.el JSONRPC library

[Josh Elsasser]

On May 20, 2018, at 12:13 PM, Clément Pit-Claudel <cpitclaudel@gmail.com> wrote:
> On 2018-05-20 11:54, João Távora wrote:
>> In fact, no. At least not ideally. *All* of LSP is
>> language-agnostic.
> 
> Doesn't LSP-mode let you use language-specific additions, for modes that require more complex interaction than the default style?

Yes, there’s some LSP-mode package right now with a broad set of custom extensions. cquery[1] has a set of extensions to provide richer cross-references, colour inactive code regions, and run semantic highlighting over a file. They hook in via an lsp-mode primitive right now (`lsp-client-on-notification-client’) that registers callback for custom JSON RPC methods.

After working through the eglot.el source, it looks like the same thing could be managed by just defining a `eglot—server-myLangServer/customMethodName` handler. The API for sending/receive input from the server looks really clean, too -- the various custom extensions floating around out there could easily be implemented as langserver-specific packages with a depend on eglot.el


[1]: https://github.com/cquery-project/emacs-cquery

Re: New jrpc.el JSONRPC library

[João Távora]

Clément Pit-Claudel <cpitclaudel@gmail.com> writes:

> On 2018-05-20 11:54, João Távora wrote:
>> In fact, no. At least not ideally. *All* of LSP is
>> language-agnostic.
>
> Doesn't LSP-mode let you use language-specific additions, for modes
> that require more complex interaction than the default style?

If your question is about lsp-mode.el, I'm not the best person to ask.

If it's about about LSP in general, here's my understanding of the
spec. LSP 3.0 allows "experimental" features to the protocol to be
explored between clients and servers, but as the wording suggests, the
idea is that these features are incubated in a some server-client
combinations, and eventually make it into the official spec in a
language-agnostic way.

LSP goes a long way to provide, in a language-agnostic way, things that
Emacs would normally do major-modely (like "codeAction" for interactive
commands, "rangeFormatting" for indentation, etc). 

That said, as Josh points out in the other thread, eglot.el's plan is
not to shut the door on these experiments in any way. That hypothetical
code should go into the major-mode's .el file, hopefully briefly, until
eventually it ends up in eglot.el as a part of the official spec.

João

Re: New jrpc.el JSONRPC library

[João Távora]

Josh Elsasser <josh@elsasser.ca> writes:

> On May 20, 2018, at 12:13 PM, Clément Pit-Claudel <cpitclaudel@gmail.com> wrote:
>> On 2018-05-20 11:54, João Távora wrote:

> After working through the eglot.el source, it looks like the same
> thing could be managed by just defining a
> `eglot—server-myLangServer/customMethodName` handler. The API for
> sending/receive input from the server looks really clean, too -- the
> various custom extensions floating around out there could easily be
> implemented as langserver-specific packages with a depend on eglot.el

Thanks for the positive feedback, and indeed creating a clean API was a
design goal, but notice that:

  (cl-defun eglot--server-myLangServer/customMethodName
     (proc &key param1 param2)
     ...) 

has two dashes in it. It isn't (yet) a part of the official eglot.el LSP
API, and your particular example might very well become something like:

  (cl-defmethod eglot-handle-notification
     (proc (method (eql :myLangServer/customMethodName)) &key param1 param2)
     ...)

or, for requests

  (cl-defmethod eglot-handle-request
     (proc id (method (eql :myLangServer/customMethodName)) &key param1 param2)
     ...)

This seems cleaner than the "automagical" function names, although it
has two minor drawbacks:

1. M-x imenu won't let me search methods by their specializers

2. eldoc mode doesn't highlight :param1 and :param2 while writing a
   method call (but this isn't really a problem in this case because
   you're not going to be writing the function calls yourself).

João

Re: New jrpc.el JSONRPC library

[Clément Pit-Claudel]

On 2018-05-20 19:11, João Távora wrote:
>> Doesn't LSP-mode let you use language-specific additions, for modes
>> that require more complex interaction than the default style?
> 
> If your question is about lsp-mode.el, I'm not the best person to ask.

Sorry, I meant LSP. I'm not sure why I added -mode after this.

> That said, as Josh points out in the other thread, eglot.el's plan is
> not to shut the door on these experiments in any way. That hypothetical
> code should go into the major-mode's .el file, hopefully briefly, until
> eventually it ends up in eglot.el as a part of the official spec.

Very nice.

The specific scenario I'm curious about is proof assistants like Coq or F*. In these, the buffer is divided into a number of regions (definitions), each of which has a state: unsent, sent, processed.  The user tells emacs which fragments to send and when, and the server responds as it processes the fragments.

I'd love to be able to move to LSP, instead of having to recode clients every time.

Clément.

Re: New jrpc.el JSONRPC library

[Josh Elsasser]

Clément Pit-Claudel <cpitclaudel@gmail.com> wrote:

> Thanks for the positive feedback, and indeed creating a clean API was a
> design goal, but notice that:
> 
>  (cl-defun eglot--server-myLangServer/customMethodName
>     (proc &key param1 param2)
>     ...) 
> 
> has two dashes in it. It isn't (yet) a part of the official eglot.el LSP
> API, and your particular example might very well become something like:

Ah, fair. I was mostly thinking aloud about extending the current GitHub master
with cquery support, which'd mean hideifdef support (if I was going to use it
at my day job, at least). May’ve been getting a little ahead of myself :)

> 
>  (cl-defmethod eglot-handle-notification
>     (proc (method (eql :myLangServer/customMethodName)) &key param1 param2)
>     ...)
> 
> or, for requests
> 
>  (cl-defmethod eglot-handle-request
>     (proc id (method (eql :myLangServer/customMethodName)) &key param1 param2)
>     ...)
> 
> This seems cleaner than the "automagical" function names, although it
> has two minor drawbacks:

IMHO the cl-defmethod approach looks a lot nicer for someone hoping to build off
eglot. Took a bit of headscratching to make the connection between the intern +
fboundp and having the various handler functions, and I could see it being a pain
to manage scores of magic functions scattered throughout files.

Can’t wait to give eglot a spin, though! I’ll try and implement / test basic cquery
support in the next couple days. I’ll need to add an extra per-proc variable to pass
a required initialzationOptions param (persistent cache directory), but that shouldn’t
prove too difficult.

-- Josh

Re: New jrpc.el JSONRPC library

[Aaron Ecay]

Hi João,

One small remark:

2018ko maiatzak 20an, João Távora-ek idatzi zuen:

[...]

>> - Please use hashtables or alists instead of plists. There are many
>> more functions to operate on hashtables and alists,
> 
> Maybe. Hmm, I really really don't know about this. Have you tried
> cl-destructuring-bind with plists? It really is the SH&T for JSON.

You can use pcase-let* and the ‘map’ pcase pattern (from the built-in
map.el) as a substitute when working with alists and hash tables.  IOW,
the following two code snippets are equivalent:

(pcase-let* (((map foo bar) '((foo . 1) (bar . 2))))
  (+ foo bar))

(cl-destructuring-bind (&key foo bar) '(:foo 1 :bar 2)
  (+ foo bar))

-- 
Aaron Ecay

Re: New jrpc.el JSONRPC library

[João Távora]

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

Hi Aaron,

Nice, i didn't know about that. Does it have an equivalent to
&allow-other-keys and &rest?

João

On Mon, May 21, 2018, 14:31 Aaron Ecay <aaronecay@gmail.com> wrote:

> Hi João,
>
> One small remark:
>
> 2018ko maiatzak 20an, João Távora-ek idatzi zuen:
>
> [...]
>
> >> - Please use hashtables or alists instead of plists. There are many
> >> more functions to operate on hashtables and alists,
> >
> > Maybe. Hmm, I really really don't know about this. Have you tried
> > cl-destructuring-bind with plists? It really is the SH&T for JSON.
>
> You can use pcase-let* and the ‘map’ pcase pattern (from the built-in
> map.el) as a substitute when working with alists and hash tables.  IOW,
> the following two code snippets are equivalent:
>
> (pcase-let* (((map foo bar) '((foo . 1) (bar . 2))))
>   (+ foo bar))
>
> (cl-destructuring-bind (&key foo bar) '(:foo 1 :bar 2)
>   (+ foo bar))
>
> --
> Aaron Ecay
>

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

Re: New jrpc.el JSONRPC library

[Aaron Ecay]

Hi João,

2018ko maiatzak 21an, João Távora-ek idatzi zuen:
> 
> Hi Aaron,
> 
> Nice, i didn't know about that. Does it have an equivalent to
> &allow-other-keys and &rest?

&allow-other-keys is always true for the pcase version.  As I understand
what &rest does, it lets you bind certain keys, and then bind the map
minus those keys to another variable.  Thereʼs nothing like that out of
the box, but it can easily be implemented:

(pcase-defmacro map-and-rest (rest-var &rest keys)
  `(and (map ,@keys)
        (app (map-filter (lambda (key _val) (not (memq key ',keys)))) ,rest-var)))

(pcase-let* (((map-and-rest rest foo bar) '((foo . 1) (bar . 2) (baz . 3))))
  (list foo bar rest))
;; -> (1 2 ((baz . 3)))


-- 
Aaron Ecay

Re: New jrpc.el JSONRPC library

[Stefan Monnier]

> Nice, i didn't know about that. Does it have an equivalent to
> &allow-other-keys and &rest?

It behaves as if &allow-other-keys is always given currently.
As for &rest you can do

    (pcase-let* ((`(,first . ,(and rest (map foo bar)))
                  '(0 (foo . 1) (bar . 2))))
      (list first rest foo bar))
    =>
    (0 ((foo . 1) (bar . 2)) 1 2)

At that point, the readability suffers, tho.


-- Stefan

Re: New jrpc.el JSONRPC library

[João Távora]

Aaron Ecay <aaronecay@gmail.com> writes:

> Hi João,
>
> 2018ko maiatzak 21an, João Távora-ek idatzi zuen:
>> 
>> Hi Aaron,
>> 
>> Nice, i didn't know about that. Does it have an equivalent to
>> &allow-other-keys and &rest?
>
> &allow-other-keys is always true for the pcase version.
> As I understand what &rest does, it lets you bind certain keys, and
> then bind the map minus those keys to another variable.

No, not "minus those keys", those keys are included in the &rest var
too. 

> Thereʼs nothing like that out of
> the box, but it can easily be implemented:
>
> (pcase-defmacro map-and-rest (rest-var &rest keys)
>   `(and (map ,@keys)
>         (app (map-filter (lambda (key _val) (not (memq key ',keys)))) ,rest-var)))
>
> (pcase-let* (((map-and-rest rest foo bar) '((foo . 1) (bar . 2) (baz . 3))))
>   (list foo bar rest))
> ;; -> (1 2 ((baz . 3)))

Hmm:

1. Not particularly readable for someone  that encounters the second
   form. Can one M-. from map-and-rest into the defmacro to learn what
   it is doing?
2. The absence of control over &allow-other-keys is a big minus. It's
   very useful to a) check that the object isn't providing an unknown
   key, or to check that you haven't mistyped a key name. Can you
   code it into the pcase-defmacro?
3. Also can you code the more complex capabilities of &key into there?

   ;; default values
   (cl-destructuring-bind (&rest rest &key (foo "default") bar) nil
      (list foo bar)) ;; -> ("default" nil)

   ;; provided-p
   (cl-destructuring-bind (&rest rest &key (foo nil foo-provided-p) bar)
          '(:bar nil :foo nil)
      (list foo bar foo-provided-p)) ;; -> (nil nil t)

   ;; alias
   (cl-destructuring-bind (&rest rest &key
          ((:extremely-long-foo-with-the-airplane foo)) bar)
          '(:extremely-long-foo-with-the-airplane 42)
      (list foo bar)) ;; -> (42 nil)

I think clients of jsonrpc.el should be able to use whatever they want
to destructure JSON (though I recommend plists and I think it should be
the default datatype passed to the dispatcher). Internally, in
jsonrpc.el there isn't an awlful lot of destructuring going on, so if
someone can provide a superior argument (performance?) backed up by some
measurements, I don't mind switching. It's just that the readability
argument isn't really holding up against cl lambda lists.

João

   
   

Re: New jrpc.el JSONRPC library

[Aaron Ecay]

Hi João,

2018ko maiatzak 21an, João Távora-ek idatzi zuen:

> No, not "minus those keys", those keys are included in the &rest var
> too.

In that case, Stefanʼs reply tells you how to get the equivalent.

> 
> 
> Hmm:
> 
> 1. Not particularly readable for someone  that encounters the second
>    form. Can one M-. from map-and-rest into the defmacro to learn what
>    it is doing?

No, but the docstrings of all the pcase-defmacros are automatically
included in the docstring of pcase (the main entry point).  Of course,
the pcase-defmacro I wrote didnʼt have a docstring because it was just a
quick proof of concept.

> > 2. The absence of control over &allow-other-keys is a big minus. It's
>    very useful to a) check that the object isn't providing an unknown
>    key, or to check that you haven't mistyped a key name. Can you
>    code it into the pcase-defmacro?

Itʼs possible to do so, but I think it would be more perspicuous to have
the validation and destructuring separate.  One might want to assert
that the record contains some field X, without caring what Xʼs value is
(and thus, not needing to bind X to a lisp variable).

> 3. Also can you code the more complex capabilities of &key into there?
> 
>    ;; default values
>    (cl-destructuring-bind (&rest rest &key (foo "default") bar) nil
>       (list foo bar)) ;; -> ("default" nil)
> 
>    ;; provided-p
>    (cl-destructuring-bind (&rest rest &key (foo nil foo-provided-p) bar)
>           '(:bar nil :foo nil)
>       (list foo bar foo-provided-p)) ;; -> (nil nil t)
> 
>    ;; alias
>    (cl-destructuring-bind (&rest rest &key
>           ((:extremely-long-foo-with-the-airplane foo)) bar)
>           '(:extremely-long-foo-with-the-airplane 42)
>       (list foo bar)) ;; -> (42 nil)

Again one could, but I personally donʼt see the advantage of (foo nil
foo-provided-p) over (memq 'foo (map-keys my-alist)).

Anyway, I first pointed out the possibility of using pcase because I
understood you to be saying that the features of
cl-destructuring-bind+plist were not easily replicated by
anything+alists.  I hope to have shown that those features are indeed
available.  Itʼs still up to you if you are satisfied by the minutiae; I
just didnʼt want you to labor under the misconception that there are no
alternatives.  :)

> 
> I think clients of jsonrpc.el should be able to use whatever they want
> to destructure JSON (though I recommend plists and I think it should be
> the default datatype passed to the dispatcher). 

Indeed, json.el in emacs core has the json-object-type variable to
control the plist-vs-alist-ness of decoded JSON.  Maybe your library can
also work with that variable?

-- 
Aaron Ecay

Re: New jrpc.el JSONRPC library

[Stefan Monnier]

>> 1. Not particularly readable for someone  that encounters the second
>>    form. Can one M-. from map-and-rest into the defmacro to learn what
>>    it is doing?
> No, but the docstrings of all the pcase-defmacros are automatically

Also the "No" above could/should be changed via `describe-symbol-backends`.


        Stefan

Re: New jrpc.el JSONRPC library

[João Távora]

Aaron Ecay <aaronecay@gmail.com> writes:

> Itʼs possible to do so, but I think it would be more perspicuous to have
> the validation and destructuring separate.

So that if you change one you have to change the other? And type the
key over and over?

> One might want to assert that the record contains some field X,
> without caring what Xʼs value is (and thus, not needing to bind X to a
> lisp variable).

Just use '_'

   (cl-destructuring-bind (&key _boring-key very-important)
       '(:very-important 42 :boring-key 92)
       very-important)
  
> Again one could, but I personally donʼt see the advantage of (foo nil
> foo-provided-p) over (memq 'foo (map-keys my-alist)).

You're comparing apples to oranges. Here's the comparison: with this
form, where "foo" appears once.

   (cl-destructuring-bind (&rest all &key (foo 'default provided-p))
      a-plist-map
   ...)

You are, to the best of my pcase.el knowledge so far, doing the same as:

   (pcase-let* ((`(. ,(and all (map foo))) any-kind-of-map)
                (provided-p (and (memq 'foo (map-keys all)) t))
                (foo (if provided-p foo 'default)))
     (some (code (to-check (for-invalid-keys-in) all) '(foo)))
     ...
     )

Where foo appears 5 times.

And I didn't even mention the fact that you can use the cl-dbind lambda
list in a cl-defmethod, for example.  If you `apply' it to a JSON object
you get all the args ready to use + the error reporting when you botch
the funcall because you botched the object (or the spec).

You could do similar stuff with pcase-lambda (though, again, much more
verbosely). But then again that isn't as versatile and pervasive as
cl-defgeneric.

> Anyway, I first pointed out the possibility of using pcase because I
> understood you to be saying that the features of
> cl-destructuring-bind+plist were not easily replicated by
> anything+alists. I hope to have shown that those features are indeed
> available.

Sorry, but they aren't: you have to program them (which is what I
asked you to demonstrate).

... just like cl-dbind doesn't have the main feature of map.el, which is
a uniform interface to different map types. But we were discussing
specifically the merits of plists.

> Itʼs still up to you if you are satisfied by the minutiae;

You know what they say: "the devil is in the minutiae!" :-)

> Indeed, json.el in emacs core has the json-object-type variable to
> control the plist-vs-alist-ness of decoded JSON.  Maybe your library can
> also work with that variable?

Yes, maybe. It must work with json.c above all (and it doesn't support
plists apparently).

João

Re: New jrpc.el JSONRPC library

[Stefan Monnier]

>    (cl-destructuring-bind (&key _boring-key very-important)
>        '(:very-important 42 :boring-key 92)
>        very-important)

FWIW, we could of course do a (pcase-defmacro cl (...) ...) such that

    (cl-destructuring-bind PAT VAL EXP)

can be written

    (pcase-let (((cl PAT) VAL)) EXP)

This is the only significant advantage of pcase patterns: they're
extensible, whereas CL's destructuring patterns are not.

CL's destructuring patterns in return have the advantage of being
available in cl-defmethod whereas there's no pcase-defmethod (yet?).

With respect to the support for "default" and "provided-p" thingies, as
recently mentioned elsewhere I prefer designs where we avoid
distinguishing nil from "absent", so I rarely if ever need them.

But of course, distinguishing nil from "absent" also comes with
advantages, so it's just the usual style tradeoffs: If you're used to
one style the other looks inconvenient.

IOW, it's another bikeshed color.


        Stefan

Re: New jrpc.el JSONRPC library

[Philipp Stephani]

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

Thanks!

João Távora <joaotavora@gmail.com> schrieb am So., 20. Mai 2018 um
17:44 Uhr:

> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> > João Távora <joaotavora@gmail.com> schrieb am Fr., 18. Mai 2018 um
> 18:28 Uhr:
> >
> >>  I propose this library, jrpc.el, for Emacs core (or GNU ELPA, or
>
> > Thanks. I think that separating out JSONRPC from LSP and having a
> > JSONRPC library in core are highly valuable additions that I'd
> > strongly support.  Some review comments follow; sorry that there are
> > so many, but I think as a foundational library jrpc.el should be as
> > high-quality as possible:
>
> Apology *not accepted* :-). Having reviewers provide so many insightful
> comments is a previlige and a pleasure. The high standards of the Emacs
> project are one of the reasons I like to contribute to it.
>
> I'll triaged your comments into "No", "Maybe", "Ok". The former require
> some more convincing on your part :-)
>
> I've already applied some of your suggestions (and some that Stefan sent
> off-list, too). See the jsonrpc-refactor branch of Eglot's github
> repository. The latest version of jsonrpc.el (I changed the name) lives
> in:
>
>
> https://raw.githubusercontent.com/joaotavora/eglot/jsonrpc-refactor/jsonrpc.el


Thanks! Could you maybe use GitHub PRs for further changes? I find it much
easier to comment on them instead of via mail.


>
>
> > - Please add extensive unit and integration tests. There are lots of
> > corner cases (what happens when the server times out, sends an invalid
> > or incomplete response, sends responses in the wrong order, etc.) that
> > should all be tested.
>
> OK. It's in the plans, if not in the stars. Help appreciated. :-) We can
> convert some of those examples to tests.  Eglot's jsonrpc-refactor
> branch, where I'm currently developing jsonrpc.el, already has
> integration tests, so at least a part of jsonrpc.el is already covered.
>

It's OK to add the tests once the API stabilizes. It would be great to have
tests e.g. for
- server process crashes
- server takes too long to respond
- invalid responses
- mismatched IDs
- very large responses
etc., just the usual failure modes.


>
> > - Please remove all defcustoms and defvars. jrpc.el is a low-level
> > library, it doesn't make sense for it to maintain global state or be
> > globally customizable. Rather, the timeout could either be a mandatory
> > argument, or a reasonable constant default could be selected, and the
> > other variables should be per-process.
>
> OK. Makes sense. Done.
>
> > - Also please remove `jrpc-current-process'. There is no general
> > notion of a "current" process;
>
> Maybe. Not convinced. It's opininated, sure. It supposes that just most
> JSONRPC applications would have the notion of a "session". So jsonrpc.el
> makes this suggestion to its users, allowing them to configure what a
> session means for them. They would also get access to M-x
> jsonrpc-events-buffer. This is entirely optional on the client's part,
> but I see no harm in JSONRPC providing this convenience. Perhaps the
> name is off and should mention "session".
>

When reading the code, I was a bit confused to see the notion of a "current
process". Later, I realized that it's only used for some debug helpers and
not for the core API.
I think it would make sense to separate the debug helpers from the core API
(connect, send, receive) more cleanly. I'm even wondering whether the debug
helpers are needed at all. Probably you saw a need for them, otherwise you
wouldn't have added them. But maybe at least in the beginning Emacs's
builtin debug functionality (edebug etc.) is already good enough? If at
all, please separate the debug helpers from the main API (with a comment
block or similar) and move them to the end, as they are not essential.


>
> > - Please don't use the `error' symbol directly. Instead, define your
> > own error symbols for each error category using `define-error'. Only
> > then clients have the chance to catch and handle errors.
>
> OK. Done. Though technically they can already catch and handle
> them. What they can't do is catch *only* JSONRPC errors and let the
> others thru. I happen to think that's a bad idea because a JSONRPC error
> is an error in your program. But there's no reason to deny this, too.
>

When designing a general-purpose API, it's often unknown how it will be
used; often it's used in unpredicted and surprising way. Even if we don't
see a good use case to catch JSON-RPC errors now, it's quite likely that
such a use case will show up in the future once the library is more widely
used, and then the very minor cost of defining specific error symbols pays
off.
Please also use specific error symbols in `jsonrpc-error'.


>
> > - `jrpc-define-process-var' isn't JSONRPC-specific, it should be moved
> > somewhere else (subr.el)?
>
> Yes, but if we do this it'll be harder for clients like eglot.el to keep
> supporting Emacs 26.1 (assuming jsonrpc.el would be in GNU ELPA and
> master simultaneously).
>
> > - Instead of defining process variables, consider using a structure or
> > EIEIO class to encapsulate all needed state for the process client.
> >
> Maybe. Structures is a bad idea, but defclass isn't at all.
>

I'm not so much opposed to structures, it's more of an implementation
detail and a matter of taste whether you prefer structures or classes.
However, there's a pretty strong argument that I forgot about: for
robustness you need a way to restart process and even have them restarted
automatically. Let's say a server process crashes or the user kills it; you
definitely don't want LSP/JSON-RPC to be unusable until the user restarts
Emacs. Therefore, instead of having the 'connect' function create the
process, the send and receive functions should ensure that it's running;
i.e. shift the paradigm from "edge-triggered" (process is created once the
user calls a function) to "level-triggered" (all functions that need a
process make sure the process is available). That is at the same time more
robust and easier to understand. However, it practically requires using a
structure or class as main data type, because the process object needs to
be replaced after creating the client.


>
> > - Please make it possible to run servers remotely. Unfortunately this
> > isn't possible with `make-process', so you have to use
> > `start-file-process' instead if `default-directory' is remote. As an
> > alternative to using `default-directory', consider passing a
> > REMOTE-DIRECTORY argument to `jrpc-connect' so that the behavior
> > doesn't depend on ambient context.
>
> Maybe. I don't fully understand the example. But jsonrpc-connect allows
> you to
> pass any pre-connected process object. It will replace its buffer,
> sentinel and filter. In the "autostart-server" branch of eglot.el, for
> example, there is a function to I start a local process with special
> arguments to that it open a server, then connect to it, then return that
> connected process. This is for Windows LSP servers that can't to stdio
> comms.
>

It's good to have such an extensibility mechanism, and I indeed overlooked
that you can pass a process object to the connect function.
I'm wondering whether that should indeed be the *only* option: leave the
process creation business entirely to the client, and only accept a process
object (or rather, process factory function, because you want to restart
processes as necessary, see above). That is, have the following interface
instead of the 'connect':

(defun jsonrpc-make-client (process-factory)
  "PROCESS-FACTORY gets called an unspecified number of times and has to
return a new live process object each time it's called."
  ...)

Optionally you could provide convenience functions to return such factories
for `make-process' etc., but you could also leave that to the users
entirely.


>
> > - I think many of the names can be internal (i.e. contain a double
> > dash) unless they are explicitly part of the public API.
>
> Maybe. What names? The names that I want to make public are already
> public, and all the others aren't.
>

I'm thinking about things like
jsonrpc-warn, jsonrpc-outstanding-request-ids, etc. These seem like
implementation details that clients shouldn't care about.


>
> > - Probably jrpc-async-request should just return nil. Or is there a
> > situation where callers have to know about IDs or timers? I think
> > those should be treated as implementation details and not be exposed
> > to clients.
>
> OK. Makes sense.
>
> > - Please use hashtables or alists instead of plists. There are many
> > more functions to operate on hashtables and alists,
>
> Maybe. Hmm, I really really don't know about this. Have you tried
> cl-destructuring-bind with plists? It really is the SH&T for JSON.
>

Not really, how would it work? Typically I use plain gethash, assq, etc. Or
`let-alist'.


>
> > and json.c doesn't support plists.
>
> That's the superior argument. I didn't know about json.c until very
> recently. I'll play along, but can you help me rewrite jsonrpc-lambda to
> something less consing perhaps?
>
> (cl-defmacro jsonrpc-lambda (cl-lambda-list &body body)
>   (declare (indent 1) (debug (sexp &rest form)))
>   (let ((jsonobj (gensym "jsonrpc-lambda-elem")))
>     `(lambda (,jsonobj)
>        (apply (cl-function (lambda ,cl-lambda-list ,@body))
>               (let (plist)
>                 (maphash (lambda (k v)
>                            (push v plist)
>                            (push (if (keywordp k) k
>                                    (intern (format ":%s" k)))
>                                  plist))
>                          ,jsonobj)
>                 plist)))))
>

Why do we need jsonrpc-lambda? Writing a normal lambda and some gethash or
assq calls doesn't sound overly bad to me. I'd just remove jsonrpc-lambda
entirely; it's the kind of syntactic sugar that I think doesn't make code
significantly more readable.


>
> > - The HTTP-style enveloping (using HTTP headers to separate responses)
> > is technically not part of JSONRPC. You could either add support for
> > other enveloping methods (though I'm not aware of any), or at least
> > call this out in the documentation.
>
> OK. Yes, it's most definetly not JSONRPC. It's an LSP thing. This is the
> reason I need to rework the "process" object into a proper defclass and
> make generic functions that only apply this enveloping to the proper
> LSPlike objects (which could/should be the default object types returned
> by jsonrpc-connect).
>
> > - I think `string-bytes' isn't guaranteed to return the number of
> > UTF-8 bytes. Rather you should encode the string explicitly to a
> > unibyte string using `encode-coding-string', and then use
> > `no-conversion' for the process coding systems.
> >
> I think Eli has already answered this authoritatively.
>
> > - Consider also sending (and parsing) a Content-Type header:
> > Content-Type: application/vscode-jsonrpc; charset=utf-8.
>
> OK.
>
> > - I don't quite understand how `jrpc-ready-predicates' is supposed to
> > be used. Are clients supposed to have some form of server-specific
> > sidechannel to detect when the server is ready?
>
> Yes that's precisely it (though it's entirely optional on the client
> part).
>
> > It might be simpler to solve this problem in Emacs core, e.g. by
> > adding a timeout to `process-send-string'.
>
> No, the "ready" here is for the application, not the transport.
>
> I understand it's obscure for a reader, so let me explain and then this
> might go into the doc.  Obviously, this came out of necessity in
> eglot.el, but I think its good functionality to keep in jsonrpc.el
> refactor (and it'd be really hard to implement outside it.)
>

If it should stay in jsonrpc.el, then it should be clear without referring
to eglot.el.


>
> The problem arises in the ordering of sending multiple asynchronous
> JSONRPC requests. So, for example in LSP, it doesn't make sense to send
> most requests if there are outstanding changes in the buffer that you
> haven't told the server about. But because the latter can come in
> through idle timers, it's useful to have this generic :deferred
> mechanism: you just put in `jsonrpc-ready-predicates' a function that
> returns nil if there are outstanding changes and then write code without
> worrying about whether the server is ready in that particular
> situation. This is better than sprinkling the code with
> "send-outstanding-changes-just-to-be-sure" before you make requests.
>

Hmm. I need to think a bit more about this. I guess it makes sense, but
please make it a property of the client, not a global variable for now.

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

Re: New jrpc.el JSONRPC library

[João Távora]

Philipp Stephani <p.stephani2@gmail.com> writes:

>  https://raw.githubusercontent.com/joaotavora/eglot/jsonrpc-refactor/jsonrpc.el
>
> Thanks! Could you maybe use GitHub PRs for further changes? I find it
> much easier to comment on them instead of via mail.

Well, the idea was to hold the discussion here since I'm proposing this
as a core library. But I guess we can do both, why not...

Make your pull requests against the `jsonrpc-refactor' branch.

> It's OK to add the tests once the API stabilizes. It would be great to have tests e.g. for
> - server process crashes
> - server takes too long to respond
> - invalid responses
> - mismatched IDs
> - very large responses
> etc., just the usual failure modes.

Have a look at the eglot-tests.el, it's testing some of these things
(crashes, reconnections, timeouts, etc...) though from an LSP
standpoint.

>  > - Also please remove `jrpc-current-process'. There is no general
>  > notion of a "current" process;
>
>  Maybe. Not convinced. It's opininated, sure. It supposes that just most
>  JSONRPC applications would have the notion of a "session". So jsonrpc.el
>  makes this suggestion to its users, allowing them to configure what a
>  session means for them. They would also get access to M-x
>  jsonrpc-events-buffer. This is entirely optional on the client's part,
>  but I see no harm in JSONRPC providing this convenience. Perhaps the
>  name is off and should mention "session".
>
> When reading the code, I was a bit confused to see the notion of a
> "current process". Later, I realized that it's only used for some
> debug helpers and not for the core API.  I think it would make sense
> to separate the debug helpers from the core API (connect, send,
> receive) more cleanly. I'm even wondering whether the debug helpers
> are needed at all. Probably you saw a need for them, otherwise you
> wouldn't have added them. But maybe at least in the beginning Emacs's
> builtin debug functionality (edebug etc.) is already good enough? If
> at all, please separate the debug helpers from the main API (with a
> comment block or similar) and move them to the end, as they are not
> essential.

jsonrpc-events-buffer is a logging facility specially apt at printing
JSON objects and edebug is not really practical to watch sessions of
JSONRPC at a glance. I guess I can extract that stuff into
jsonrpc-debug.el, but lets make that one of the last things.

>  > - Please don't use the `error' symbol directly. Instead, define your
>  > own error symbols for each error category using `define-error'. Only
>  > then clients have the chance to catch and handle errors.
>
>  OK. Done. Though technically they can already catch and handle
>  them. What they can't do is catch *only* JSONRPC errors and let the
>  others thru. I happen to think that's a bad idea because a JSONRPC error
>  is an error in your program. But there's no reason to deny this, too.
>
> When designing a general-purpose API, it's often unknown how it will
> be used; often it's used in unpredicted and surprising way.

Yeah, but it's the API's responsibility to guide the user. Common things
easy, complicate things possible. I just made it possible.

> Please also use specific error symbols in `jsonrpc-error'.

I think I already did,  jsonrpc-error-message and jsonrpc-error-code

>  > - `jrpc-define-process-var' isn't JSONRPC-specific, it should be moved
>  > somewhere else (subr.el)?
>
>  Yes, but if we do this it'll be harder for clients like eglot.el to keep
>  supporting Emacs 26.1 (assuming jsonrpc.el would be in GNU ELPA and
>  master simultaneously).

Never mind, this is going away completely once I use classes to
represent JSON endpoints.

>
>  > - Instead of defining process variables, consider using a structure or
>  > EIEIO class to encapsulate all needed state for the process client.
>  >
>  Maybe. Structures is a bad idea, but defclass isn't at all.
>
> I'm not so much opposed to structures, it's more of an implementation
> detail and a matter of taste whether you prefer structures or classes.

It's most definitly *not* an implementation detail *when using them for
an API*, because if you change the implementation of you structure-based
library, all the code compiled against it will break. You can think of
structures as just an array with fancy accessor names that all expand to
aref's. they're only useful when confined to the same compilation unit.

iow, speed comes at a price.

For a real-life example read the comments in this emacs-lsp issue:
https://github.com/emacs-lsp/lsp-mode/pull/289

> However, there's a pretty strong argument that I forgot about: for
> robustness you need a way to restart process and even have them
> restarted automatically. Let's say a server process crashes or the
> user kills it; you definitely don't want LSP/JSON-RPC to be unusable
> until the user restarts Emacs. Therefore, instead of having the
> 'connect' function create the process, the send and receive functions
> should ensure that it's running; i.e. shift the paradigm from
> "edge-triggered" (process is created once the user calls a function)
> to "level-triggered" (all functions that need a process make sure the
> process is available). That is at the same time more robust and easier
> to understand.

Auto-restarting on send isn't a spectacular idea . You should leave it
to the client to do this imo.

eglot.el, the only existing jsonrpc.el client today, does something that
is similar and that you may think is sufficient: when the process-based
server crashes, jsonrpc.el catches it in the sentinel and calls an
eglot.el hook that reconnects automatically. Obviously, this only
happens if the previous session lasted more than x seconds, to prevent
infinite reconnections.

If this doesn't suit you, in a hypothetical stephani.el client, you
should be able to do the "ensure-connected" dance without much effort
using the same jsonrpc.el API.

>  > - Please make it possible to run servers remotely. Unfortunately this
>  > isn't possible with `make-process', so you have to use
>  > `start-file-process' instead if `default-directory' is remote. As an
>  > alternative to using `default-directory', consider passing a
>  > REMOTE-DIRECTORY argument to `jrpc-connect' so that the behavior
>  > doesn't depend on ambient context.
>
>  Maybe. I don't fully understand the example. But jsonrpc-connect allows you to
>  pass any pre-connected process object. It will replace its buffer,
>  sentinel and filter. In the "autostart-server" branch of eglot.el, for
>  example, there is a function to I start a local process with special
>  arguments to that it open a server, then connect to it, then return that
>  connected process. This is for Windows LSP servers that can't to stdio
>  comms.
>
> It's good to have such an extensibility mechanism, and I indeed
> overlooked that you can pass a process object to the connect function.
> I'm wondering whether that should indeed be the *only* option: leave
> the process creation business entirely to the client, and only accept
> a process object (or rather, process factory function, because you
> want to restart processes as necessary, see above). That is, have the
> following interface instead of the 'connect':
>
> (defun jsonrpc-make-client (process-factory) "PROCESS-FACTORY gets
> called an unspecified number of times and has to return a new live
> process object each time it's called."  ...)

Factories are so 1995 :-) But think I understand what you want more or
less. In master eglot.el, I'm already using classes. If you don't want
the stdio/tcp process-based defaults (or if you want them with some
added twist), you can pass a class symbol to the connect function and
jsonrpc.el will makes the object with nothing but fundamental JSONRPC
stuff (hidden in the base class). There's your factory, I guess. Then
generic functions are called whenever behaviour can be customized.

Figuring out what these generic functions and where the default methods
lie will be the meat of desining the API.

> Optionally you could provide convenience functions to return such
> factories for `make-process' etc., but you could also leave that to
> the users entirely.

Yeah default connection/endpoint class should probably give you a
process similar to what is used for talking LSP with clients. 
   
>  > - I think many of the names can be internal (i.e. contain a double
>  > dash) unless they are explicitly part of the public API.
>
>  Maybe. What names? The names that I want to make public are already
>  public, and all the others aren't.
>
> I'm thinking about things like jsonrpc-warn,
> jsonrpc-outstanding-request-ids, etc.

jsonrpc-outstanding-request-ids is an unused leftover that should be
deleted.

jsonrpc-warn is supposed is a convenience function for leting the client
signal warnings specific to jsonrpc. I see no wrong in making it public,
but I guess you can argue that clients shoudn't introduce in the jsonrpc
log at all.

Any more problems?

>  Maybe. Hmm, I really really don't know about this. Have you tried
>  cl-destructuring-bind with plists? It really is the SH&T for JSON.
>
> Not really, how would it work? Typically I use plain gethash, assq,
> etc. Or `let-alist'.

See my answers in the other thread where I go into length about this. In
short, cl-destructuring-bind gives you destructuring, validation, setting
defaults, and consistency with generic function arguments lists. 

>  > and json.c doesn't support plists.
>
>  That's the superior argument. I didn't know about json.c until very
>  recently. I'll play along, but can you help me rewrite jsonrpc-lambda to
>  something less consing perhaps?
>
>  (cl-defmacro jsonrpc-lambda (cl-lambda-list &body body)
>    (declare (indent 1) (debug (sexp &rest form)))
>    (let ((jsonobj (gensym "jsonrpc-lambda-elem")))
>      `(lambda (,jsonobj)
>         (apply (cl-function (lambda ,cl-lambda-list ,@body))
>                (let (plist)
>                  (maphash (lambda (k v)
>                             (push v plist)
>                             (push (if (keywordp k) k
>                                     (intern (format ":%s" k)))
>                                   plist))
>                           ,jsonobj)
>                  plist)))))
>
> Why do we need jsonrpc-lambda? Writing a normal lambda and some
> gethash or assq calls doesn't sound overly bad to me. I'd just remove
> jsonrpc-lambda entirely; it's the kind of syntactic sugar that I think
> doesn't make code significantly more readable.

Here's an example: Suppose you're implementing a spec that requires you
to iterate an array of JSON objects that

 - *must* have the key :foo;
 - *may* have the key :bar, defaulting to "baz";
 - *must not* have any other key.

I do this with:

   (mapc (jsonrpc-lambda (&key (foo nil foo-provided-p) (bar "baz"))
              (unless foo-provided-p (error "Gotta have foo!"))
              (frobnicate foo bar))
         json-objects)

Can you come up with something more concise in elisp? Come to think of
it, what about in any other language?

You could have used a cl-defmethod/cl-defun instead of the
jsonrpc-lambda anonymous function.

>  I understand it's obscure for a reader, so let me explain and then this
>  might go into the doc.  Obviously, this came out of necessity in
>  eglot.el, but I think its good functionality to keep in jsonrpc.el
>  refactor (and it'd be really hard to implement outside it.)
>
> If it should stay in jsonrpc.el, then it should be clear without
> referring to eglot.el.

It's not a problem specific to eglot.el, but to LSP in general and other
applications that process events asynchronously but need order in their
communications.

> Hmm. I need to think a bit more about this. I guess it makes sense,
> but please make it a property of the client, not a global variable for
> now.

It's not exactly a "global variable", but a hook variable. And it's
going to be a generic function with a default "send immediately"
behaviour. It can't easily be implemented entirely in the client, since
it requires that the client cannot tell if the request or notificaiton
sent really went through immediately.

Let me try to reword generally: a jsonrpc-client in an order-sensitive
JSONRPC-based protocol (such as LSP) *can* indicate to jsonrpc.el that
it *may* defer certain requests to the future. The decision is not
jsonrpc's to make: the client must customize an API point (used to be
the hook, now is a cl-method) that the jsonrpc.el server probes from
time to time. This enables the client to write code as if it weren't
order-sensitive at all.

João

Re: New jrpc.el JSONRPC library

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> > Thanks, this sounds like a good addition.  Does it (or can it) work
>> > with the built-in JSON support we have on the master branch?
>> Not yet. I didn't know about that, but indeed it has to be on the
>> roadmap, so I'll start looking into it.
> Thanks, I think this is an important development direction.  We added
> JSON implementation in C for a good reason, so we'd like to have other
> core package be able to use that.

After having a look at json.c, I confirmed that it isn't designed to
work with property lists (plists). Is this for a very good technical
reason or just a personal preference for alists and hash-tables?

If json.c provided plist support it could more easily become a drop-in
replacement for json.el in the future. In my opinion, as I wrote
elsewhere, it would also assist in the destructuring of JSON objects in
elisp with cl-compatible lambda lists (cl-defmethod,
cl-destructuring-bind, etc..)

It appears to be very easy to add plist support to json-parse-string in
json.c. See the attached patch. If there are no great objections I can
add support for plist support to json-serialize as well. In this case it
would make sense to respect json.el's variable json-object-type.

Thanks,
João

diff --git a/src/json.c b/src/json.c
index b046d34f66..710d1cf888 100644
--- a/src/json.c
+++ b/src/json.c
@@ -605,6 +605,7 @@ OBJECT.  */)
 enum json_object_type {
   json_object_hashtable,
   json_object_alist,
+  json_object_plist
 };
 
 /* Convert a JSON object to a Lisp object.  */
@@ -692,6 +693,25 @@ json_to_lisp (json_t *json, enum json_object_type object_type)
               result = Fnreverse (result);
               break;
             }
+          case json_object_plist:
+            {
+              result = Qnil;
+              const char *key_str;
+              json_t *value;
+              json_object_foreach (json, key_str, value)
+                {
+                  /* No idea if using AUTO_STRING and Fconcat for
+                     making keywords is idiomatic, but seems to work
+                     nicely */
+                  AUTO_STRING (colon, ":");
+                  Lisp_Object key =
+                    Fintern (CALLN (Fconcat, colon, json_build_string (key_str))
+                             , Qnil);
+                  result = Fcons (json_to_lisp (value, object_type), result);
+                  result = Fcons (key, result);
+                }
+              break;
+            }
           default:
             /* Can't get here.  */
             emacs_abort ();
@@ -721,8 +741,10 @@ json_parse_object_type (ptrdiff_t nargs, Lisp_Object *args)
           return json_object_hashtable;
         else if (EQ (value, Qalist))
           return json_object_alist;
+        else if (EQ (value, Qplist))
+          return json_object_plist;
         else
-          wrong_choice (list2 (Qhash_table, Qalist), value);
+          wrong_choice (list3 (Qhash_table, Qalist, Qplist), value);
       }
     default:
       wrong_type_argument (Qplistp, Flist (nargs, args));
@@ -731,16 +753,17 @@ json_parse_object_type (ptrdiff_t nargs, Lisp_Object *args)
 
 DEFUN ("json-parse-string", Fjson_parse_string, Sjson_parse_string, 1, MANY,
        NULL,
-       doc: /* Parse the JSON STRING into a Lisp object.
-This is essentially the reverse operation of `json-serialize', which
-see.  The returned object will be a vector, hashtable, or alist.  Its
+       doc: /* Parse the JSON STRING into a Lisp object.  This is
+essentially the reverse operation of `json-serialize', which see.  The
+returned object will be a vector, hashtable, alist, or plist.  Its
 elements will be `:null', `:false', t, numbers, strings, or further
-vectors, hashtables, and alists.  If there are duplicate keys in an
-object, all but the last one are ignored.  If STRING doesn't contain a
-valid JSON object, an error of type `json-parse-error' is signaled.
-The keyword argument `:object-type' specifies which Lisp type is used
-to represent objects; it can be `hash-table' or `alist'.
-usage: (json-parse-string STRING &key (OBJECT-TYPE \\='hash-table))  */)
+vectors, hashtables, alists, or plists.  If there are duplicate keys
+in an object, all but the last one are ignored.  If STRING doesn't
+contain a valid JSON object, an error of type `json-parse-error' is
+signaled.  The keyword argument `:object-type' specifies which Lisp
+type is used to represent objects; it can be `hash-table', `alist' or
+`plist'.
+usage: (json-parse-string STRING &key (OBJECT-TYPE \\='hash-table)) */)
   (ptrdiff_t nargs, Lisp_Object *args)
 {
   ptrdiff_t count = SPECPDL_INDEX ();
@@ -912,6 +935,7 @@ syms_of_json (void)
 
   DEFSYM (QCobject_type, ":object-type");
   DEFSYM (Qalist, "alist");
+  DEFSYM (Qplist, "plist")
 
   defsubr (&Sjson_serialize);
   defsubr (&Sjson_insert);

[PATCH] jsonrpc.el

[João Távora]

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

Eli Zaretskii <eliz@gnu.org> writes:
>> > Thanks, this sounds like a good addition.  Does it (or can it) work
>> > with the built-in JSON support we have on the master branch?
>> Not yet. I didn't know about that, but indeed it has to be on the
>> roadmap, so I'll start looking into it.
> Thanks, I think this is an important development direction.  We added
> JSON implementation in C for a good reason, so we'd like to have other
> core package be able to use that.

A few weeks ago, I posted my jsonrpc.el here and had good feedback:

   http://lists.gnu.org/archive/html/emacs-devel/2018-06/msg00288.html

If noone objects, I'm going to push the attached patch to core.  Then
I'll work on a manual and NEWS entry.

I'll also make a 1-file ELPA package out of the new lisp/jsonrpc.el
file.  This way eglot.el (already in ELPA) can specify that as a
dependency when running on Emacs 26.1.

>> So again, if LSP support is wanted, eglot.el fits that bill (and
>> better than others IMNSHO :-) But I'd let it mature out of core for a
>> little while more.
> Fair enough, thanks.

eglot.el is coming along nicely.  If the core-and-ELPA strategy works
out for jsonrpc.el, we can think about pushing that too.

João


[-- Attachment #2: 0001-Add-lisp-jsonrpc.el.patch --]
[-- Type: text/x-diff, Size: 45639 bytes --]

From 48ad6deb166642520e2ad66152ac05290d0709ea Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Thu, 28 Jun 2018 13:05:38 +0100
Subject: [PATCH] Add lisp/jsonrpc.el

* lisp/jsonrpc.el: New file

* test/lisp/jsonrpc-tests.el: New file
---
 lisp/jsonrpc.el            | 738 +++++++++++++++++++++++++++++++++++++
 test/lisp/jsonrpc-tests.el | 242 ++++++++++++
 2 files changed, 980 insertions(+)
 create mode 100644 lisp/jsonrpc.el
 create mode 100644 test/lisp/jsonrpc-tests.el

diff --git a/lisp/jsonrpc.el b/lisp/jsonrpc.el
new file mode 100644
index 0000000000..70044320b4
--- /dev/null
+++ b/lisp/jsonrpc.el
@@ -0,0 +1,738 @@
+;;; jsonrpc.el --- JSON-RPC library                  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2018 Free Software Foundation, Inc.
+
+;; Author: João Távora <joaotavora@gmail.com>
+;; Maintainer: João Távora <joaotavora@gmail.com>
+;; Keywords: processes, languages, extensions
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; (Library originally extracted from eglot.el, an Emacs LSP client)
+;;
+;; This library implements the JSONRPC 2.0 specification as described
+;; in http://www.jsonrpc.org/.  As the name suggests, JSONRPC is a
+;; generic Remote Procedure Call protocol designed around JSON
+;; objects.
+;;
+;; Quoting from the spec: "[JSONRPC] is transport agnostic in that the
+;; concepts can be used within the same process, over sockets, over
+;; http, or in many various message passing environments."
+;;
+;; To model this agnosticism, jsonrpc.el uses objects derived from a
+;; base `jsonrpc-connection' class, which is "abstract" or "virtual"
+;; (in modern OO parlance) and represents the connection to the remote
+;; JSON endpoint.  Around this class we can define two distinct APIs:
+;;
+;; 1) A user interface to the JSONRPC _application_, whereby the
+;; `jsonrpc-connection' object is instantiated and used to communicate
+;; with the remote JSONRPC enpoint.
+;;
+;; In this scenario, the JSONRPC application makes the object using
+;; `make-instance' and initiates contacts to the remove endpoint by
+;; passing it to `jsonrpc-notify', `jsonrpc-request' and
+;; `jsonrpc-async-request'.  For handling remotely initiated contacts,
+;; which generally come in asynchronously, the `make-instance'
+;; invocation should include `:request-dispatcher' and
+;; `:notification-dispatcher' initargs, which are two functions
+;; receiving the connection object, a symbol naming the JSONRPC
+;; method, and a JSONRPC "params" object.
+;;
+;; The function passed as `:request-dispatcher' handles the remote
+;; endpoint's requests, which expect a reply from the local endpoint.
+;; The function may return locally or non-locally (error).  A local
+;; return value should be a JSON object which determines a success
+;; response and is serialized in the JSONRPC "result" object forwarded
+;; to the server.  If, however, it uses the `jsonrpc-error' function
+;; to exit non-locally, this responds to the server with a JSONRPC
+;; "error" object instead, the details of which are filled out with
+;; the arguments with whatever was passed to `jsonrpc-error'.  A
+;; suitable error reponse is also sent to the server if the function
+;; error unexpectedly with any other error that doesn't originate in a
+;; deliberate call to `jsonrpc-error'.
+;;
+;; 2) A inheritance-based interface to the JSONPRPC _transport
+;; implementation_, whereby `jsonrpc-connection' is subclassed so
+;; users of the user interface can communicate with JSONRPC endpoints
+;; using different underlying transport strategies.
+;;
+;; There are mandatory and optional parts to this API.
+;;
+;; For initiating contacts to the endpoint and replying to it, that
+;; subclass of `jsonrpc-connection' must implement
+;; `jsonrpc-connection-send' method.
+;;
+;; Likewise, for handling the three types remote endpoint's contacts
+;; (responses to requests, remotely initiated requests and remotely
+;; initiated notifications) the transport implementation must arrange
+;; for the function `jsonrpc-connection-receive' to be called after
+;; noticing a new JSONRPC message on the wire (whatever that "wire"
+;; may be).
+;;
+;; Finally, and optionally, the `jsonrpc-connection' subclass should
+;; implement `jsonrpc-shutdown' and `jsonrpc-running-p' if these
+;; concepts apply to the transport.
+;;
+;; For convenience, jsonrpc.el comes built-in with a
+;; `jsonrpc-process-connection' transport implementation that can talk
+;; to local subprocesses (through stdin/stdout) and TCP hosts using
+;; sockets.  This uses some basic HTTP-style enveloping headers for
+;; JSON objects sent over the wire.  For an example of an application
+;; using this transport scheme on top of JSONRPC, see the Language
+;; Server Protocol
+;; (https://microsoft.github.io/language-server-protocol/specification).
+;; `jsonrpc-process-connection' also implements `jsonrpc-shutdown',
+;; `jsonrpc-running-p'.
+;;
+;;;; About deferred requests and `jsonrpc-connection-p':
+;;
+;; In the user API.
+;;
+;;;; JSON object format:
+;;
+;; JSON objects are exchanged as keyword-value plists: plists are
+;; handed to the dispatcher functions and, likewise, plists should be
+;; given to `jsonrpc-notify', `jsonrpc-request' and
+;; `jsonrpc-async-request'.
+;;
+;; To facilitate handling plists, this library make liberal use of
+;; cl-lib.el and suggests (but doesn't force) its clients to do the
+;; same.  A macro `jsonrpc-lambda' can be used to create a lambda for
+;; destructuring a JSON-object like in this example:
+;;
+;;  (jsonrpc-async-request
+;;   myproc :frobnicate `(:foo "trix")
+;;   :success-fn (jsonrpc-lambda (&key bar baz &allow-other-keys)
+;;                 (message "Server replied back %s and %s!"
+;;                          bar baz))
+;;   :error-fn (jsonrpc-lambda (&key code message _data)
+;;               (message "Sadly, server reports %s: %s"
+;;                        code message)))
+;;
+;;; Code:
+
+(require 'cl-lib)
+(require 'json)
+(require 'eieio)
+(require 'subr-x)
+(require 'warnings)
+(require 'pcase)
+(require 'ert) ; to escape a `condition-case-unless-debug'
+(require 'array) ; xor
+
+\f
+;;; Public API
+;;;
+;;;###autoload
+(defclass jsonrpc-connection ()
+  ((name
+    :accessor jsonrpc-name
+    :initarg :name
+    :documentation "A name for the connection")
+   (-request-dispatcher
+    :accessor jsonrpc--request-dispatcher
+    :initform #'ignore
+    :initarg :request-dispatcher
+    :documentation "Dispatcher for remotely invoked requests.")
+   (-notification-dispatcher
+    :accessor jsonrpc--notification-dispatcher
+    :initform #'ignore
+    :initarg :notification-dispatcher
+    :documentation "Dispatcher for remotely invoked notifications.")
+   (last-error
+    :accessor jsonrpc-last-error
+    :documentation "Last JSONRPC error message received from endpoint.")
+   (-request-continuations
+    :initform (make-hash-table)
+    :accessor jsonrpc--request-continuations
+    :documentation "A hash table of request ID to continuation lambdas.")
+   (-events-buffer
+    :accessor jsonrpc--events-buffer
+    :documentation "A buffer pretty-printing the JSON-RPC RPC events")
+   (-deferred-actions
+    :initform (make-hash-table :test #'equal)
+    :accessor jsonrpc--deferred-actions
+    :documentation "Map (DEFERRED BUF) to (FN TIMER ID).  FN is\
+a saved DEFERRED `async-request' from BUF, to be sent not later\
+than TIMER as ID.")
+   (-next-request-id
+    :initform 0
+    :accessor jsonrpc--next-request-id
+    :documentation "Next number used for a request"))
+  :documentation "Base class representing a JSONRPC connection.
+The following initargs are accepted:
+
+:NAME (mandatory), a string naming the connection
+
+:REQUEST-DISPATCHER (optional), a function of three
+arguments (CONN METHOD PARAMS) for handling JSONRPC requests.
+CONN is a `jsonrpc-connection' object, method is a symbol, and
+PARAMS is a plist representing a JSON object.  The function is
+expected to return a JSONRPC result, a plist of (:result
+RESULT) or signal an error of type `jsonrpc-error'.
+
+:NOTIFICATION-DISPATCHER (optional), a function of three
+arguments (CONN METHOD PARAMS) for handling JSONRPC
+notifications.  CONN, METHOD and PARAMS are the same as in
+:REQUEST-DISPATCHER.")
+
+;;; API mandatory
+(cl-defgeneric jsonrpc-connection-send (conn &key id method params result error)
+  "Send a JSONRPC message to connection CONN.
+ID, METHOD, PARAMS, RESULT and ERROR. ")
+
+;;; API optional
+(cl-defgeneric jsonrpc-shutdown (conn)
+  "Shutdown the JSONRPC connection CONN.")
+
+;;; API optional
+(cl-defgeneric jsonrpc-running-p (conn)
+  "Tell if the JSONRPC connection CONN is still running.")
+
+;;; API optional
+(cl-defgeneric jsonrpc-connection-ready-p (connection what)
+  "Tell if CONNECTION is ready for WHAT in current buffer.
+If it isn't, a deferrable `jsonrpc-async-request' will be
+deferred to the future.  By default, all connections are ready
+for sending requests immediately."
+  (:method (_s _what)   ;; by default all connections are ready
+           t))
+
+\f
+;;; Convenience
+;;;
+(cl-defmacro jsonrpc-lambda (cl-lambda-list &body body)
+  (declare (indent 1) (debug (sexp &rest form)))
+  (let ((e (gensym "jsonrpc-lambda-elem")))
+    `(lambda (,e) (apply (cl-function (lambda ,cl-lambda-list ,@body)) ,e))))
+
+(defun jsonrpc-events-buffer (connection)
+  "Get or create JSONRPC events buffer for CONNECTION."
+  (let* ((probe (jsonrpc--events-buffer connection))
+         (buffer (or (and (buffer-live-p probe)
+                          probe)
+                     (let ((buffer (get-buffer-create
+                                    (format "*%s events*"
+                                            (jsonrpc-name connection)))))
+                       (with-current-buffer buffer
+                         (buffer-disable-undo)
+                         (read-only-mode t)
+                         (setf (jsonrpc--events-buffer connection) buffer))
+                       buffer))))
+    buffer))
+
+(defun jsonrpc-forget-pending-continuations (connection)
+  "Stop waiting for responses from the current JSONRPC CONNECTION."
+  (clrhash (jsonrpc--request-continuations connection)))
+
+(defun jsonrpc-connection-receive (connection message)
+  "Process MESSAGE just received from CONNECTION.
+This function will destructure MESSAGE and call the appropriate
+dispatcher in CONNECTION."
+  (cl-destructuring-bind (&key method id error params result _jsonrpc)
+      message
+    (let (continuations)
+      (jsonrpc--log-event connection message 'server)
+      (setf (jsonrpc-last-error connection) error)
+      (cond
+       (;; A remote request
+        (and method id)
+        (let* ((debug-on-error (and debug-on-error (not (ert-running-test))))
+               (reply
+                (condition-case-unless-debug _ignore
+                    (condition-case oops
+                        `(:result ,(funcall (jsonrpc--request-dispatcher connection)
+                                            connection (intern method) params))
+                      (jsonrpc-error
+                       `(:error
+                         (:code
+                          ,(or (alist-get 'jsonrpc-error-code (cdr oops)) -32603)
+                          :message ,(or (alist-get 'jsonrpc-error-message
+                                                   (cdr oops))
+                                        "Internal error")))))
+                  (error
+                   `(:error (:code -32603 :message "Internal error"))))))
+          (apply #'jsonrpc--reply connection id reply)))
+       (;; A remote notification
+        method
+        (funcall (jsonrpc--notification-dispatcher connection)
+                 connection (intern method) params))
+       (;; A remote response
+        (setq continuations
+              (and id (gethash id (jsonrpc--request-continuations connection))))
+        (let ((timer (nth 2 continuations)))
+          (when timer (cancel-timer timer)))
+        (remhash id (jsonrpc--request-continuations connection))
+        (if error (funcall (nth 1 continuations) error)
+          (funcall (nth 0 continuations) result)))
+       (;; An abnormal situation
+        id (jsonrpc--warn "No continuation for id %s" id)))
+      (jsonrpc--call-deferred connection))))
+
+\f
+;;; Contacting the remote endpoint
+;;;
+(defun jsonrpc-error (&rest args)
+  "Error out with FORMAT and ARGS.
+If invoked inside a dispatcher function, this function is suitable
+for replying to the remote endpoint with an error message.
+
+ARGS can be of the form (FORMAT-STRING . MOREARGS) for replying
+with a -32603 error code and a message formed by formatting
+FORMAT-STRING with MOREARGS.
+
+Alternatively ARGS can be plist representing a JSONRPC error
+object, using the keywords `:code', `:message' and `:data'."
+  (if (stringp (car args))
+      (let ((msg
+             (apply #'format-message (car args) (cdr args))))
+        (signal 'jsonrpc-error
+                `(,msg
+                  (jsonrpc-error-code . ,32603)
+                  (jsonrpc-error-message . ,msg))))
+    (cl-destructuring-bind (&key code message data) args
+      (signal 'jsonrpc-error
+              `(,(format "[jsonrpc] error ")
+                (jsonrpc-error-code . ,code)
+                (jsonrpc-error-message . ,message)
+                (jsonrpc-error-data . ,data))))))
+
+(cl-defun jsonrpc-async-request (connection
+                                 method
+                                 params
+                                 &rest args
+                                 &key _success-fn _error-fn
+                                 _timeout-fn
+                                 _timeout _deferred)
+  "Make a request to CONNECTION, expecting a reply, return immediately.
+The JSONRPC request is formed by METHOD, a symbol, and PARAMS a
+JSON object.
+
+The caller can expect SUCCESS-FN or ERROR-FN to be called with a
+JSONRPC `:result' or `:error' object, respectively.  If this
+doesn't happen after TIMEOUT seconds (defaults to
+`jsonrpc-request-timeout'), the caller can expect TIMEOUT-FN to be
+called with no arguments. The default values of SUCCESS-FN,
+ERROR-FN and TIMEOUT-FN simply log the events into
+`jsonrpc-events-buffer'.
+
+If DEFERRED is non-nil, maybe defer the request to a future time
+when the server is thought to be ready according to
+`jsonrpc-connection-ready-p' (which see).  The request might
+never be sent at all, in case it is overridden in the meantime by
+a new request with identical DEFERRED and for the same buffer.
+However, in that situation, the original timeout is kept.
+
+Returns nil."
+  (apply #'jsonrpc--async-request-1 connection method params args)
+  nil)
+
+(cl-defun jsonrpc-request (connection method params &key deferred timeout)
+  "Make a request to CONNECTION, wait for a reply.
+Like `jsonrpc-async-request' for CONNECTION, METHOD and PARAMS, but
+synchronous, i.e. doesn't exit until anything
+interesting (success, error or timeout) happens.  Furthermore,
+only exit locally (and return the JSONRPC result object) if the
+request is successful, otherwise exit non-locally with an error
+of type `jsonrpc-error'.
+
+DEFERRED is passed to `jsonrpc-async-request', which see."
+  (let* ((tag (cl-gensym "jsonrpc-request-catch-tag")) id-and-timer
+         (retval
+          (unwind-protect ; protect against user-quit, for example
+              (catch tag
+                (setq
+                 id-and-timer
+                 (jsonrpc--async-request-1
+                  connection method params
+                  :success-fn (lambda (result) (throw tag `(done ,result)))
+                  :error-fn
+                  (jsonrpc-lambda
+                      (&key code message data)
+                    (throw tag `(error (jsonrpc-error-code . ,code)
+                                       (jsonrpc-error-message . ,message)
+                                       (jsonrpc-error-data . ,data))))
+                  :timeout-fn
+                  (lambda ()
+                    (throw tag '(error (jsonrpc-error-message . "Timed out"))))
+                  :deferred deferred
+                  :timeout timeout))
+                (while t (accept-process-output nil 30)))
+            (pcase-let* ((`(,id ,timer) id-and-timer))
+              (remhash id (jsonrpc--request-continuations connection))
+              (remhash (list deferred (current-buffer))
+                       (jsonrpc--deferred-actions connection))
+              (when timer (cancel-timer timer))))))
+    (when (eq 'error (car retval))
+      (signal 'jsonrpc-error
+              (cons
+               (format "request id=%s failed:" (car id-and-timer))
+               (cdr retval))))
+    (cadr retval)))
+
+(cl-defun jsonrpc-notify (connection method params)
+  "Notify CONNECTION of something, don't expect a reply."
+  (jsonrpc-connection-send connection
+                           :method method
+                           :params params))
+
+(defconst jrpc-default-request-timeout 10
+  "Time in seconds before timing out a JSONRPC request.")
+
+\f
+;;; Specfic to `jsonrpc-process-connection'
+;;;
+;;;###autoload
+(defclass jsonrpc-process-connection (jsonrpc-connection)
+  ((-process
+    :initarg :process :accessor jsonrpc--process
+    :documentation "Process object wrapped by the this connection.")
+   (-expected-bytes
+    :accessor jsonrpc--expected-bytes
+    :documentation "How many bytes declared by server")
+   (-on-shutdown
+    :accessor jsonrpc--on-shutdown
+    :initform #'ignore
+    :initarg :on-shutdown
+    :documentation "Function run when the process dies."))
+  :documentation "A JSONRPC connection over an Emacs process.
+The following initargs are accepted:
+
+:PROCESS (mandatory), a live running Emacs process object or a
+function of no arguments producing one such object.  The process
+represents either a pipe connection to locally running process or
+a stream connection to a network host.  The remote endpoint is
+expected to understand JSONRPC messages with basic HTTP-style
+enveloping headers such as \"Content-Length:\".
+
+:ON-SHUTDOWN (optional), a function of one argument, the
+connection object, called when the process dies .")
+
+(cl-defmethod initialize-instance ((conn jsonrpc-process-connection) slots)
+  (cl-call-next-method)
+  (let* ((proc (plist-get slots :process))
+         (proc (if (functionp proc) (funcall proc) proc))
+         (buffer (get-buffer-create (format "*%s output*" (process-name proc))))
+         (stderr (get-buffer-create (format "*%s stderr*" (process-name proc)))))
+    (setf (jsonrpc--process conn) proc)
+    (set-process-buffer proc buffer)
+    (process-put proc 'jsonrpc-stderr stderr)
+    (set-process-filter proc #'jsonrpc--process-filter)
+    (set-process-sentinel proc #'jsonrpc--process-sentinel)
+    (with-current-buffer (process-buffer proc)
+      (set-marker (process-mark proc) (point-min))
+      (let ((inhibit-read-only t)) (erase-buffer) (read-only-mode t) proc))
+    (process-put proc 'jsonrpc-connection conn)))
+
+(cl-defmethod jsonrpc-connection-send ((connection jsonrpc-process-connection)
+                                       &rest args
+                                       &key
+                                       _id
+                                       method
+                                       _params
+                                       _result
+                                       _error
+                                       _partial)
+  "Send MESSAGE, a JSON object, to CONNECTION."
+  (when method
+    (plist-put args :method
+               (cond ((keywordp method) (substring (symbol-name method) 1))
+                     ((and method (symbolp method)) (symbol-name method)))))
+  (let* ( (message `(:jsonrpc "2.0" ,@args))
+          (json (jsonrpc--json-encode message))
+          (headers
+           `(("Content-Length" . ,(format "%d" (string-bytes json)))
+             ;; ("Content-Type" . "application/vscode-jsonrpc; charset=utf-8")
+             )))
+    (process-send-string
+     (jsonrpc--process connection)
+     (cl-loop for (header . value) in headers
+              concat (concat header ": " value "\r\n") into header-section
+              finally return (format "%s\r\n%s" header-section json)))
+    (jsonrpc--log-event connection message 'client)))
+
+(defun jsonrpc-process-type (conn)
+  "Return the `process-type' of JSONRPC connection CONN."
+  (process-type (jsonrpc--process conn)))
+
+(cl-defmethod jsonrpc-running-p ((conn jsonrpc-process-connection))
+  "Return non-nil if JSONRPC connection CONN is running."
+  (process-live-p (jsonrpc--process conn)))
+
+(cl-defmethod jsonrpc-shutdown ((conn jsonrpc-process-connection))
+  "Shutdown the JSONRPC connection CONN."
+  (cl-loop
+   with proc = (jsonrpc--process conn)
+   do
+   (delete-process proc)
+   (accept-process-output nil 0.1)
+   while (not (process-get proc 'jsonrpc-sentinel-done))
+   do (jsonrpc--warn
+       "Sentinel for %s still hasn't run,  deleting it!" proc)))
+
+(defun jsonrpc-stderr-buffer (conn)
+  "Get CONN's standard error buffer, if any."
+  (process-get (jsonrpc--process conn) 'jsonrpc-stderr))
+
+\f
+;;; Private stuff
+;;;
+(define-error 'jsonrpc-error "jsonrpc-error")
+
+(defun jsonrpc--json-read ()
+  "Read JSON object in buffer, move point to end of buffer."
+  ;; TODO: I guess we can make these macros if/when jsonrpc.el
+  ;; goes into Emacs core.
+  (cond ((fboundp 'json-parse-buffer) (json-parse-buffer
+                                       :object-type 'plist
+                                       :null-object nil
+                                       :false-object :json-false))
+        (t                            (let ((json-object-type 'plist))
+                                        (json-read)))))
+
+(defun jsonrpc--json-encode (object)
+  "Encode OBJECT into a JSON string."
+  (cond ((fboundp 'json-serialize) (json-serialize
+                                    object
+                                    :false-object :json-false
+                                    :null-object nil))
+        (t                         (let ((json-false :json-false)
+                                         (json-null nil))
+                                     (json-encode object)))))
+
+(cl-defun jsonrpc--reply (connection id &key (result nil result-supplied-p) error)
+  "Reply to CONNECTION's request ID with RESULT or ERROR."
+  (jsonrpc-connection-send connection :id id :result result :error error))
+
+(defun jsonrpc--call-deferred (connection)
+  "Call CONNECTION's deferred actions, who may again defer themselves."
+  (when-let ((actions (hash-table-values (jsonrpc--deferred-actions connection))))
+    (jsonrpc--debug connection `(:maybe-run-deferred ,(mapcar #'caddr actions)))
+    (mapc #'funcall (mapcar #'car actions))))
+
+(defun jsonrpc--process-sentinel (proc change)
+  "Called when PROC undergoes CHANGE."
+  (let ((connection (process-get proc 'jsonrpc-connection)))
+    (jsonrpc--debug connection `(:message "Connection state changed" :change ,change))
+    (when (not (process-live-p proc))
+      (with-current-buffer (jsonrpc-events-buffer connection)
+        (let ((inhibit-read-only t))
+          (insert "\n----------b---y---e---b---y---e----------\n")))
+      ;; Cancel outstanding timers
+      (maphash (lambda (_id triplet)
+                 (pcase-let ((`(,_success ,_error ,timeout) triplet))
+                   (when timeout (cancel-timer timeout))))
+               (jsonrpc--request-continuations connection))
+      (unwind-protect
+          ;; Call all outstanding error handlers
+          (maphash (lambda (_id triplet)
+                     (pcase-let ((`(,_success ,error ,_timeout) triplet))
+                       (funcall error `(:code -1 :message "Server died"))))
+                   (jsonrpc--request-continuations connection))
+        (jsonrpc--message "Server exited with status %s" (process-exit-status proc))
+        (process-put proc 'jsonrpc-sentinel-done t)
+        (delete-process proc)
+        (funcall (jsonrpc--on-shutdown connection) connection)))))
+
+(defun jsonrpc--process-filter (proc string)
+  "Called when new data STRING has arrived for PROC."
+  (when (buffer-live-p (process-buffer proc))
+    (with-current-buffer (process-buffer proc)
+      (let* ((inhibit-read-only t)
+             (connection (process-get proc 'jsonrpc-connection))
+             (expected-bytes (jsonrpc--expected-bytes connection)))
+        ;; Insert the text, advancing the process marker.
+        ;;
+        (save-excursion
+          (goto-char (process-mark proc))
+          (insert string)
+          (set-marker (process-mark proc) (point)))
+        ;; Loop (more than one message might have arrived)
+        ;;
+        (unwind-protect
+            (let (done)
+              (while (not done)
+                (cond
+                 ((not expected-bytes)
+                  ;; Starting a new message
+                  ;;
+                  (setq expected-bytes
+                        (and (search-forward-regexp
+                              "\\(?:.*: .*\r\n\\)*Content-Length: \
+*\\([[:digit:]]+\\)\r\n\\(?:.*: .*\r\n\\)*\r\n"
+                              (+ (point) 100)
+                              t)
+                             (string-to-number (match-string 1))))
+                  (unless expected-bytes
+                    (setq done :waiting-for-new-message)))
+                 (t
+                  ;; Attempt to complete a message body
+                  ;;
+                  (let ((available-bytes (- (position-bytes (process-mark proc))
+                                            (position-bytes (point)))))
+                    (cond
+                     ((>= available-bytes
+                          expected-bytes)
+                      (let* ((message-end (byte-to-position
+                                           (+ (position-bytes (point))
+                                              expected-bytes))))
+                        (unwind-protect
+                            (save-restriction
+                              (narrow-to-region (point) message-end)
+                              (let* ((json-message
+                                      (condition-case-unless-debug oops
+                                          (jsonrpc--json-read)
+                                        (error
+                                         (jsonrpc--warn "Invalid JSON: %s %s"
+                                                        (cdr oops) (buffer-string))
+                                         nil))))
+                                (when json-message
+                                  ;; Process content in another
+                                  ;; buffer, shielding proc buffer from
+                                  ;; tamper
+                                  (with-temp-buffer
+                                    (jsonrpc-connection-receive connection
+                                                                json-message)))))
+                          (goto-char message-end)
+                          (delete-region (point-min) (point))
+                          (setq expected-bytes nil))))
+                     (t
+                      ;; Message is still incomplete
+                      ;;
+                      (setq done :waiting-for-more-bytes-in-this-message))))))))
+          ;; Saved parsing state for next visit to this filter
+          ;;
+          (setf (jsonrpc--expected-bytes connection) expected-bytes))))))
+
+(cl-defun jsonrpc--async-request-1 (connection
+                                    method
+                                    params
+                                    &rest args
+                                    &key success-fn error-fn timeout-fn
+                                    (timeout jrpc-default-request-timeout)
+                                    (deferred nil))
+  "Does actual work for `jsonrpc-async-request'.
+
+Return a list (ID TIMER). ID is the new request's ID, or nil if
+the request was deferred. TIMER is a timer object set (or nil, if
+TIMEOUT is nil)."
+  (pcase-let* ((buf (current-buffer)) (point (point))
+               (`(,_ ,timer ,old-id)
+                (and deferred (gethash (list deferred buf)
+                                       (jsonrpc--deferred-actions connection))))
+               (id (or old-id (cl-incf (jsonrpc--next-request-id connection))))
+               (make-timer
+                (lambda ( )
+                  (when timeout
+                    (run-with-timer
+                     timeout nil
+                     (lambda ()
+                       (remhash id (jsonrpc--request-continuations connection))
+                       (remhash (list deferred buf)
+                                (jsonrpc--deferred-actions connection))
+                       (if timeout-fn (funcall timeout-fn)
+                         (jsonrpc--debug
+                          connection `(:timed-out ,method :id ,id
+                                                  :params ,params)))))))))
+    (when deferred
+      (if (jsonrpc-connection-ready-p connection deferred)
+          ;; Server is ready, we jump below and send it immediately.
+          (remhash (list deferred buf) (jsonrpc--deferred-actions connection))
+        ;; Otherwise, save in `eglot--deferred-actions' and exit non-locally
+        (unless old-id
+          (jsonrpc--debug connection `(:deferring ,method :id ,id :params
+                                                  ,params)))
+        (puthash (list deferred buf)
+                 (list (lambda ()
+                         (when (buffer-live-p buf)
+                           (with-current-buffer buf
+                             (save-excursion (goto-char point)
+                                             (apply #'jsonrpc-async-request
+                                                    connection
+                                                    method params args)))))
+                       (or timer (setq timer (funcall make-timer))) id)
+                 (jsonrpc--deferred-actions connection))
+        (cl-return-from jsonrpc--async-request-1 (list id timer))))
+    ;; Really send it
+    ;;
+    (jsonrpc-connection-send connection
+                             :id id
+                             :method method
+                             :params params)
+    (puthash id
+             (list (or success-fn
+                       (jsonrpc-lambda (&rest _ignored)
+                         (jsonrpc--debug
+                          connection (list :message "success ignored"
+                                           :id id))))
+                   (or error-fn
+                       (jsonrpc-lambda (&key code message &allow-other-keys)
+                         (jsonrpc--debug
+                          connection (list
+                                      :message
+                                      (format "error ignored, status set (%s)"
+                                              message)
+                                      :id id :error code))))
+                   (setq timer (funcall make-timer)))
+             (jsonrpc--request-continuations connection))
+    (list id timer)))
+
+(defun jsonrpc--message (format &rest args)
+  "Message out with FORMAT with ARGS."
+  (message "[jsonrpc] %s" (apply #'format format args)))
+
+(defun jsonrpc--debug (server format &rest args)
+  "Debug message for SERVER with FORMAT and ARGS."
+  (jsonrpc--log-event
+   server (if (stringp format)`(:message ,(format format args)) format)))
+
+(defun jsonrpc--warn (format &rest args)
+  "Warning message with FORMAT and ARGS."
+  (apply #'jsonrpc--message (concat "(warning) " format) args)
+  (let ((warning-minimum-level :error))
+    (display-warning 'jsonrpc
+                     (apply #'format format args)
+                     :warning)))
+
+(defun jsonrpc--log-event (connection message &optional type)
+  "Log a JSONRPC-related event.
+CONNECTION is the current connection.  MESSAGE is a JSON-like
+plist.  TYPE is a symbol saying if this is a client or server
+originated."
+  (with-current-buffer (jsonrpc-events-buffer connection)
+    (cl-destructuring-bind (&key method id error &allow-other-keys) message
+      (let* ((inhibit-read-only t)
+             (subtype (cond ((and method id)       'request)
+                            (method                'notification)
+                            (id                    'reply)
+                            (t                     'message)))
+             (type
+              (concat (format "%s" (or type 'internal))
+                      (if type
+                          (format "-%s" subtype)))))
+        (goto-char (point-max))
+        (let ((msg (format "%s%s%s %s:\n%s\n"
+                           type
+                           (if id (format " (id:%s)" id) "")
+                           (if error " ERROR" "")
+                           (current-time-string)
+                           (pp-to-string message))))
+          (when error
+            (setq msg (propertize msg 'face 'error)))
+          (insert-before-markers msg))))))
+
+(provide 'jsonrpc)
+;;; jsonrpc.el ends here
diff --git a/test/lisp/jsonrpc-tests.el b/test/lisp/jsonrpc-tests.el
new file mode 100644
index 0000000000..bfdb513ada
--- /dev/null
+++ b/test/lisp/jsonrpc-tests.el
@@ -0,0 +1,242 @@
+;;; jsonrpc-tests.el --- tests for jsonrpc.el        -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2018 Free Software Foundation, Inc.
+
+;; Author: João Távora <joaotavora@gmail.com>
+;; Maintainer: João Távora <joaotavora@gmail.com>
+;; Keywords: tests
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; About "deferred" tests, `jsonrpc--test-client' has a flag that we
+;; test this flag in the this `jsonrpc-connection-ready-p' API method.
+;; It holds any `jsonrpc-request's and `jsonrpc-async-request's
+;; explicitly passed `:deferred'.  After clearing the flag, the held
+;; requests are actually sent to the server in the next opportunity
+;; (when receiving or sending something to the server).
+
+;;; Code:
+
+(require 'ert)
+(require 'jsonrpc)
+(require 'eieio)
+
+(defclass jsonrpc--test-endpoint (jsonrpc-process-connection)
+  ((scp :accessor jsonrpc--shutdown-complete-p)))
+
+(defclass jsonrpc--test-client (jsonrpc--test-endpoint)
+  ((hold-deferred :initform t :accessor jsonrpc--hold-deferred)))
+
+(cl-defmacro jsonrpc--with-emacsrpc-fixture ((endpoint-sym) &body body)
+  (declare (indent 1) (debug t))
+  (let ((server (gensym "server-")) (listen-server (gensym "listen-server-")))
+    `(let* (,server
+            (,listen-server
+             (make-network-process
+              :name "Emacs RPC server" :server t :host "localhost"
+              :service 0
+              :log (lambda (_server client _message)
+                     (setq ,server
+                           (make-instance
+                            'jsonrpc--test-endpoint
+                            :name (process-name client)
+                            :process client
+                            :request-dispatcher
+                            (lambda (_endpoint method params)
+                              (unless (memq method '(+ - * / vconcat append
+                                                       sit-for ignore))
+                                (signal 'jsonrpc-error
+                                        `((jsonrpc-error-message
+                                           . "Sorry, this isn't allowed")
+                                          (jsonrpc-error-code . -32601))))
+                              (apply method (append params nil)))
+                            :on-shutdown
+                            (lambda (conn)
+                              (setf (jsonrpc--shutdown-complete-p conn) t)))))))
+            (,endpoint-sym (make-instance
+                            'jsonrpc--test-client
+                            "Emacs RPC client"
+                            :process
+                            (open-network-stream "JSONRPC test tcp endpoint"
+                                                 nil "localhost"
+                                                 (process-contact ,listen-server
+                                                                  :service))
+                            :on-shutdown
+                            (lambda (conn)
+                              (setf (jsonrpc--shutdown-complete-p conn) t)))))
+       (unwind-protect
+           (progn
+             (cl-assert ,endpoint-sym)
+             ,@body
+             (kill-buffer (jsonrpc--events-buffer ,endpoint-sym))
+             (when ,server
+               (kill-buffer (jsonrpc--events-buffer ,server))))
+         (unwind-protect
+             (jsonrpc-shutdown ,endpoint-sym)
+           (unwind-protect
+               (jsonrpc-shutdown ,server)
+             (cl-loop do (delete-process ,listen-server)
+                      while (progn (accept-process-output nil 0.1)
+                                   (process-live-p ,listen-server))
+                      do (jsonrpc--message
+                          "test listen-server is still running, waiting"))))))))
+
+(ert-deftest returns-3 ()
+  "A basic test for adding two numbers in our test RPC."
+  (jsonrpc--with-emacsrpc-fixture (conn)
+    (should (= 3 (jsonrpc-request conn '+ [1 2])))))
+
+(ert-deftest errors-with--32601 ()
+  "Errors with -32601"
+  (jsonrpc--with-emacsrpc-fixture (conn)
+    (condition-case err
+        (progn
+          (jsonrpc-request conn 'delete-directory "~/tmp")
+          (ert-fail "A `jsonrpc-error' should have been signalled!"))
+      (jsonrpc-error
+       (should (= -32601 (cdr (assoc 'jsonrpc-error-code (cdr err)))))))))
+
+(ert-deftest signals-an--32603-JSONRPC-error ()
+  "Signals an -32603 JSONRPC error."
+  (jsonrpc--with-emacsrpc-fixture (conn)
+    (condition-case err
+        (progn
+          (jsonrpc-request conn '+ ["a" 2])
+          (ert-fail "A `jsonrpc-error' should have been signalled!"))
+      (jsonrpc-error
+       (should (= -32603 (cdr (assoc 'jsonrpc-error-code (cdr err)))))))))
+
+(ert-deftest times-out ()
+  "Request for 3-sec sit-for with 1-sec timeout times out."
+  (jsonrpc--with-emacsrpc-fixture (conn)
+    (should-error
+     (jsonrpc-request conn 'sit-for [3] :timeout 1))))
+
+(ert-deftest doesnt-time-out ()
+  :tags '(:expensive-test)
+  "Request for 1-sec sit-for with 2-sec timeout succeeds."
+  (jsonrpc--with-emacsrpc-fixture (conn)
+    (jsonrpc-request conn 'sit-for [1] :timeout 2)))
+
+(ert-deftest stretching-it-but-works ()
+  "Vector of numbers or vector of vector of numbers are serialized."
+  (jsonrpc--with-emacsrpc-fixture (conn)
+    ;; (vconcat [1 2 3] [3 4 5]) => [1 2 3 3 4 5] which can be
+    ;; serialized.
+    (should (equal
+             [1 2 3 3 4 5]
+             (jsonrpc-request conn 'vconcat [[1 2 3] [3 4 5]])))))
+
+(ert-deftest json-el-cant-serialize-this ()
+  "Can't serialize a response that is half-vector/half-list."
+  (jsonrpc--with-emacsrpc-fixture (conn)
+    (should-error
+     ;; (append [1 2 3] [3 4 5]) => (1 2 3 . [3 4 5]), which can't be
+     ;; serialized
+     (jsonrpc-request conn 'append [[1 2 3] [3 4 5]]))))
+
+(cl-defmethod jsonrpc-connection-ready-p
+  ((conn jsonrpc--test-client) what)
+  (and (cl-call-next-method)
+       (or (not (string-match "deferred" what))
+           (not (jsonrpc--hold-deferred conn)))))
+
+(ert-deftest deferred-action-toolate ()
+  :tags '(:expensive-test)
+  "Deferred request fails because noone clears the flag."
+  (jsonrpc--with-emacsrpc-fixture (conn)
+    (should-error
+     (jsonrpc-request conn '+ [1 2]
+                      :deferred "deferred-testing" :timeout 0.5)
+     :type 'jsonrpc-error)
+    (should
+     (= 3 (jsonrpc-request conn '+ [1 2]
+                           :timeout 0.5)))))
+
+(ert-deftest deferred-action-intime ()
+  :tags '(:expensive-test)
+  "Deferred request barely makes it after event clears a flag."
+  ;; Send an async request, which returns immediately. However the
+  ;; success fun which sets the flag only runs after some time.
+  (jsonrpc--with-emacsrpc-fixture (conn)
+    (jsonrpc-async-request conn
+                           'sit-for [0.5]
+                           :success-fn
+                           (lambda (_result)
+                             (setf (jsonrpc--hold-deferred conn) nil)))
+    ;; Now wait for an answer to this request, which should be sent as
+    ;; soon as the previous one is answered.
+    (should
+     (= 3 (jsonrpc-request conn '+ [1 2]
+                           :deferred "deferred"
+                           :timeout 1)))))
+
+(ert-deftest deferred-action-complex-tests ()
+  :tags '(:expensive-test)
+  "Test a more complex situation with deferred requests."
+  (jsonrpc--with-emacsrpc-fixture (conn)
+    (let (n-deferred-1
+          n-deferred-2
+          second-deferred-went-through-p)
+      ;; This returns immediately
+      (jsonrpc-async-request
+       conn
+       'sit-for [0.1]
+       :success-fn
+       (lambda (_result)
+         ;; this only gets runs after the "first deferred" is stashed.
+         (setq n-deferred-1
+               (hash-table-count (jsonrpc--deferred-actions conn)))))
+      (should-error
+       ;; This stashes the request and waits. It will error because
+       ;; no-one clears the "hold deferred" flag.
+       (jsonrpc-request conn 'ignore ["first deferred"]
+                        :deferred "first deferred"
+                        :timeout 0.5)
+       :type 'jsonrpc-error)
+      ;; The error means the deferred actions stash is now empty
+      (should (zerop (hash-table-count (jsonrpc--deferred-actions conn))))
+      ;; Again, this returns immediately.
+      (jsonrpc-async-request
+       conn
+       'sit-for [0.1]
+       :success-fn
+       (lambda (_result)
+         ;; This gets run while "third deferred" below is waiting for
+         ;; a reply.  Notice that we clear the flag in time here.
+         (setq n-deferred-2 (hash-table-count (jsonrpc--deferred-actions conn)))
+         (setf (jsonrpc--hold-deferred conn) nil)))
+      ;; This again stashes a request and returns immediately.
+      (jsonrpc-async-request conn 'ignore ["second deferred"]
+                             :deferred "second deferred"
+                             :timeout 1
+                             :success-fn
+                             (lambda (_result)
+                               (setq second-deferred-went-through-p t)))
+      ;; And this also stashes a request, but waits.  Eventually the
+      ;; flag is cleared in time and both requests go through.
+      (jsonrpc-request conn 'ignore ["third deferred"]
+                       :deferred "third deferred"
+                       :timeout 1)
+      (should second-deferred-went-through-p)
+      (should (eq 1 n-deferred-1))
+      (should (eq 2 n-deferred-2))
+      (should (eq 0 (hash-table-count (jsonrpc--deferred-actions conn)))))))
+
+
+
+(provide 'jsonrpc-tests)
+;;; jsonrpc-tests.el ends here
-- 
2.17.1

Re: [PATCH] jsonrpc.el

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: emacs-devel@gnu.org,  monnier@iro.umontreal.ca,  vibhavp@gmail.com
> Date: Thu, 28 Jun 2018 13:13:40 +0100
> 
> A few weeks ago, I posted my jsonrpc.el here and had good feedback:
> 
>    http://lists.gnu.org/archive/html/emacs-devel/2018-06/msg00288.html
> 
> If noone objects, I'm going to push the attached patch to core.  Then
> I'll work on a manual and NEWS entry.

I'd prefer if you could commit the documentation changes together with
the code.  At least some minimal documentation.  I really dislike to
have significant code changes in the repository without the docs to go
with them, unless it's on a scratch/temporary branch.

Is that feasible for you?

Thanks.

Re: [PATCH] jsonrpc.el

[João Távora]

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

On Thu, Jun 28, 2018 at 2:18 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: João Távora <joaotavora@gmail.com>
> > Cc: emacs-devel@gnu.org,  monnier@iro.umontreal.ca,  vibhavp@gmail.com
> > Date: Thu, 28 Jun 2018 13:13:40 +0100
> >
> > A few weeks ago, I posted my jsonrpc.el here and had good feedback:
> >
> >    http://lists.gnu.org/archive/html/emacs-devel/2018-06/msg00288.html
> >
> > If noone objects, I'm going to push the attached patch to core.  Then
> > I'll work on a manual and NEWS entry.
>
> I'd prefer if you could commit the documentation changes together with
> the code.  At least some minimal documentation.  I really dislike to
> have significant code changes in the repository without the docs to go
> with them, unless it's on a scratch/temporary branch.
>
> Is that feasible for you?
>

Sure, no problem.  The manual is mostly already the reasonably thorough
"Commentary: " section, so I'll translate that to texi and direct readers
there.

Where should I put the manual BTW? The Elisp manual, right?
Where exactly?

-- 
João Távora

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

Re: [PATCH] jsonrpc.el

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 28 Jun 2018 14:23:00 +0100
> Cc: emacs-devel <emacs-devel@gnu.org>, Stefan Monnier <monnier@iro.umontreal.ca>, 
> 	Vibhav Pant <vibhavp@gmail.com>
> 
>  I'd prefer if you could commit the documentation changes together with
>  the code.  At least some minimal documentation.  I really dislike to
>  have significant code changes in the repository without the docs to go
>  with them, unless it's on a scratch/temporary branch.
> 
>  Is that feasible for you?
> 
> Sure, no problem.  The manual is mostly already the reasonably thorough
> "Commentary: " section, so I'll translate that to texi and direct readers there.  

Thanks.

> Where should I put the manual BTW? The Elisp manual, right? 

Yes.

> Where exactly?

Either inside "Parsing JSON" or right after it, on the same sectioning
level.  I think.