Rewrite nrepl callback handling

[vspinu]

This is a continuation of the conversation at #1098

[vspinu]

One idea was to drop buffer argument in handlers and provide a macro that would create a handler with current buffer and current connection stored in its closure.

The lexical approach is awkward with respect to the composition of the handlers that @cichli proposed. So, I guess, this leaves us with the dynamic approach only.

[bbatsov]

Btw, I understand why we need the connection, but why do we need the current/another buffer? Not everything acts on it, so maybe the handlers should just obtain the buffers they need. Passing some buffer to all handlers always seems wrong to me.

[vspinu]

but why do we need the current/another buffer? Not everything acts on it, so maybe the handlers should just obtain the buffers they need.

I see at least two reasons for that.

Most importantly, this is useful for "global" handlers that CIDER or the USER can insert into the chain of handlers. Most common example is caching. As with (setq cider-buffer-ns ns) in current nrepl-make-response-handler which is always called when value is present.

Secondly, if you don't bind the buffer automatically then the handler has to do that itself (all eval handlers for example). This means that you cannot simply define a callback and then store only its name in nrepl-pending-requests. You will still need to define a lambda-generator, call it in the code and then store the whole calllback lambda in the request hash table. I am pretty sure if we store the current buffer we will reduce the use of closures as callbacks almost entirely.

I think the general idea is to store a complete context of the invocation and it's rather independent whether the handler wants to use it or not. For completeness I would also include the current point in this list (a marker actually). Examples are error handlers that must invoke a jump, and would like to push mark or xref-mark before hand. It might be useful for asyncronuous completion as well. Imagine asyncronous eldoc which would inhibit message display if the point moved in the meanwhile.

A more general idea would be to have nrepl-defhandler macro which would allow to store arbitrary context. You would call it like (nrepl-defhandler my-handler ((buf (current-bufer)) (mode major-mode)) &rest body) but that basically means re-implementing closures which you can store nicely in the nrepl-pending-requests structure. Not sure it's worth the bang.

[bbatsov]

I know why it's done as it is. My problem is that there are plenty of handlers that just ignore the "attached" buffer and currently this isn't even an optional param.

I think the general idea is to store a complete context of the invocation and it's rather independent whether the handler wants to use it or not. For completeness I would also include the current point in this list (a marker actually). Examples are error handlers that must invoke a jump, and would like to push mark or xref-mark before hand. It might be useful for asyncronuous completion as well. Imagine asyncronous eldoc which would inhibit message display if the point moved in the meanwhile.

A more general idea would be to have nrepl-defhandler macro which would allow to store arbitrary context. You would call it like (nrepl-defhandler my-handler ((buf (current-bufer)) (mode major-mode)) &rest body) but that basically means re-implementing closures which you can store nicely in the nrepl-pending-requests structure. Not sure it's worth the bang.

Yeah, those are nice points. I have to spend more time thinking about this, but in the mean time - more ideas are welcome!

[vspinu]

A caller making an nREPL request will then simply construct their response handler using function composition:

(->> (cider-repl--out-response-handler buffer) (cider-repl--value-response-handler buffer) (cider--ns-response-handler) ...)

This is basically building middlware stacks each time you make a request. Seems rather cumbersome to me with no real gain. First, you have to read this backwards because the last one is executed first. From technical prospective, you store a huge lambda in the request cache. From a programer prospective it's rather inconvenient because of two competing concepts - handler and middleware that dynamically generates the handler (BTW, -handler postfix is inappropriate here. CIDER's usage of the term handler also seems wrong.).

How about a simpler linear dispatch a la capf? You have a global nrepl-handlers list which contains functions that take one argument - the response and returns a (possibly altered) response object or nil. The response object is passed in turn from one handler to the next handler. Each of these functions is called in turn till one of them returns nil. If all functions returned non-nil, the callback origiinaly reggisterd by the caller is finally called.

You can also compose handlers directly:

(nrepl-send-request request-obj 
                    (-compose 'handler1
                              'handler2
                              ....))

You can still use generators to dynamically produce closures that capture locals, but if we already capture the context (connection, buffer and point) this will be rarely needed.

[vspinu]

(BTW, -handler postfix is inappropriate here. CIDER's usage of the term handler also seems wrong.).

Never mind. It's a handler constructor, not a handler itself, so it's fine to call it something-handler. I just need more sleep.