bug#73857: [PATCH] * lisp/progmodes/eglot.el: add support for insertReplaceEdit

[João Távora]

First, the patch looks good. I haven't been able to test it, but it
looks good, very good even.

The issue seems to be valid, and I've confirmed it, but I don't
fully understand it.  Here, I find it somewhat dissatisfying that
what would  seem to be a client-side issue (i.e. a bug in Eglot) should
be "fixed" by adding a feature so as to somehow circumvent the
problem.

But another possible explanation is that the issue is not a bug in Eglot
but a bug in certain servers (such as rust-analyzer) which given
the reasonable/common absence of a capability in the client (the one
that's being proposed by Casey) utilizes Eglot's current capabilities
in a suboptimal way that triggers a problem.

In that case -- which would have to be confirmed -- fixing the server
or adding the capability to the client are two equally valid alternatives
to solve this.  We seem to have the latter in the patch.

When I find the time I will try to test this patch, unless someone
beats me to it with a convincing test (even better a convincing
automated test in eglot-tests.el).  Such a person could be
Dmitry, who has worked on Eglot completion logic before.  I've
added him to CC.

João



On Sat, Nov 2, 2024 at 11:56 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> Ping!  João, any comments to the patch or the issue in general?
>
> > Cc: 73857@debbugs.gnu.org
> > Date: Fri, 18 Oct 2024 08:56:32 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> >
> > > From: Casey Banner <kcbanner@gmail.com>
> > > Date: Thu, 17 Oct 2024 20:54:38 -0400
> > >
> > > Since 3.16, LSP supports the capability `insertReplaceSupport`. This
> > > allows textEdit to be an `InsertReplaceEdit` see:
> > > (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#insertReplaceEdit)
> > >
> > > This patch adds support for this capability, and uses the `replace`
> > > field of the `InsertReplaceEdit`. Original functionality (ie.
> > > `TextEdit`) is preserved.
> > >
> > > The benefits of this were originally discussed here:
> > > https://github.com/joaotavora/eglot/discussions/1456, but this is a 
> > > summary:
> > >
> > > Consider this file:
> > >
> > > ```
> > > const Foo = struct {
> > >     correct_name: u32,
> > > };
> > >
> > > fn example(foo: Foo) u32 {
> > >     return foo.correct_name;
> > > }
> > > ```
> > >
> > > 1. Place the cursor on 6:22 (the _ in correct_name)
> > > 2. Backspace once to delete the t
> > > 3. Receive the following LSP message: `<-- textDocument/completion[20] 
> > > {"jsonrpc":"2.0","id":20,"result":
> > > {"isIncomplete":false,"items":[{"label":"correct_name","kind":5,"detail":"u32","documentation":
> > > {"kind":"plaintext","value":""},"sortText":"2_correct_name","textEdit":{"range":{"start":
> > > {"line":5,"character":15},"end":{"line":5,"character":21}},"newText":"correct_name"}}]}}`
> > > 4. Accept the completion
> > > 5. The buffer now contains `    return foo.correct_name_name;` on line 6
> > >
> > > I expected it to replace the entire token, resulting in `    return 
> > > foo.correct_name;`.
> > >
> > > Indeed with this patch applied (and an LSP that supports the
> > > capability), the behaviour I expected is now what happens.
> > >
> > > This is the first real elisp that I've written besides configuration, so
> > > I'm not sure if this is the correct way, but it seems to work for me.
> > >
> > > Patch is attached.
> >
> > Thanks.
> >
> > João, any comments?
> >
> > > From bbf79f95636d699ccf9ba7028e6c3dce23af2378 Mon Sep 17 00:00:00 2001
> > > From: kcbanner <kcbanner@gmail.com>
> > > Date: Thu, 17 Oct 2024 00:43:32 -0400
> > > Subject: [PATCH] * lisp/progmodes/eglot.el: add support for 
> > > insertReplaceEdit
> > >
> > > ---
> > >  lisp/progmodes/eglot.el | 25 +++++++++++++++++++------
> > >  1 file changed, 19 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
> > > index 0a14146a245..8285506928f 100644
> > > --- a/lisp/progmodes/eglot.el
> > > +++ b/lisp/progmodes/eglot.el
> > > @@ -647,6 +647,7 @@ eglot--uri-path-allowed-chars
> > >                        (:detail :deprecated :children))
> > >        (TextDocumentEdit (:textDocument :edits) ())
> > >        (TextEdit (:range :newText))
> > > +      (InsertReplaceEdit (:newText :insert :replace))
> > >        (VersionedTextDocumentIdentifier (:uri :version) ())
> > >        (WorkDoneProgress (:kind) (:title :message :percentage 
> > > :cancellable))
> > >        (WorkspaceEdit () (:changes :documentChanges))
> > > @@ -959,7 +960,8 @@ eglot-client-capabilities
> > >                                                         ["documentation"
> > >                                                          "details"
> > >                                                          
> > > "additionalTextEdits"])
> > > -                                      :tagSupport (:valueSet [1]))
> > > +                                      :tagSupport (:valueSet [1])
> > > +                                      :insertReplaceSupport t)
> > >                                      :contextSupport t)
> > >               :hover              (list :dynamicRegistration :json-false
> > >                                         :contentFormat 
> > > (eglot--accepted-formats))
> > > @@ -3368,12 +3370,19 @@ eglot-completion-at-point
> > >                          ;; state, _not_ the current "foo.bar".
> > >                          (delete-region orig-pos (point))
> > >                          (insert (substring bounds-string (- orig-pos 
> > > (car bounds))))
> > > -                        (eglot--dbind ((TextEdit) range newText) textEdit
> > > -                          (pcase-let ((`(,beg . ,end)
> > > +                        (eglot--dcase textEdit
> > > +                          (((TextEdit) range newText)
> > > +                           (pcase-let ((`(,beg . ,end)
> > >                                         (eglot-range-region range)))
> > >                              (delete-region beg end)
> > >                              (goto-char beg)
> > > -                            (funcall (or snippet-fn #'insert) newText))))
> > > +                            (funcall (or snippet-fn #'insert) newText)))
> > > +                          (((InsertReplaceEdit) newText replace)
> > > +                           (pcase-let ((`(,beg . ,end)
> > > +                                        (eglot-range-region replace)))
> > > +                             (delete-region beg end)
> > > +                             (goto-char beg)
> > > +                             (funcall (or snippet-fn #'insert) 
> > > newText)))))
> > >                         (snippet-fn
> > >                          ;; A snippet should be inserted, but using plain
> > >                          ;; `insertText'.  This requires us to delete the
> > > @@ -3602,8 +3611,12 @@ eglot--apply-text-edits
> > >                            (replace-buffer-contents temp)))
> > >                        (when reporter
> > >                          (eglot--reporter-update reporter (cl-incf 
> > > done))))))))
> > > -            (mapcar (eglot--lambda ((TextEdit) range newText)
> > > -                      (cons newText (eglot-range-region range 'markers)))
> > > +            (mapcar (lambda (text-edit-or-insert-replace-edit)
> > > +                      (eglot--dcase text-edit-or-insert-replace-edit
> > > +                        (((TextEdit) range newText)
> > > +                         (cons newText (eglot-range-region range 
> > > 'markers)))
> > > +                        (((InsertReplaceEdit) newText replace)
> > > +                         (cons newText (eglot-range-region replace 
> > > 'markers)))))
> > >                      (reverse edits)))
> > >        (undo-amalgamate-change-group change-group)
> > >        (when reporter
> > > --
> > > 2.46.0.windows.1
> > >
> >
> >
> >
> >



--
João Távora