GNU bug report logs - #72705
31.0.50; eglot--dumb-tryc Filters out too much

Previous Next

Full log

Message #38 received at 72705 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: 72705 <at> debbugs.gnu.org
Subject: Re: bug#72705: 31.0.50; eglot--dumb-tryc Filters out too much
Date: Fri, 23 Aug 2024 11:23:50 +0100

Dmitry Gutov <dmitry <at> gutov.dev> writes:

> On 22/08/2024 19:59, João Távora wrote:
>> Dmitry Gutov <dmitry <at> gutov.dev> writes:
>> 
>>> Like with the previous discussions about Eglot in Emacs 29, I think
>>> there will be a lot of users who just use the version of it that comes
>>> with the release, without upgrading.
>> Hopefully upgrading should be a matter of M-x eglot-upgrade-eglot.
>> But no problem on my side.  In a separate "don't merge" commit, look
>> into editing the etc/EGLOT-NEWS there which I've already bumped to
>> 1.17.30 (the version of Eglot to be bundled with Emacs 30).  I'll then
>> copy that text to master eventually and release 1.17 to GNU Elpa.
>
> Great! Bump it to 1.17.60?

No, keep it at 1.17.30.  That's the 1.17-ish version of Eglot that will
ship with Emacs 30, just as 1.12.29 is the 1.12-ish version of Eglot
that shipped with Emacs 29.

The change you're proposing will eventually also be merged to master
and be part of the 1.18 GNU Elpa release (earlier I wrote 1.17 GNU Elpa
by mistake).

> The current proposal doesn't fix it, though it probably makes things a
> bit better (simply because the "common" string is on average computed
> to be shorter). A somewhat complete solution would probably include
> per-language "stop character" map - where we could check the `probe'
> for chars such as "(" (or whatever the language uses) and shorten the
> return value accordingly if found.
>
> It would replace the currently added "string-prefix-p" check, I guess.

Sounds clever but I really doubt that would work in the general case.

I'm not getting my point across I think.  An LSP label is just a label.
Except for in very primitive uses of LSP, it's not meant to be inserted
or to be used in the computation of prefixes or commonality between
completions.  These primitives uses are deprecated as I hope the LSP
quote from the spec confirmed.

Foreign as it might seem to Emacs devs and users, LSP doesn't want any
concept of "prefix" (or suffix or commonality).  In general, two very
closely related completions in the final insertion result can have two
wildly different labels displayed to the user.  In this sense, the
"completions" popup in LSP is less like our all-too-familiar listing of
strings that can help complete the thing at point and more akin to a
specialized context menu for "things to _do_ at point".  There's not
even an LSP mandate that this thing to _do_ must include something to
insert at point.  Currently it could be just tidying up imports
elsewhere in the LSP document.  In future LSP versions, it could be just
running an arbitrary server action, or sending an email.

In conclusion, Emacs's partial completion makes no sense in LSP (except
for the aforementioned fringe/deprecated cases).  The correct fix for
this separate issue to disable partial completion: either a full
completion can be inserted or a listing of all applicable completions
should be shown.

The other "fix" is to lobby with the LSP spec people, but they're very
VSCode-inclined.  From what I've seen, even other editors which are more
popular than Emacs have very little sway there.

>>> completing to 'foo_bar_' keeps the popup active, so I just press RET
>>> to pick the first completion.
>> With the above 'foobar(int x, ' example and with bare C-M-i it's
>> even
>> worse: it's hard (if not impossible) to complete to the full completion
>> and thus get the desired snippet expansion because the common prefix of
>> both completion labels ends with a space.
>
> Yeah, it'd be great to have C-M-i stop before the paren.

No, it wouldn't.  It'd fix this particular case, but it could break in
some other future version of LSP where the LSP label isn't a prefix of
the thing that selecting that label would insert.

>>> An alternative might be to use a completion-all-completions call, to
>>> assert the full list of completions. Or some completions that are in
>>> it anyway.
>> Yes, I generally prefer "end-to-end" tests using interactive
>> interfaces.
>> So unless you're duplicating the test I think this part should be
>> skipped.  But your call.
>
> It's a different test (albeit with the same result as an existing
> one). I don't mind rewriting it in a different style, if you
> prefer. The result is kind of messy either way.

My suggestion is just test that *Completions* pops up.

>>> It's the same case as '("foo123" . 3) in the test above, isn't it?
>> I don't think so.  In a rust-analyzer hello world (which you can
>> make
>> with cargo init, if I'm not mistaken), both
>>     v.c<cursor>1234.12345676890
>>     v.count_on<cursor>1234.12345676890
>> should expand to
>>     v.count_ones()<cursor>.1234567890
>> In the first case, you'll have to manually select "count_ones" from
>> the
>> completions buffer.  The expansion and removal of the 1234 happens via
>> LSP text edits, which are enacte by the :exit-function.
>
> Okay, I am seeing a difference too: C-M-i does eat the suffix in the
> Rust example (the "1234"). But completion with Company does not :-/

I can't even get Company to appear in those situations.

But I wouldn't bother about it.  The bug report was about C-M-i
originally.  The important thing is that the LSP textEdit is run (via
exit-function) with the correct LSP document state that the server
expects.  As far as I can tell, this is happening right now, so I'd be
nice to have a test to shore it up.

> I guess the difference might come from C-M-i always going through
> try-completion, whereas the Company popup relies on all-completions.
>
> There's a similar difference when using gopls.
>
> But Clangd actually looks special (just filed separate bug#72765).
>
>> This scenario
>> wasn't working a while ago and I'd like to protect it against
>> regression.
>
> Would we have a test that launches rust-analyzer (and depends on it
> being installed, and a Cargo project initialized)?

Yes.  Look in the existing eglot-tests.el: there is already such a test:
eglot-test-rust-analyzer-watches-files. I'd just cargo-cult the lines
until the "cargo init" call.

João

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.