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:

> On 20/08/2024 12:40, João Távora wrote:
>> On Tue, Aug 20, 2024 at 3:08 AM Dmitry Gutov <dmitry@gutov.dev> wrote:
>> 
>>> So far I've tested with gopls and clangd (when writing the new tests),
>>> typescript-language-server and rust-analyzer.
>> That's pretty good.  If you could add pylsp or pyright
>> (basedpyright),
>> I'd feel even more confident.
>
> Installed pylsp with apt - the basic things I tested seem to work too.

Great.

>> I don't have time to spend examining these details, just the final
>> outcome is relevant: partial completions cannot be inserted, but
>> fragments of a completion can be completed to a fully formed
>> completion (eventually running exit-function then).
>
> And only fragments that prefix-match all completions as
> prefix. exit-function seems to function.

Also good.

> In relation to the current state, but I think I've figured it out -
> see the new revision attached.

Great.

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

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.

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

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.

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

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

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

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

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.

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.

João