bug#60557: [PATCH] Fix eglot prompt when connection already exists

[Evgeni Kolev <evgeni.d.kolev@gmail.com>]

Hi João, thank you for the detailed reply.

Could you confirm I correctly understand proposal 2.1)? Example interactions:

- M-x eglot RET ;; eglot guesses correctly the CONTACT
- live connection exists?
    - no - new connection
    - yes - disconnect + new connection

- M-x eglot /some/path RET ;; eglot can't guess so prompts the user
- live connection exists?
    - no - new connection
    - yes - disconnect + new connection

If I understand correctly, 2.1) always disconnects + reconnects.

Regarding 2.2), I don't fully understand it - could you clarify how
2.2) is different from the _user's_ perspective?

I'm also thinking about option 3) making M-x eglot able to detect a
change in CONTACT:

- M-x eglot RET
- live connection exists?
    - no - new connection
    - yes - new CONTACT provided?
        - no - reconnect to old CONTACT
        - yes - disconnect + new connection to new CONTACT

However, I couldn't find a way to detect a change in CONTACT because
it's not preserved as-is (for example '(gopls)) but changed to a
complex structure and stored to (eglot--saved-initargs server).

It's also possible to check current-prefix-arg - if nil, then M-x
eglot can deduce the CONTACT hasn't changed and can re-connect. If
non-nil, then disconnect + connect.

Still, I'm not sure 3) is worth it because the code will become
complex, but from the user's perspective nothing will change - for the
user reconnect is the same as disconnect + connect.


On Thu, Jan 12, 2023 at 12:53 PM João Távora <joaotavora@gmail.com> wrote:
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> Cc: joaotavora@gmail.com
> >> From: Evgeni Kolev <evgeni.d.kolev@gmail.com>
> >> Date: Wed, 4 Jan 2023 17:44:27 +0200
> >>
> >> [CC-ing João Távora, author of eglot]
> >>
> >> This patch fixes an incorrect behavior of eglot. To reproduce:
> >> - run M-x eglot
> >> - run M-x eglot (again)
> >> - eglot now prompts whether to reconnect to the existing LSP
> >> - enter "n" (for no)
> >> - now eglot shuts down the LSP process and reconnects - this is not
> >> correct, the expected behavior is to not reconnect
> >>
> >> The patch is below.
> >
> > João, any comments?
>
> Yes, sorry for the delay.
>
> Evgeni Kolev <evgeni.d.kolev@gmail.com> writes:
>
> > This patch fixes an incorrect behavior of eglot. To reproduce:
> > - run M-x eglot
> > - run M-x eglot (again)
> > - eglot now prompts whether to reconnect to the existing LSP
> > - enter "n" (for no)
> > - now eglot shuts down the LSP process and reconnects - this is not
> > correct, the expected behavior is to not reconnect
>
> Well you _are_ seing the expected behaviour!  To "disconnect and connect
> again" is different from "reconnect".  Thought often not much in
> practice, the former means you can use different command arguments to
> the server, like when doing C-u M-x eglot or when M-x eglot can't guess
> a command and lets you type one in.
>
> So in a way, it's like the message should be:
>
>    "Do you want to reconnect to same language server or do you want to
>     disconnect and connect again with whatever new things you've passed
>     to M-x eglot?"
>
> However, I agree 100% with you that this message is very confusing.
> I've been baffled by it before.  So we should attempt a fix.
>
> Unfortunately, your patch introduces a problem, so I don't think it
> should be merged.  If we install it, it means that a user doing:
>
>   M-x eglot RET /path/to/some/server-that-works-but/not-very-well RET
>   M-x eglot RET /this/one/is/much-better RET
>
> will never have an interactive chance to actually try the 'much-better'
> server.  For this user, it's either reconnect to 'not-very-well' or
> don't do anything at all.  And that's probably not what the user meant,
> and she has wasted the effort of typing "/this/one/is/much-better".  Do
> you understand?
>
> Going forward, we must ask: do we want to keep the "do you want to
> reconnect instead" possibility?
>
> 1) if yes, then there probably has to be a three-way query somewhere
>    offering options a) reconnect, b) disconnect-then-connect, and c) do
>    nothing.  This is a bit akward, but it is doable.  This query should
>    happen sooner than it does not, probably in 'eglot--guess-contact',
>    _before_ asking the user about any interactive arguments to 'M-x
>    eglot'
>
> 2) if no, then we are saying that an interactive 'M-x eglot' is either
>    going to tear down any current connection or do anything at all.
>    That's fine by me.  A simple reconnect is still just an 'M-x
>    eglot-reconnect' away.  If we 2.1) don't touch 'eglot--guess-contact'
>    it'll still be a bit akward to be given the possiblity to answer "no"
>    after typing in the new server path.  If we 2.2) adjust
>    'eglot--guess-contact', that quirk could be solved.
>
> I think 2.1 and 2.2 are the best solutions here, 2.2 being clearly
> better.  But 2.1 is less disruptive and easier to code than 2.2.
>
> João