joaotavora / eglot

A client for Language Server Protocol servers
GNU General Public License v3.0
2.25k stars 200 forks source link

eglot-completion-at-point deleting extra text beyond the completion boundaries #1339

Closed MatthewTromp closed 9 months ago

MatthewTromp commented 9 months ago
  1. Install rust-analyzer

  2. Create a new rust project. Replace main.rs with the following:

    fn main() {
    let v: usize = 1;
    1234.1234567890
    }

    (1234.1234567890 is a placeholder for some hypothetical method call on some hypothetical object)

  3. Enter either rust-ts-mode or rust-mode.

  4. M-x eglot (with rust-analyser)

  5. Place point on the 1234 line and insert v.c:

    v.c1234.1234567890
  6. With point still on c, press M-<tab> to get eglot completions. Select count-ones. The result will be:

    v.count_ones4567890

    The completion has replaced text beyond the symbol it was operating on. The expected behavior is

    v.count_ones.1234567890

    That is, the symbol currently being completed (c1234) should be replaced with the suggested completion.

Note that if you insert v., then get completions and select one, everything behaves as expected. That is, starting with

v.1234.1234567890

with point after v., press M-<tab>, then select count_ones, the result is

v.count_ones.1234567890

as expected.

I think the cause is code in :exit-function in eglot-completion-at-point which tries to delete the text added by the accepted completion, which fails when the completion actually itself net removed text rather than adding it. Also related is #160.

Emacs version: GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo version 1.18.0, Xaw scroll bars) of 2023-12-24, commit ba3d3c699e12e2b236a353aa4dbfd1937d47f080 JSONRPC dump: eglot_jsonrpc_dump.txt

joaotavora commented 9 months ago

Thanks for the complete issue report. Will read later.

On Sun, Dec 24, 2023, 18:39 Matthew Tromp @.***> wrote:

  1. Install rust-analyzer
  2. Create a new rust project. Replace main.rs with the following:

fn main() { let v: usize = 1; 1234.1234567890}

(1234.1234567890 is a placeholder for some hypothetical method call on some hypothetical object)

  1. Enter either rust-ts-mode or rust-mode.
  2. M-x eglot (with rust-analyser)
  3. Place point on the 1234 line and insert v.c:

v.c1234.1234567890

  1. With point still on c, press M- to get eglot completions. Select count-ones. The result will be:

v.count_ones4567890

The completion has replaced text beyond the symbol it was operating on. The expected behavior is

v.count_ones.1234567890

That is, the symbol currently being completed (c1234) should be replaced with the suggested completion.

Note that if you insert v., then get completions and select one, everything behaves as expected. That is, starting with

v.1234.1234567890

with point after v., press M-, then select count_ones, the result is

v.count_ones.1234567890

as expected.

I think the cause is code in :exit-function in eglot-completion-at-point which tries to delete the text added by the accepted completion, which fails when the completion actually itself net removed text rather than adding it. Also related is #160 https://github.com/joaotavora/eglot/issues/160.

Emacs version: GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo version 1.18.0, Xaw scroll bars) of 2023-12-24, commit ba3d3c699e12e2b236a353aa4dbfd1937d47f080 JSONRPC dump: eglot_jsonrpc_dump.txt https://github.com/joaotavora/eglot/files/13762585/eglot_jsonrpc_dump.txt

— Reply to this email directly, view it on GitHub https://github.com/joaotavora/eglot/issues/1339, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC6PQ2D3W5K4Z4DT2CQEETYLBZHJAVCNFSM6AAAAABBBWLHIGVHI2DSMVQWIX3LMV43ASLTON2WKOZSGA2TKMJYGU3DSMI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

joaotavora commented 9 months ago

I've reproduced the problem and fix one of many cases. This is hairy stuff, and for now we'll just have to accept that partial completions or completions inside symbols in Eglot are very quirky. I'll leave the commit message here:

commit a6ef458e3831001b0acad57cf8fa75b77a4aff3f (HEAD -> master)
Author: João Távora <joaotavora@gmail.com>
Date:   Tue Dec 26 00:31:29 2023 +0000

    Eglot: partial fix for middle-of-symbol completions

    * lisp/progmodes/eglot.el (eglot-completion-at-point): Fix
    completion reversion in :exit-function.

    In a rust-ts-mode buffer such as this main.rs file

      fn main() {
        let v: usize = 1;
        v.c<cursor-here>1234.1234567890
      }

    the server wants to edit the line to read, after C-M-i and selecting
    "count_ones"

        v.count_ones<cursor-here>.1234567890

    But it couldn't apply the edit to the correct initial state because
    that state wasn't correctly restored.  This commit fixes that.

    However, if the initial state is

        v.count_on1234.1234567890

    then completion still fails, because the 'try-completion' call in
    eglot-completion-at-point will just return complete to "count_ones"
    and Emacs doesn't consider this a completion "exit", so it'll
    completely ignore the exit function.

    I think 'try-completion' (and 'test-completion') simply can't be used
    here (for one, they obey styles, and styles are off-limits in LSP),
    but I'll leave that for another commit.

    Github-reference: https://github.com/joaotavora/eglot/issues/1339
joaotavora commented 9 months ago

The commit that I think closes this issue (as closed as it will ever get) is now pushed to Emacs.git master.

Note that the previous "insert only v." example now seems to fail, but it's not a failure. rust-analyzer doesn't want to gobble the "1234" in that case, and that can be seen clearly from the edit it provides. The fact that it did gobble it up previously was an artifact of the previous buggy implementation.

commit 4dcbf61c1518dc53061707aeff8887517e050003 (HEAD -> master)
Author: João Távora <joaotavora@gmail.com>
Date:   Tue Dec 26 07:47:29 2023 -0600

    Eglot: Make 'try-completion' less broken

    The 'try-completion' completion operation, used mostly in vanilla
    'completion-at-point' invoked with C-M-i is close to impossible to get
    right in LSP because of the arbitrary edits handled in
    ':exit-function'.

    When this operation is invoked on the table, returning the pattern
    argument unchanged somehow (TM) makes a sole completion show the
    *Completions* buffer, where selecting it will recover context
    necessary for `:exit-function' and call that function.  It doesn't
    break any other cases I know, and that's good enough for now.

    https://github.com/joaotavora/eglot/issues/1339

    * lisp/progmodes/eglot.el (eglot-completion-at-point): Return pattern
    when 'try-completion' is invoked.
joaotavora commented 9 months ago

Nope, sorry. Doesn't work. Can never work as far as I can tell. I won't try anymore, but anyone else can:

commit d376462c7183752bf44b9bd20bf5020fe7eaf75a (HEAD -> master, origin/master, origin/HEAD)
Author: João Távora <joaotavora@gmail.com>
Date:   Tue Dec 26 08:10:04 2023 -0600

    Revert "Eglot: Make 'try-completion' less broken"

    This reverts commit 4dcbf61c1518dc53061707aeff8887517e050003.

    It's not correct, breaks tests.  I declare it impossible to make C-M-i
    use of 'try-completion' behave sanely with LSP in its current state.
    YMMV.  Use a completion tooltip, like Company.