bug#70929: 30.0.50; eglot--apply-text-edits prevents point adjustment

bug#70929: 30.0.50; eglot--apply-text-edits prevents point adjustment

[Troy Brown]

Language Servers may use onTypeFormatting to provide indentation for a
buffer.  When this happens, the language server will indicate a
newline trigger character (in the DocumentOnTypeFormattingOptions).
In the Emacs buffer, after hitting RET, point is moved to the next
line and a textDocument/onTypeFormatting request is sent from Eglot to
the server.  The server responds back with the corresponding spacing
prefix for the line in newText of the TextEdit response.  However,
when Eglot applies the text edit to insert this spacing, via
eglot--apply-text-edits, it uses save-excursion, and this prevents the
point from being pushed to the end of the inserted spacing.  It would
seem that save-excursion should be avoided when applying text edits.
This issue has been observed with the Ada Language Server.

bug#70929: 30.0.50; eglot--apply-text-edits prevents point adjustment

[Felician Nemeth]

Troy Brown <brownts@troybrown.dev> writes:

> Language Servers may use onTypeFormatting to provide indentation for a
> buffer.  When this happens, the language server will indicate a
> newline trigger character (in the DocumentOnTypeFormattingOptions).
> In the Emacs buffer, after hitting RET, point is moved to the next
> line and a textDocument/onTypeFormatting request is sent from Eglot to
> the server.  The server responds back with the corresponding spacing
> prefix for the line in newText of the TextEdit response.  However,
> when Eglot applies the text edit to insert this spacing, via
> eglot--apply-text-edits, it uses save-excursion, and this prevents the
> point from being pushed to the end of the inserted spacing.  It would
> seem that save-excursion should be avoided when applying text edits.
> This issue has been observed with the Ada Language Server.

If I remember correctly, the LSP specification does not say where the
point should be after onTypeFormatting.  Something like this motivated
the rust-analyzer developers to introduce their own SnippetTextEdit
extension.  The upcoming LSP version is going to contain a slightly
different version of the SnippetTextEdit.

If my memories are correct, Ada Language Server should use this
SnippetTextEdit to unambiguously communicate its intent here.  (However,
Eglot does not currently supports SnippetTextEdit.)

bug#70929: 30.0.50; eglot--apply-text-edits prevents point adjustment

[Eli Zaretskii]

> From: Troy Brown <brownts@troybrown.dev>
> Date: Mon, 13 May 2024 22:15:07 -0400
> 
> Language Servers may use onTypeFormatting to provide indentation for a
> buffer.  When this happens, the language server will indicate a
> newline trigger character (in the DocumentOnTypeFormattingOptions).
> In the Emacs buffer, after hitting RET, point is moved to the next
> line and a textDocument/onTypeFormatting request is sent from Eglot to
> the server.  The server responds back with the corresponding spacing
> prefix for the line in newText of the TextEdit response.  However,
> when Eglot applies the text edit to insert this spacing, via
> eglot--apply-text-edits, it uses save-excursion, and this prevents the
> point from being pushed to the end of the inserted spacing.  It would
> seem that save-excursion should be avoided when applying text edits.
> This issue has been observed with the Ada Language Server.

Thanks.

João, any comments or suggestions?

bug#70929: 30.0.50; eglot--apply-text-edits prevents point adjustment

[João Távora]

On Tue, May 14, 2024 at 7:23 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Troy Brown <brownts@troybrown.dev>
> > Date: Mon, 13 May 2024 22:15:07 -0400
> >
> > Language Servers may use onTypeFormatting to provide indentation for a
> > buffer.  When this happens, the language server will indicate a
> > newline trigger character (in the DocumentOnTypeFormattingOptions).
> > In the Emacs buffer, after hitting RET, point is moved to the next
> > line and a textDocument/onTypeFormatting request is sent from Eglot to
> > the server.  The server responds back with the corresponding spacing
> > prefix for the line in newText of the TextEdit response.  However,
> > when Eglot applies the text edit to insert this spacing, via
> > eglot--apply-text-edits, it uses save-excursion, and this prevents the
> > point from being pushed to the end of the inserted spacing.  It would
> > seem that save-excursion should be avoided when applying text edits.
> > This issue has been observed with the Ada Language Server.

I've reproduced this on the clangd server and c++-ts-mode, but only after
turning _off_ electric indent-mode, which hides this effect.

> eglot--apply-text-edits, it uses save-excursion, and this prevents the
> point from being pushed to the end of the inserted spacing.  It would
> seem that save-excursion should be avoided when applying text edits.

Doing that naively would lead to chaos.  Edits can be supplied to arbitrary
places in the buffer.  Edits can happen in many situations, even when inserting
completions, for example.  If you circumscribe yourself to OnTypeFormatting,
even Clangd for example performs edits before the full expression that precedes
the newline.  There not even anything forcing the server to provide
whitespace-only changes.

The solution would have to be something like checking these specific
edits in this specific situation supplied by the server one by one and upon
finding one that  is whitespace only and overlaps the points make that
change a "push" change.

This is simply too complex of a change (you can take a stab at it and
provide ample testing, if you want).

The workaround of enabling electric-indent-mode or just turning off
OnTypeFormatting
via eglot-ignored-server-capabilities is much better.

bug#70929: 30.0.50; eglot--apply-text-edits prevents point adjustment

[Troy Brown]

On Tue, May 14, 2024 at 1:30 AM Felician Nemeth
<felician.nemeth@gmail.com> wrote:
>
> If I remember correctly, the LSP specification does not say where the
> point should be after onTypeFormatting.  Something like this motivated
> the rust-analyzer developers to introduce their own SnippetTextEdit
> extension.  The upcoming LSP version is going to contain a slightly
> different version of the SnippetTextEdit.
>
> If my memories are correct, Ada Language Server should use this
> SnippetTextEdit to unambiguously communicate its intent here.  (However,
> Eglot does not currently supports SnippetTextEdit.)

It was my understanding as well that they went out of their way to not
indicate point location, however you end up with editors doing
different things.  Maybe the Ada LS should take a different approach
here though, as you mention.

bug#70929: 30.0.50; eglot--apply-text-edits prevents point adjustment

[Troy Brown]

On Tue, May 14, 2024 at 5:28 AM João Távora <joaotavora@gmail.com> wrote:
>
> I've reproduced this on the clangd server and c++-ts-mode, but only after
> turning _off_ electric indent-mode, which hides this effect.

Yes, that's correct, electric-indent-mode can hide this.

> > eglot--apply-text-edits, it uses save-excursion, and this prevents the
> > point from being pushed to the end of the inserted spacing.  It would
> > seem that save-excursion should be avoided when applying text edits.
>
> Doing that naively would lead to chaos.  Edits can be supplied to arbitrary
> places in the buffer.  Edits can happen in many situations, even when inserting
> completions, for example.  If you circumscribe yourself to OnTypeFormatting,
> even Clangd for example performs edits before the full expression that precedes
> the newline.  There not even anything forcing the server to provide
> whitespace-only changes.
>

Possibly, although VSCode, which is probably considered the model LSP
client implementation, allows this to happen.  Not saying it's
necessarily correct in doing so, just another data point.

> The workaround of enabling electric-indent-mode or just turning off
> OnTypeFormatting
> via eglot-ignored-server-capabilities is much better.

I'm not sure a workaround of turning this off is desirable if you're
trying to use it for indentation.  If the mode doesn't have internal
indentation support, this will fallback to something like
indent-relative which might get you in the ballpark but won't be as
accurate as having the language server provide you with the correct
indentation.

I'll bring this to the attention of the Ada Language Server developers.

bug#70929: 30.0.50; eglot--apply-text-edits prevents point adjustment

[João Távora]

On Tue, May 14, 2024 at 1:44 PM Troy Brown <brownts@troybrown.dev> wrote:

> Possibly, although VSCode, which is probably considered the model LSP
> client implementation, allows this to happen.  Not saying it's
> necessarily correct in doing so, just another data point.

VSCode has its own mini-plugin for each language (sometimes not so
mini).  Akin to a major mode, but has somewhat less than 40 years
of work put into it.  In that aspect, it's not exactly a model of what
LSP purports to bring: a language-agnostic solutions.

So I don't model Eglot after VSCode, and have never done so. I model it after
LSP and my knowledge of Emacs.  That's not to say that I will ignore
if you show here whichever solution VSCode uses for this (if anything).

> > The workaround of enabling electric-indent-mode or just turning off
> > OnTypeFormatting
> > via eglot-ignored-server-capabilities is much better.
>
> I'm not sure a workaround of turning this off is desirable if you're
> trying to use it for indentation.  If the mode doesn't have internal
> indentation support, this will fallback to something like
> indent-relative which might get you in the ballpark but won't be as
> accurate as having the language server provide you with the correct
> indentation.

Sure, a workaround is not a solution, by definition.  But the way to
implement the solution in a LSP language-agnostic way -- which is what
eglot.el does -- is murky right now.

To gain confidence in any approach , I'd ideally first have to have
unit tests running on say, 5 servers that support OnTypeFormatting.  Code
up those tests in test/lisp/progmodes/eglot-tests.el.  Verify that the
"after the point indentation" indentation cases all fail currently for
those 5 servers and the "elsewhere changes" cases all pass.

Then, get to coding a solution.  For a successful solution all cases
should pass.

This is non-trivial work, so I'm not rushing to get started, especially
since the  electric-indent-mode workaround is pretty decent.  For some
meaning of "decent", at least :-) I find it relevant that so many users
(including me) who are using OnTypeFormatting and never noticed this
bug until today.

But if you're interested, you (or anyone) could help get it started
by surveying servers that support OnTypeFormatting, documenting how to
install these 5 servers conveniently in a GNU/LInux system (perhaps a
Docker image).  This would make headway with the essential tests.
You're even more welcome to write the tests yourself.


As to the solution, maybe it would end up being something that relies
on the status quo behaviour of the majority of servers like e.g.
knowing that the "before point" indentation edit is always the last one
(I'm just conjecturing here, no idea if don't know if this is the case).
I don't like this kind of solution.

Or maybe the solution is super-clean and is just about asking
`replace-buffer-contents` to use the equivalent of `insert-before-markers`
and proving it doesn't break anything anywhere else.

Or maybe we can scale this up to the LSP folks so they help us understand
exactly how this should work and maybe code it up in the standard, so servers
behave predictably.

João

bug#70929: 30.0.50; eglot--apply-text-edits prevents point adjustment

[Troy Brown]

On Tue, May 14, 2024 at 10:16 AM João Távora <joaotavora@gmail.com> wrote:
>
> VSCode has its own mini-plugin for each language (sometimes not so
> mini).  Akin to a major mode, but has somewhat less than 40 years
> of work put into it.  In that aspect, it's not exactly a model of what
> LSP purports to bring: a language-agnostic solutions.
>

For another datapoint, lsp-mode behaves the same as Eglot in this
regard, so Eglot is not alone in this behavior.  Oddly, the two have
very similar implementations for applying edits.

I've also filed a bug report with the Ada Language Server developers:
https://github.com/AdaCore/ada_language_server/issues/1197

bug#70929: 30.0.50; eglot--apply-text-edits prevents point adjustment

[João Távora]

On Wed, May 15, 2024 at 1:58 PM Troy Brown <brownts@troybrown.dev> wrote:
> For another datapoint, lsp-mode behaves the same as Eglot

That's good to know.

> Oddly, the two have
> very similar implementations for applying edits.

Doesn't suprise me that much. I know I wrote mine from scratch ;-) but
the design space for this performing feature isn't infinite.  I've also heard
unconfirmed reports of "inspiration" taken from Eglot, no idea, if here
or elsewhere or true at all.  But if true, I think that's great too.

João

bug#70929: 30.0.50; eglot--apply-text-edits prevents point adjustment

[Troy Brown]

On Tue, May 14, 2024 at 10:16 AM João Távora <joaotavora@gmail.com> wrote:
>
> So I don't model Eglot after VSCode, and have never done so. I model it after
> LSP and my knowledge of Emacs.  That's not to say that I will ignore
> if you show here whichever solution VSCode uses for this (if anything).
>

According to the Ada Language Server developers, clients usually use a
minimal diff algorithm for applying edits which allows the cursor to
be put at the correct location.  Apparently, this is what VSCode and
GNATstudio both do.  According to them, this is an issue in the LSP
client, not in the server.

See the issue response here:
https://github.com/AdaCore/ada_language_server/issues/1197