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

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

[Evgeni Kolev]

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

commit f7626c070d4bd63fab1df33153ab9deaec0a3f7b (HEAD -> master)
Author: Evgeni Kolev <evgenysw@gmail.com>
Date:   Wed Jan 4 17:36:30 2023 +0200

    Fix eglot prompt when connection already exists

    When M-x eglot is executed twice, the second time it prompts whether
    to reconnect. Answering "no" was not working correctly - the existing
    LSP process was still reconnected.

    This behavior is fixed, answering "no" results in keeping the existing
    connection.

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 6d192d9b333..7ac123f3c09 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1116,10 +1116,9 @@ eglot
   (setq managed-major-mode (eglot--ensure-list managed-major-mode))
   (let* ((current-server (eglot-current-server))
          (live-p (and current-server (jsonrpc-running-p current-server))))
-    (if (and live-p
-             interactive
-             (y-or-n-p "[eglot] Live process found, reconnect instead? "))
-        (eglot-reconnect current-server interactive)
+    (if (and live-p interactive)
+        (when (y-or-n-p "[eglot] Live process found, reconnect instead? ")
+          (eglot-reconnect current-server interactive))
       (when live-p (ignore-errors (eglot-shutdown current-server)))
       (eglot--connect managed-major-mode project class contact language-id))))

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

[Eli Zaretskii]

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

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

[João Távora]

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

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

[Evgeni Kolev]

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

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

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1615 bytes --]

On Sat, Jan 14, 2023 at 6:03 AM Evgeni Kolev <evgeni.d.kolev@gmail.com>
wrote:
>
> 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

Correct.

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

Correct, according to 2.1.

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

Yes, that is correct.

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

In 2.2, the user is *first* prompted yes/no to make a new connection.
Only then is the user potentially asked to enter new contact
information.  In 2.1, this order is reversed.

If the user answers "no", the order in 2.1 does not make much
 sense because the work that the user has put in to enter a new
contact is mostly wasted.  This is why 2.2 is preferrable, even if
its implementation is probably slightly more complicated.

> 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

I think this is too complicated.

João

[-- Attachment #2: Type: text/html, Size: 2080 bytes --]

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

[João Távora]

Evgeni Kolev <evgeni.d.kolev@gmail.com> writes:

> Hi João, thank you for the detailed reply.
>
> Regarding 2.2), I don't fully understand it - could you clarify how
> 2.2) is different from the _user's_ perspective?

Hi Evgeni,

Because I think you've found an important flaw in Eglot, and in the
interest of speeding up this discussion I've just pushed a change to
progmodes/eglot.el that effects the alternative 2.2 I describe earlier.

I hope you have a chance to test it.  If you agree with the new
behaviour, this ticket can be closed.

[ Eli, I pushed this change to emacs-29 as this in effect a bugfix and
Eglot is a new package anyway.  Let me know if you'd rather I push this
to master instead (it doesn't make much of a difference in practice
since eglot.el is a GNU ELPA packge) ]

João

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

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  60557@debbugs.gnu.org
> Date: Mon, 16 Jan 2023 11:53:14 +0000
> 
> [ Eli, I pushed this change to emacs-29 as this in effect a bugfix and
> Eglot is a new package anyway.  Let me know if you'd rather I push this
> to master instead (it doesn't make much of a difference in practice
> since eglot.el is a GNU ELPA packge) ]

The release branch is OK for this kind of changes, thanks.

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

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  60557@debbugs.gnu.org
> Date: Mon, 16 Jan 2023 11:53:14 +0000
> 
> [ Eli, I pushed this change to emacs-29 as this in effect a bugfix and
> Eglot is a new package anyway.  Let me know if you'd rather I push this
> to master instead (it doesn't make much of a difference in practice
> since eglot.el is a GNU ELPA packge) ]

The release branch is OK for this kind of changes, thanks.

However, note that this change now causes

  In eglot:
  progmodes/eglot.el:1078:44: Warning: Unused lexical argument `interactive'

because the INTERACTIVE argument is now unused.  Should it be renamed
to _interactive?

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

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1159 bytes --]

On Mon, Jan 16, 2023 at 2:15 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: João Távora <joaotavora@gmail.com>
> > Cc: Eli Zaretskii <eliz@gnu.org>,  60557@debbugs.gnu.org
> > Date: Mon, 16 Jan 2023 11:53:14 +0000
> >
> > [ Eli, I pushed this change to emacs-29 as this in effect a bugfix and
> > Eglot is a new package anyway.  Let me know if you'd rather I push this
> > to master instead (it doesn't make much of a difference in practice
> > since eglot.el is a GNU ELPA packge) ]
>
> The release branch is OK for this kind of changes, thanks.
>
> However, note that this change now causes
>
>   In eglot:
>   progmodes/eglot.el:1078:44: Warning: Unused lexical argument
`interactive'
>
> because the INTERACTIVE argument is now unused.

You're right, I missed that. Sorry.

> Should it be renamed to _interactive?

Yes, that would be fine.  As would removing  the argument entirely.

M-x eglot is _not_ meant  to be called from Lisp at at all, but some time
ago I found at least one user on emacs-devel was doing so.  So removing
it may break some code (that is probably already broken anyway, but
still...).

João

[-- Attachment #2: Type: text/html, Size: 1608 bytes --]

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

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Mon, 16 Jan 2023 14:42:25 +0000
> Cc: evgeni.d.kolev@gmail.com, 60557@debbugs.gnu.org
> 
> >   In eglot:
> >   progmodes/eglot.el:1078:44: Warning: Unused lexical argument `interactive'
> >
> > because the INTERACTIVE argument is now unused. 
> 
> You're right, I missed that. Sorry.
>  
> > Should it be renamed to _interactive?
> 
> Yes, that would be fine.  As would removing  the argument entirely. 
> 
> M-x eglot is _not_ meant  to be called from Lisp at at all, but some time 
> ago I found at least one user on emacs-devel was doing so.  So removing 
> it may break some code (that is probably already broken anyway, but still...).

I renamed it and updated the doc string to that effect.

Closing.

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

[Evgeni Kolev]

Hi João, thank you for addressing this quickly!

Sorry for the delay on my side - I've been dealing with the flu the
last few days.

On Mon, Jan 16, 2023 at 4:54 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: João Távora <joaotavora@gmail.com>
> > Date: Mon, 16 Jan 2023 14:42:25 +0000
> > Cc: evgeni.d.kolev@gmail.com, 60557@debbugs.gnu.org
> >
> > >   In eglot:
> > >   progmodes/eglot.el:1078:44: Warning: Unused lexical argument `interactive'
> > >
> > > because the INTERACTIVE argument is now unused.
> >
> > You're right, I missed that. Sorry.
> >
> > > Should it be renamed to _interactive?
> >
> > Yes, that would be fine.  As would removing  the argument entirely.
> >
> > M-x eglot is _not_ meant  to be called from Lisp at at all, but some time
> > ago I found at least one user on emacs-devel was doing so.  So removing
> > it may break some code (that is probably already broken anyway, but still...).
>
> I renamed it and updated the doc string to that effect.
>
> Closing.