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

Previous Next

Full log

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

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

On 21/08/2024 19:52, João Távora wrote:

>> See the new test 'eglot-test-try-completion-inside-symbol' inside the
>> patch. Wasn't sure whether to make it use 'completion-try-completion'
>> or 'completion-at-point', but the former seems more explicit.
> 
> This all looks very good, especially the abundant tests.  You may push
> right now if you want.

Thanks! The next question I should ask, though, is can we get it into 
Emacs 30?

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.

> I'll just comment on those:
> 
> +(ert-deftest eglot-test-common-prefix-completion ()
> +  "Test completion appending the common prefix."
> +  (skip-unless (executable-find "clangd"))
> +  (eglot--with-fixture
> +      `(("project" . (("coiso.c" .
> 
> Eh.  You may replace "coiso.c" for the Russian equivalent of "thingy.c"
> if you want :-).  Or something more imaginative.

schtuka? The Portuguese version sounds pretty good to my ear. I'd try 
the Greek variant, but "pragma" already has a different meaning in C langs.

> +                       ,(concat "int foo_bar; int foo_bar_baz;"
> +                                "int main() {foo")))))
> +    (with-current-buffer
> +        (eglot--find-file-noselect "project/coiso.c")
> +      (eglot--wait-for-clangd)
> +      (goto-char (point-max))
> +      (completion-at-point)
> +      (should (looking-back "{foo_bar")))))
> 
> Here, we completed to foo_bar and it's a fully formed completion.  I
> understand you've checked that exit-function does run.  This is good.

I did check, though not in any of the included tests.

> 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? In my testing completing to foo_bar_ usually is 
a good thing (you have fewer chars left to type), even though nothing 
else happens.

To try a different example, if I have C functions foo_bar_1 and 
foo_bar_2, with Clang input 'foo_' does get completed to 'foo_bar_', 
without exit-function running. But to get it called, I only need to 
additionally type '1' and press C-M-i again. Or, if Company is used, 
completing to 'foo_bar_' keeps the popup active, so I just press RET to 
pick the first completion.

> It happens to "work" here because clangd supplies the "insertText" as
> well as the "textEdit" property and that "insertText" isn't meaningless.
> The LSP spec recommendation is for servers to prefer supplying textEdit.
> 
>      * The `insertText` is subject to interpretation by the client side.
>      * Some tools might not take the string literally. For example
>      * VS Code when code complete is requested in this example
>      * `con<cursor position>` and a completion item with an `insertText` of
>      * `console` is provided it will only insert `sole`. Therefore it is
>      * recommended to use `textEdit` instead since it avoids additional client
>      * side interpretation.
> 
> Anyway this is _not_ a regression in your patch: the current state also
> does this mistake.  Though, I wonder if you could fix it as well (in a
> follow-up commit, maybe).

It seems to me that performing *some* completion is better than always 
returning the original input, isn't it? Do you have an example of a 
language server and a scenario where this becomes a problem?

> +(ert-deftest eglot-test-stop-completion-on-nonprefix ()
> +  "Test completion also resulting in 'Complete, but not unique'."
> +  (skip-unless (executable-find "clangd"))
> +  (eglot--with-fixture
> +      `(("project" . (("coiso.c" .
> +                       ,(concat "int foot; int footer; int fo_obar;"
> +                                "int main() {foo")))))
> +    (with-current-buffer
> +        (eglot--find-file-noselect "project/coiso.c")
> +      (eglot--wait-for-clangd)
> +      (goto-char (point-max))
> +      (completion-at-point)
> +      (should (looking-back "foo")))))
> 
> Here, we're looking-back to "foo" here because _no_ completion took
> place.  This means a *Completions* buffer has appeared and is now
> offering foot and footer.

Right.

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

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

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

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?

> The tests for rust-analyzer require a bit more boilerplate (they require
> a "cargo" call to setup a temporary project), but other than that it's
> mostly the same.  They won't run on Hydra (no rust-analyzer there)), but
> I do run them on occasion.

Was a bit non-trivial to get it running indeed.

Previous Next

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