Package: emacs;
Reported by: Casey Banner <kcbanner <at> gmail.com>
Date: Fri, 18 Oct 2024 00:56:02 UTC
Severity: wishlist
Tags: patch
Done: Stefan Kangas <stefankangas <at> gmail.com>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 73857 in the body.
You can then email your comments to 73857 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
bug-gnu-emacs <at> gnu.org:bug#73857; Package emacs.
(Fri, 18 Oct 2024 00:56:02 GMT) Full text and rfc822 format available.Casey Banner <kcbanner <at> gmail.com>:bug-gnu-emacs <at> gnu.org.
(Fri, 18 Oct 2024 00:56:02 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Casey Banner <kcbanner <at> gmail.com> To: bug-gnu-emacs <at> gnu.org Subject: [PATCH] * lisp/progmodes/eglot.el: add support for insertReplaceEdit Date: Thu, 17 Oct 2024 20:54:38 -0400
[Message part 1 (text/plain, inline)]
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!
Casey
In GNU Emacs 30.0.91 (build 1, x86_64-w64-mingw32) of 2024-10-17 built
on DESKTOP-EK25TL1
Repository revision: 50620e96d763bd21854a95c580b572687eebc319
Repository branch: eglot_insert_replace_edit
Windowing system distributor 'Microsoft Corp.', version 10.0.19045
System Description: Microsoft Windows 10 Pro (v10.0.2009.19045.5011)
Configured using:
'configure -prefix=/e/dev/emacs-src --without-dbus --without-pop
--with-native-compilation --with-xml2 --with-wide-int
--without-compress-install 'CFLAGS=-O2 -mtune=native -march=native
-fomit-frame-pointer -ftree-vectorize'
PKG_CONFIG_PATH=/mingw64/lib/pkgconfig:/mingw64/share/pkgconfig'
[Message part 2 (text/html, inline)]
[0001-lisp-progmodes-eglot.el-add-support-for-insertReplac.patch (application/octet-stream, attachment)]
bug-gnu-emacs <at> gnu.org:bug#73857; Package emacs.
(Fri, 18 Oct 2024 05:58:01 GMT) Full text and rfc822 format available.Message #8 received at 73857 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Casey Banner <kcbanner <at> gmail.com>, João Távora <joaotavora <at> gmail.com> Cc: 73857 <at> debbugs.gnu.org Subject: Re: bug#73857: [PATCH] * lisp/progmodes/eglot.el: add support for insertReplaceEdit Date: Fri, 18 Oct 2024 08:56:32 +0300
> From: Casey Banner <kcbanner <at> 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 <at> 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
>
bug-gnu-emacs <at> gnu.org:bug#73857; Package emacs.
(Sat, 02 Nov 2024 11:57:02 GMT) Full text and rfc822 format available.Message #11 received at 73857 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: joaotavora <at> gmail.com Cc: kcbanner <at> gmail.com, 73857 <at> debbugs.gnu.org Subject: Re: bug#73857: [PATCH] * lisp/progmodes/eglot.el: add support for insertReplaceEdit Date: Sat, 02 Nov 2024 13:56:35 +0200
Ping! João, any comments to the patch or the issue in general?
> Cc: 73857 <at> debbugs.gnu.org
> Date: Fri, 18 Oct 2024 08:56:32 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
>
> > From: Casey Banner <kcbanner <at> 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 <at> 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
> >
>
>
>
>
bug-gnu-emacs <at> gnu.org:bug#73857; Package emacs.
(Sat, 02 Nov 2024 13:22:02 GMT) Full text and rfc822 format available.Message #14 received at 73857 <at> debbugs.gnu.org (full text, mbox):
From: João Távora <joaotavora <at> gmail.com> To: Eli Zaretskii <eliz <at> gnu.org>, Dmitry Gutov <dmitry <at> gutov.dev> Cc: kcbanner <at> gmail.com, 73857 <at> debbugs.gnu.org Subject: Re: bug#73857: [PATCH] * lisp/progmodes/eglot.el: add support for insertReplaceEdit Date: Sat, 2 Nov 2024 13:22:16 +0000
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 <at> gnu.org> wrote:
>
> Ping! João, any comments to the patch or the issue in general?
>
> > Cc: 73857 <at> debbugs.gnu.org
> > Date: Fri, 18 Oct 2024 08:56:32 +0300
> > From: Eli Zaretskii <eliz <at> gnu.org>
> >
> > > From: Casey Banner <kcbanner <at> 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 <at> 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
Stefan Kangas <stefankangas <at> gmail.com>
to control <at> debbugs.gnu.org.
(Sun, 03 Nov 2024 05:54:03 GMT) Full text and rfc822 format available.bug-gnu-emacs <at> gnu.org:bug#73857; Package emacs.
(Mon, 04 Nov 2024 00:26:02 GMT) Full text and rfc822 format available.Message #19 received at 73857 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dmitry <at> gutov.dev> To: João Távora <joaotavora <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org> Cc: kcbanner <at> gmail.com, 73857 <at> debbugs.gnu.org Subject: Re: bug#73857: [PATCH] * lisp/progmodes/eglot.el: add support for insertReplaceEdit Date: Mon, 4 Nov 2024 02:24:57 +0200
Hi! On 02/11/2024 15:22, João Távora wrote: > 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. I agree that such a change should come with automated test(s). Happy to provide pointers, or look at writing that myself in a little while. Speaking of client-side issue, it might as well be. Or it is possible that our original expectations were wrong, and the protocol expected the suffix to never be replaced using the original convention. Now the next extension allows us to ask the servers to do that instead. Regarding capabilities, though, it seems the editors that support the extension, so far, all include the ability to configure which of the behaviors (insert or replace) will be used, with a way to invoke the non-default behavior using a combination like Shift+Enter. Is that something we want? Or, to put it differently, are we sure that the "insert" behavior won't be preferred at least in some contexts (only determined by the user)? The protocol doc says this: * Most editors support two different operations when accepting a completion * item. One is to insert a completion text and the other is to replace an * existing text with a completion text. Since this can usually not be * predetermined by a server it can report both ranges. Examples of configurability: * VS Code: https://github.com/microsoft/language-server-protocol/issues/1260#issuecomment-833014788 (link to a video in a comment) * Sublime: https://github.com/sublimelsp/LSP/pull/1809 * [dbaeumer about Eclipse]: https://github.com/microsoft/language-server-protocol/issues/1260#issuecomment-836775280 I'm not sure which method should be used in our completion UI to say "flip default this time", but maybe a user option would help.
bug-gnu-emacs <at> gnu.org:bug#73857; Package emacs.
(Thu, 13 Feb 2025 10:19:02 GMT) Full text and rfc822 format available.Message #22 received at 73857 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Kangas <stefankangas <at> gmail.com> To: Dmitry Gutov <dmitry <at> gutov.dev> Cc: kcbanner <at> gmail.com, Eli Zaretskii <eliz <at> gnu.org>, João Távora <joaotavora <at> gmail.com>, 73857 <at> debbugs.gnu.org Subject: Re: bug#73857: [PATCH] * lisp/progmodes/eglot.el: add support for insertReplaceEdit Date: Thu, 13 Feb 2025 02:18:16 -0800
Dmitry Gutov <dmitry <at> gutov.dev> writes: > Hi! > > On 02/11/2024 15:22, João Távora wrote: >> 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. > > I agree that such a change should come with automated test(s). Happy to provide > pointers, or look at writing that myself in a little while. > > Speaking of client-side issue, it might as well be. Or it is possible that our > original expectations were wrong, and the protocol expected the suffix to never > be replaced using the original convention. Now the next extension allows us to > ask the servers to do that instead. > > Regarding capabilities, though, it seems the editors that support the extension, > so far, all include the ability to configure which of the behaviors (insert or > replace) will be used, with a way to invoke the non-default behavior using a > combination like Shift+Enter. > > Is that something we want? Or, to put it differently, are we sure that the > "insert" behavior won't be preferred at least in some contexts (only determined > by the user)? > > The protocol doc says this: > > * Most editors support two different operations when accepting a completion > * item. One is to insert a completion text and the other is to replace an > * existing text with a completion text. Since this can usually not be > * predetermined by a server it can report both ranges. > > Examples of configurability: > > * VS Code: > https://github.com/microsoft/language-server-protocol/issues/1260#issuecomment-833014788 > (link to a video in a comment) > * Sublime: https://github.com/sublimelsp/LSP/pull/1809 > * [dbaeumer about Eclipse]: > https://github.com/microsoft/language-server-protocol/issues/1260#issuecomment-836775280 > > I'm not sure which method should be used in our completion UI to say "flip > default this time", but maybe a user option would help. Casey, I see that there were several followups to your proposed patch. Did you make any progress with it? Thanks in advance.
bug-gnu-emacs <at> gnu.org:bug#73857; Package emacs.
(Thu, 13 Feb 2025 10:22:01 GMT) Full text and rfc822 format available.Message #25 received at 73857 <at> debbugs.gnu.org (full text, mbox):
From: João Távora <joaotavora <at> gmail.com> To: Stefan Kangas <stefankangas <at> gmail.com> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, Eli Zaretskii <eliz <at> gnu.org>, 73857 <at> debbugs.gnu.org, kcbanner <at> gmail.com Subject: Re: bug#73857: [PATCH] * lisp/progmodes/eglot.el: add support for insertReplaceEdit Date: Thu, 13 Feb 2025 10:21:19 +0000
I've added a variation on Casey's patch, perhaps I forgot to send
email to close this bug:
commit 1143cf09a339d57051a4341103c9e342d8876649
Author: João Távora <joaotavora <at> gmail.com>
Date: Mon Jan 20 18:58:05 2025 +0000
Eglot: add support for insertReplaceEdit (bug#73857)
* lisp/progmodes/eglot.el (eglot-server-programs): Mention zig-ts-mode.
(eglot--lsp-interface-alist): Describe 'InsertReplaceEdit'.
(eglot-client-capabilities): Advertise 'insertReplaceSupport'.
(eglot-completion-at-point): Consider 'InsertReplaceEdit'.
(eglot--apply-text-edits): Consider 'InsertReplaceEdit'.
* test/lisp/progmodes/eglot-tests.el
(eglot-test-zig-insert-replace-completion): New test.
Special thanks to kcbanner <at> gmail.com
Added a zig test, too.
On Thu, Feb 13, 2025 at 10:18 AM Stefan Kangas <stefankangas <at> gmail.com> wrote:
>
> Dmitry Gutov <dmitry <at> gutov.dev> writes:
>
> > Hi!
> >
> > On 02/11/2024 15:22, João Távora wrote:
> >> 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.
> >
> > I agree that such a change should come with automated test(s). Happy to provide
> > pointers, or look at writing that myself in a little while.
> >
> > Speaking of client-side issue, it might as well be. Or it is possible that our
> > original expectations were wrong, and the protocol expected the suffix to never
> > be replaced using the original convention. Now the next extension allows us to
> > ask the servers to do that instead.
> >
> > Regarding capabilities, though, it seems the editors that support the extension,
> > so far, all include the ability to configure which of the behaviors (insert or
> > replace) will be used, with a way to invoke the non-default behavior using a
> > combination like Shift+Enter.
> >
> > Is that something we want? Or, to put it differently, are we sure that the
> > "insert" behavior won't be preferred at least in some contexts (only determined
> > by the user)?
> >
> > The protocol doc says this:
> >
> > * Most editors support two different operations when accepting a completion
> > * item. One is to insert a completion text and the other is to replace an
> > * existing text with a completion text. Since this can usually not be
> > * predetermined by a server it can report both ranges.
> >
> > Examples of configurability:
> >
> > * VS Code:
> > https://github.com/microsoft/language-server-protocol/issues/1260#issuecomment-833014788
> > (link to a video in a comment)
> > * Sublime: https://github.com/sublimelsp/LSP/pull/1809
> > * [dbaeumer about Eclipse]:
> > https://github.com/microsoft/language-server-protocol/issues/1260#issuecomment-836775280
> >
> > I'm not sure which method should be used in our completion UI to say "flip
> > default this time", but maybe a user option would help.
>
> Casey, I see that there were several followups to your proposed patch.
>
> Did you make any progress with it? Thanks in advance.
--
João Távora
Stefan Kangas <stefankangas <at> gmail.com>:Casey Banner <kcbanner <at> gmail.com>:Message #30 received at 73857-done <at> debbugs.gnu.org (full text, mbox):
From: Stefan Kangas <stefankangas <at> gmail.com> To: João Távora <joaotavora <at> gmail.com> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, Eli Zaretskii <eliz <at> gnu.org>, 73857-done <at> debbugs.gnu.org, kcbanner <at> gmail.com Subject: Re: bug#73857: [PATCH] * lisp/progmodes/eglot.el: add support for insertReplaceEdit Date: Thu, 13 Feb 2025 02:47:02 -0800
João Távora <joaotavora <at> gmail.com> writes: > I've added a variation on Casey's patch, perhaps I forgot to send > email to close this bug: > > commit 1143cf09a339d57051a4341103c9e342d8876649 > Author: João Távora <joaotavora <at> gmail.com> > Date: Mon Jan 20 18:58:05 2025 +0000 > > Eglot: add support for insertReplaceEdit (bug#73857) > > * lisp/progmodes/eglot.el (eglot-server-programs): Mention zig-ts-mode. > (eglot--lsp-interface-alist): Describe 'InsertReplaceEdit'. > (eglot-client-capabilities): Advertise 'insertReplaceSupport'. > (eglot-completion-at-point): Consider 'InsertReplaceEdit'. > (eglot--apply-text-edits): Consider 'InsertReplaceEdit'. > > * test/lisp/progmodes/eglot-tests.el > (eglot-test-zig-insert-replace-completion): New test. > > Special thanks to kcbanner <at> gmail.com > > Added a zig test, too. OK, thanks. I'm therefore closing this bug report now.
Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org.
(Thu, 13 Mar 2025 11:24:23 GMT) Full text and rfc822 format available.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.