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

[Dmitry Gutov <dmitry@gutov.dev>]

On 22/08/2024 19:59, João Távora wrote:
> 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.

Great! Bump it to 1.17.60?

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

Thanks, now I remember: it's 
https://github.com/company-mode/company-mode/issues/1412.

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.

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

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

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

Right!

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

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