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

Previous Next

Full log

View this message in rfc822 format

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: João Távora <joaotavora <at> gmail.com>
Cc: 72705 <at> debbugs.gnu.org
Subject: bug#72705: 31.0.50; eglot--dumb-tryc Filters out too much
Date: Sun, 25 Aug 2024 05:49:59 +0300

[Message part 1 (text/plain, inline)]

On 23/08/2024 13:23, João Távora wrote:

>> 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).

Perfect, thanks.

> 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.

I see what you're saying, but insofar that current completion is mostly 
working out well, adding the special logic for parens would improve a 
lot of cases, and keep others the same degree of broken (the 
email-sending ones, for sure). So it might be worth a try.

Anyway, stopping any partial completion (at first I didn't understand 
that you meant a more general notion that the partial-completion 
c-style) should be similarly easy to what the current patch does. You 
would just drop the second clause in eglot--dumb-tryc, I think. Or maybe 
both the first and the second.

> 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.

It seems we only have our hacks to help. They got pretty advanced, though.

>> 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.

Quite possibly, yes. Though reverting to the simpler behavior at that 
point would just require a 3-4 lines diff, as discussed.

>>>> 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.

Alas, this check does not succeed:

  (should (get-buffer-window "*Completions"))

This works:

  (when (buffer-live-p "*Completions*")
    (kill-buffer "*Completions*"))
  (completion-at-point)
  (should (looking-back "foo"))
  (should (looking-at "123"))
  (should (get-buffer "*Completions*"))

Is that better?

>>>> 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.

You might have an older version installed?

> 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.

Okay, see the attached patch with the added test.

It took way longer than expected. Seems pretty brittle (note how 
eglot--wait-for-rust-analyzer is defined), but should be better than 
nothing.

I also checked how VS Code behaves in such completions, since we were 
talking about what LSP's authors designed for or not. Works differently:

1) Completions are not filtered by suffix, ever.
2) Completing count_ones() does not delete the suffix ("1234" remains).

It might mean that either behavior is okay, though.

[eglot--dumb-tryc-more-checks-v3.diff (text/x-patch, attachment)]

Previous Next

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