GNU bug report logs -
#60557
[PATCH] Fix eglot prompt when connection already exists
Previous Next
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 60557 in the body.
You can then email your comments to 60557 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#60557
; Package
emacs
.
(Wed, 04 Jan 2023 15:46:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Evgeni Kolev <evgeni.d.kolev <at> gmail.com>
:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org
.
(Wed, 04 Jan 2023 15:46:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[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 <at> 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))))
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#60557
; Package
emacs
.
(Thu, 12 Jan 2023 09:36:01 GMT)
Full text and
rfc822 format available.
Message #8 received at 60557 <at> debbugs.gnu.org (full text, mbox):
> Cc: joaotavora <at> gmail.com
> From: Evgeni Kolev <evgeni.d.kolev <at> 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?
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#60557
; Package
emacs
.
(Thu, 12 Jan 2023 10:53:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 60557 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> Cc: joaotavora <at> gmail.com
>> From: Evgeni Kolev <evgeni.d.kolev <at> 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 <at> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#60557
; Package
emacs
.
(Sat, 14 Jan 2023 06:04:02 GMT)
Full text and
rfc822 format available.
Message #14 received at 60557 <at> debbugs.gnu.org (full text, mbox):
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 <at> gmail.com> wrote:
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> Cc: joaotavora <at> gmail.com
> >> From: Evgeni Kolev <evgeni.d.kolev <at> 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 <at> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#60557
; Package
emacs
.
(Sat, 14 Jan 2023 17:23:01 GMT)
Full text and
rfc822 format available.
Message #17 received at 60557 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Sat, Jan 14, 2023 at 6:03 AM Evgeni Kolev <evgeni.d.kolev <at> 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
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#60557
; Package
emacs
.
(Mon, 16 Jan 2023 11:52:03 GMT)
Full text and
rfc822 format available.
Message #20 received at 60557 <at> debbugs.gnu.org (full text, mbox):
Evgeni Kolev <evgeni.d.kolev <at> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#60557
; Package
emacs
.
(Mon, 16 Jan 2023 14:09:02 GMT)
Full text and
rfc822 format available.
Message #23 received at 60557 <at> debbugs.gnu.org (full text, mbox):
> From: João Távora <joaotavora <at> gmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 60557 <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#60557
; Package
emacs
.
(Mon, 16 Jan 2023 14:16:01 GMT)
Full text and
rfc822 format available.
Message #26 received at 60557 <at> debbugs.gnu.org (full text, mbox):
> From: João Távora <joaotavora <at> gmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 60557 <at> 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?
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#60557
; Package
emacs
.
(Mon, 16 Jan 2023 14:42:02 GMT)
Full text and
rfc822 format available.
Message #29 received at 60557 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On Mon, Jan 16, 2023 at 2:15 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > From: João Távora <joaotavora <at> gmail.com>
> > Cc: Eli Zaretskii <eliz <at> gnu.org>, 60557 <at> 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
[Message part 2 (text/html, inline)]
Reply sent
to
Eli Zaretskii <eliz <at> gnu.org>
:
You have taken responsibility.
(Mon, 16 Jan 2023 14:55:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Evgeni Kolev <evgeni.d.kolev <at> gmail.com>
:
bug acknowledged by developer.
(Mon, 16 Jan 2023 14:55:02 GMT)
Full text and
rfc822 format available.
Message #34 received at 60557-done <at> debbugs.gnu.org (full text, mbox):
> From: João Távora <joaotavora <at> gmail.com>
> Date: Mon, 16 Jan 2023 14:42:25 +0000
> Cc: evgeni.d.kolev <at> gmail.com, 60557 <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#60557
; Package
emacs
.
(Wed, 18 Jan 2023 06:00:02 GMT)
Full text and
rfc822 format available.
Message #37 received at 60557-done <at> debbugs.gnu.org (full text, mbox):
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 <at> gnu.org> wrote:
>
> > From: João Távora <joaotavora <at> gmail.com>
> > Date: Mon, 16 Jan 2023 14:42:25 +0000
> > Cc: evgeni.d.kolev <at> gmail.com, 60557 <at> 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 archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org
.
(Wed, 15 Feb 2023 12:24:09 GMT)
Full text and
rfc822 format available.
This bug report was last modified 1 year and 70 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.