WIP: pcase macro

[mkcms]

Previously we were strict about the structure and contents of LSP
objects. This led to errors when certain servers sent messages with
fields not defined in the specification (because those fields are
used by other clients, or the protocol evolved and added some fields
and we never upgraded).

This commit makes eglot tolerate unknown keys in all incoming
messages.

• eglot.el (eglot--destructuring-bind): New macro.
  (eglot-handle-notification):
  (eglot--register-unregister):
  (eglot--xref-make):
  (xref-backend-apropos):
  (eglot-completion-at-point):
  (eglot--sig-info):
  (eglot-help-at-point):
  (eglot--apply-workspace-edit):
  (eglot-code-actions):
  (eglot--register-workspace/didChangeWatchedFiles): Use it instead of
  cl-destructuring-bind.

[mkcms]

@joaotavora So is this the right approach? Do we want to always allow other keys? I believe that we should - accept all but use the minimum required.

You mentioned here that we could add eglot--dbind which works like cl-destructuring-bind but accepts a type argument. I don't really understand this. Would it be a symbol whose value is a type specification? This seems like a lot of work to maintain a list of all LSP types in eglot and IMO reduces 'locality' (when reading the code, you need to jump around the file to know what the keys are)

[joaotavora]

@joaotavora So is this the right approach? Do we want to always allow other keys? I believe that we should - accept all but use the minimum required.

We want to allow-other-keys sometimes right? We want to decide that at runtime, if possible (or, if it's very slow, at compile-time). Hiding it in a macro such as the one you wrote it's trivial to implement.

Regarding the type argument, it could be a symbol like ResponseMessage or CreateFileOptions or in general anything the specs describes as export interface. In a future more sophisticated version of that macro, we can check in runtime if the message being destructured exactly matches the format that some version of the spec defines for that type.

But in a very first version, it's OK for users of the eglot--dbind macro and eglot--lambda macro to just pass nil for that argument, meaning something like "I don't want any validation yet". But later this might come in handy. Don't worry I can provide that part.

[joaotavora]

@mkcms I've pushed a commit that doesn't yet solve the problem but implements some of my ideas. You can try out the new macros for yourself and start using them instead of jsonrpc-lambda and cl-destructuring-bind.

In the future, we might want to add more macros on top of those, a "destructuring case" comes to mind.

This should hopefully make it easier to track back to the spec from the lisp code in eglot.

[mkcms]

@joaotavora
Nice, I will test those macros later. From what I see, one thing that I think would be nice to have, is multiple bindings in one call to eglot--dbind, like in pcase. E.g. if one matching fails, try another and then signal an error if that fails.
This would for example simplify solutions to #164 (check if an object is a Command or a CodeAction).

I also wrote some of those macros for fun. I didn't really pay attention to performance or readability though, they're just a proof-of-concept. Also, options to control strict checking aren't implemented yet.

With pcase:

(pcase-defmacro lsp--Object (fields)
  `(and (pred listp) it
        ,@(mapcar (lambda (field) `(guard (plist-member it ,field)))
                 (butlast fields))
        ,@(mapcar (lambda (field) 
                    `(let ,field
                       (plist-get it ,(intern (format ":%s" field)))))
                  (car (last fields)))))

(pcase-defmacro lsp-TextEdit (symbols)
  `(lsp--Object (:range :newText ,symbols)))

(pcase-defmacro lsp-WorkspaceEdit (symbols)
  `(lsp--Object (:changes :documentChanges ,symbols)))

;; Example
(let ((response '(:range (:start (:character 1 :line 1)
                                 :end (:character 2 :line 1))
                         :newText "x")))
  (pcase response
    ((lsp-WorkspaceEdit (changes documentChanges))
     (format "WorkspaceEdit with changes %S" changes))
    ((lsp-TextEdit (newText range)) 
     (format "TextEdit with text %S" newText))))

And a custom macro:

(defmacro eglot--dbind (exp &rest clauses)
  (declare (indent 1) (debug (form &rest (sexp sexp body))))
  (let ((expval (make-symbol "eglot--dbind-expval")))
    `(let ((,expval ,exp))
       (cond ,@(mapcar (pcase-lambda (`(,type ,args . ,body))
                         (cond ((eq type 'string)
                                `((stringp ,expval) (let ((,args ,expval)) ,@body)))
                               (t
                                `((cl-every (apply-partially #'plist-member ,expval)
                                            (get ',type 'eglot--destructuring))
                                  (cl-destructuring-bind (&key ,@args) ,expval
                                    ,@body)))))
                       clauses)
             (t (error "No match"))))))

(put 'TextEdit 'eglot--destructuring '(:range :newText))
(put 'WorkspaceEdit 'eglot--destructuring '(:changes :documentChanges))

(eglot--dbind (list :range '(:start (:line 1 :character 1)
                                    (:end (:line 1 :character 5)))
                    :newText "asdf")
  (string x (message "got a string %s" x))
  (WorkspaceEdit (changes documentChanges)
                 (message "got WorkspaceEdit with changes %s" changes))
  (TextEdit (range newText)
            (message "got TextEdit with newText %s" newText)))

[joaotavora]

From what I see, one thing that I think would be nice to have, is multiple bindings in one call to eglot--dbind, like in pcase. E.g. if one matching fails, try another and then signal an error if that fails.

This is precisely what the eglot--dcase macro shall do. It can make use of eglot--dbind.

I also wrote some of those macros for fun. I didn't really pay attention to performance or readability though, they're just a proof-of-concept.

These look very good. Perhaps they can substitute my versions, since their API seems to be similar. But I don't have time now to review them now.

Anyway, to fix #144 in the short-term, I think it won't be wasted effort to replace most cl-destructuring-bind and jsonrpc-lambda with eglot--dbind and eglot--lambda. Unless you think the API of my macros isn't good for some reason.

[joaotavora]

The pcase-* macrology is definitely very good and readable. It can probably be enhanced to honor eglot-strict-mode.

[mkcms]

It can probably be enhanced to honor eglot-strict-mode.

I will try to implement that.

[mkcms]

@joaotavora
I pushed a commit which implements the pcase macro.
It's messy... please take a look if you can.
Right now, the macro honors only disallow-non-standard-keys of eglot-strict-mode by not matching lists that have extra keys. enforce-required-keys is used by default, because the required keys is how the macro knows whether the object was actually matched.
I also modified eglot--dbind to use pcase. I'm not sure if it's good.

[mkcms]

For now, I brought back the old eglot--dbind and eglot--call-with-interface, because I realised I broke the old behavior.

[joaotavora]

~This is better some in a separate branch, by the way...~ nevermind this is a separate branch

…

On Sun, Nov 25, 2018, 00:33 mkcms ***@***.*** wrote: For now, I brought back the old eglot--dbind and eglot--call-with-interface, because I realised I broke the old behavior. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#156 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXnw4TTMBBAXePBshX98cby6haG31g1ks5uyeU8gaJpZM4YoDC_> .

[leungbk]

@joaotavora @mkcms Is there still interest in this? I wrote something similar in lsp-mode at emacs-lsp/lsp-mode@6bf62bd, though I hadn't considered allowing unknown keys. I wouldn't mind helping to wrap this up, if no one has the time to work on it.

[skangas]

@leungbk Given the lack of reply in 6 months, I think it's fair to say that any work you would want to do here is very welcome. Just open a new PR with your changes. Thanks in advance!