bug#72705: 31.0.50; eglot--dumb-tryc Filters out too much

["João Távora" <joaotavora@gmail.com>]

Dmitry Gutov <dmitry@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.

>> Unfortunately, if the the two ints were named "foo_bar_1" and
>> "foo_bar_2" it would also complete to foo_bar_, which is wrong in the
>> general case, since that text is not a fully formed completion.
>> exit-function cannot run here.
>
> Is it really a problem?

Yes.  Start 'clangd --completion-style=detailed' in this thingy.cpp C++
file:

   int foobar(int x, char c) {
     return x + c;
   }
    
   int foobar(int x, float f) {
     return x + f;
   }
    
   int main() {
     foob<cursor here>
   }

if you press C-M-i here this will expand to the partial 'foobar(int x, '
which doesn't make C/C++ syntactic sense in that context.  Emacs is just
inserting the LSP "label", which as the name implies, is just for
displaying, not for inserting.  The goal of these two completions is to
expand a snippet and the snippet can only be expanded through the exit
function.  Even if we complexified Eglot's capf to insert the
'insertText' instead of the 'label' (assuming it existed), we'd be
inserting a partial snippet skeleton, even more nonsensical in C/C++.

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

> It seems to me that performing *some* completion is better than always
> returning the original input, isn't it?

No, not always.  LSP just wasn't thought with the "partial completion"
scenario in mind.

> Do you have an example of a language server and a scenario where this
> becomes a problem?

The above.

>> The current state does not do this, and chooses one of the two
>> completions indiscrimately.  This is, I think, the original bug you
>> reported here.  So this is clearly a win.  To underline it and defend
>> against regresison, I suggest to also assert that the *Completions*
>> buffer popped up.
>
> Sure.

Great.

>> Though, maybe not going so far as asserting the there
>> are exactly those two completions in that order -- doing it
>> "graphically" might prove too brittle.
>
> 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.

>> +(ert-deftest eglot-test-try-completion-inside-symbol ()
>> +  "Test completion table inside symbol, with only prefix matching."
>> +  (skip-unless (executable-find "clangd"))
>> +  (eglot--with-fixture
>> +      `(("project" . (("coiso.c" .
>> +                       ,(concat
>> +                         "int foobar;"
>> +                         "int main() {foo123")))))
>> +    (with-current-buffer
>> +        (eglot--find-file-noselect "project/coiso.c")
>> +      (eglot--wait-for-clangd)
>> +      (goto-char (- (point-max) 3))
>> +      (should
>> +       (equal
>> +        '("foo123" . 3)
>> +        (completion-try-completion
>> +         "foo123"
>> +         (nth 2 (eglot-completion-at-point)) nil 3))))))
>> I don't understand the point of this last test.  Are you checking
>> that
>> nothing changes?  What are you guarding against?
>
> With the input, I'm describing the actual prefix and suffix that get
> used. It might help someone who comes later to understand this
> separate case. Indeed "nothing happens", it's just that something did
> happen with the previous version of my patch, and we're testing
> against that.

Fine.  So maybe be a little bit more explicit in the comment or
docstring about what you _don't_ want to happen.  AFAIU, you don't want
'foobar123' or 'foobar' to be the result of that completion.

> Might be more interesting to have a similar example where the prefix
> changes while the (non-emtpy) suffix stays the same, but I think we
> aren't going to support this in this c-style.
>
>> Finally, an interesting test could be the rust-analyzer one from the
>> issue 1339 issue, which starts with.
>> fn main() {
>>      let v: usize = 1;
>>      1234.1234567890
>> }
>> I've manually checked that neither the v.c1234.12345676890 nor the
>> v.count_on1234.1234567890 case has regressed.
>
> Yep, that's the one I fixed in the last revision.
>
> 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.  This scenario
wasn't working a while ago and I'd like to protect it against
regression.

João