bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[Troy Brown]

The documentation for eglot-managed-mode-hook indicates that the hook
is run after Eglot has started/stopped managing a buffer.  I was
trying to use this to perform setup/teardown of functionality when
this happened, but it appears the registered hooks are never called on
shutdown.  The following is a little test that can be run in a buffer
which has LSP support, to demonstrate the problem.  My expectation is
that "Buffer not managed" is output and the my-eglot-hook-var is
changed to 'not-managed when a shutdown occurs.

--8<---------------cut here---------------start------------->8---
(defun my-eglot-test ()
  (defun my-eglot-hook ()
    (message "my-eglot-hook invoked")
    (if (eglot-managed-p)
        (progn
          (message "Buffer is managed")
          (setq-local my-eglot-hook-var 'managed))
      (message "Buffer not managed")
      (setq-local my-eglot-hook-var 'not-managed)))
  (add-hook 'eglot-managed-mode-hook #'my-eglot-hook)
  (setq-local my-eglot-hook-var 'initial)
  (cl-assert (not (eglot-managed-p)))
  (cl-assert (eq my-eglot-hook-var 'initial))
  (call-interactively #'eglot)
  (cl-assert (eglot-managed-p))
  (cl-assert (eq my-eglot-hook-var 'managed))
  (sleep-for 3) ; wait for server connection
  (call-interactively #'eglot-shutdown)
  (cl-assert (not (eglot-managed-p)))
  (cl-assert (eq my-eglot-hook-var 'not-managed)))
--8<---------------cut here---------------end--------------->8---

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[Eli Zaretskii]

> From: Troy Brown <brownts@troybrown.dev>
> Date: Wed, 15 May 2024 08:38:28 -0400
> 
> The documentation for eglot-managed-mode-hook indicates that the hook
> is run after Eglot has started/stopped managing a buffer.  I was
> trying to use this to perform setup/teardown of functionality when
> this happened, but it appears the registered hooks are never called on
> shutdown.  The following is a little test that can be run in a buffer
> which has LSP support, to demonstrate the problem.  My expectation is
> that "Buffer not managed" is output and the my-eglot-hook-var is
> changed to 'not-managed when a shutdown occurs.
> 
> --8<---------------cut here---------------start------------->8---
> (defun my-eglot-test ()
>   (defun my-eglot-hook ()
>     (message "my-eglot-hook invoked")
>     (if (eglot-managed-p)
>         (progn
>           (message "Buffer is managed")
>           (setq-local my-eglot-hook-var 'managed))
>       (message "Buffer not managed")
>       (setq-local my-eglot-hook-var 'not-managed)))
>   (add-hook 'eglot-managed-mode-hook #'my-eglot-hook)
>   (setq-local my-eglot-hook-var 'initial)
>   (cl-assert (not (eglot-managed-p)))
>   (cl-assert (eq my-eglot-hook-var 'initial))
>   (call-interactively #'eglot)
>   (cl-assert (eglot-managed-p))
>   (cl-assert (eq my-eglot-hook-var 'managed))
>   (sleep-for 3) ; wait for server connection
>   (call-interactively #'eglot-shutdown)
>   (cl-assert (not (eglot-managed-p)))
>   (cl-assert (eq my-eglot-hook-var 'not-managed)))
> --8<---------------cut here---------------end--------------->8---

João, any comments or suggestions?

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[João Távora]

On Sat, May 25, 2024 at 8:53 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Troy Brown <brownts@troybrown.dev>
> > Date: Wed, 15 May 2024 08:38:28 -0400
> >
> > The documentation for eglot-managed-mode-hook indicates that the hook
> > is run after Eglot has started/stopped managing a buffer.  I was
> > trying to use this to perform setup/teardown of functionality when
> > this happened, but it appears the registered hooks are never called on
> > shutdown.  The following is a little test that can be run in a buffer
> > which has LSP support, to demonstrate the problem.  My expectation is
> > that "Buffer not managed" is output and the my-eglot-hook-var is
> > changed to 'not-managed when a shutdown occurs.
> >
> > --8<---------------cut here---------------start------------->8---
> > (defun my-eglot-test ()
> >   (defun my-eglot-hook ()
> >     (message "my-eglot-hook invoked")
> >     (if (eglot-managed-p)
> >         (progn
> >           (message "Buffer is managed")
> >           (setq-local my-eglot-hook-var 'managed))
> >       (message "Buffer not managed")
> >       (setq-local my-eglot-hook-var 'not-managed)))
> >   (add-hook 'eglot-managed-mode-hook #'my-eglot-hook)
> >   (setq-local my-eglot-hook-var 'initial)
> >   (cl-assert (not (eglot-managed-p)))
> >   (cl-assert (eq my-eglot-hook-var 'initial))
> >   (call-interactively #'eglot)
> >   (cl-assert (eglot-managed-p))
> >   (cl-assert (eq my-eglot-hook-var 'managed))
> >   (sleep-for 3) ; wait for server connection
> >   (call-interactively #'eglot-shutdown)
> >   (cl-assert (not (eglot-managed-p)))
> >   (cl-assert (eq my-eglot-hook-var 'not-managed)))
> > --8<---------------cut here---------------end--------------->8---
>
> João, any comments or suggestions?

eglot-managed-mode-hook is an abnormal minor mode hook because there
is no eglot-managed-mode minor mode, there is only eglot--managed-mode
which is a "--" definition on purpose (that does have the normal hooks
of course).

So when e-m-m-hook was added it was made to run only on "turn on"
because that's where it was most needed.  We can try changing Eglot
to also run it on "turn off", but that is a backward incompatible
change.

Alternatively, we can add a new eglot-managed-mode-off-hook.

Whatever the decision, there is the additional question of _when_ to
run the "off".  Maybe it's simple to  decide, but at least the the
"on" e-m -m-hook is specially  designed to run after some
LSP communication has taken  place,  which is of the  reasons
eglot--managed-mode-hook wasn't  suitable.  So maybe the "off"
should run in a similarly careful symmetrical position.

Finally, Troy can probably also make use of the internal
eglot--managed-mode-hook.  It's a "--", unsupported and dangerous
in theory, in practice it should be OK.

I'm CCing Felicián and Stefan for suggestions.

João

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[Troy Brown]

On Sun, May 26, 2024 at 6:46 PM João Távora <joaotavora@gmail.com> wrote:
>
> So when e-m-m-hook was added it was made to run only on "turn on"
> because that's where it was most needed.  We can try changing Eglot
> to also run it on "turn off", but that is a backward incompatible
> change.
>

If it was never intended to run on "turn off", then the comment string
for eglot-managed-mode-hook likely should be updated as it indicates
it is run when Eglot stops managing the buffer (i.e., "... run by
Eglot after it started/stopped managing a buffer").

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[João Távora]

On Mon, May 27, 2024 at 1:29 PM Troy Brown <brownts@troybrown.dev> wrote:
>
> On Sun, May 26, 2024 at 6:46 PM João Távora <joaotavora@gmail.com> wrote:
> >
> > So when e-m-m-hook was added it was made to run only on "turn on"
> > because that's where it was most needed.  We can try changing Eglot
> > to also run it on "turn off", but that is a backward incompatible
> > change.
> >
>
> If it was never intended to run on "turn off", then the comment string
> for eglot-managed-mode-hook likely should be updated as it indicates
> it is run when Eglot stops managing the buffer (i.e., "... run by
> Eglot after it started/stopped managing a buffer").

Ah, so this is only a doc bug and you don't actually need it?  If so,
fine with me, just provide a trivial patch for that and I'm sure some
will install it ASAP.

João

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[Troy Brown]

On Mon, May 27, 2024 at 8:35 AM João Távora <joaotavora@gmail.com> wrote:
>
> Ah, so this is only a doc bug and you don't actually need it?  If so,
> fine with me, just provide a trivial patch for that and I'm sure some
> will install it ASAP.
>

I had tried to use it and found out it didn't work as expected, so I
took a different approach.  At this point, no I don't need it.  I did
however think it was a legitimate bug (not just a doc bug) and
reported it to prevent someone else from running into it in the
future.  If it's just a doc bug, that's fine, it wasn't clear to me.

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[João Távora]

On Mon, May 27, 2024 at 1:46 PM Troy Brown <brownts@troybrown.dev> wrote:
> I did
> however think it was a legitimate bug (not just a doc bug) and

Bugs are only "legitimate" when they are harming someone somewhere.
This hook has been there for a number of years, and noone has complained
that I can remember. If you have a use for the on-shutdown, then it's
a bug.  It'd help to know about this use case. If you don't have a use,
it's just a doc bug, and patches welcome.

João

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[Troy Brown]

On Mon, May 27, 2024 at 10:09 AM João Távora <joaotavora@gmail.com> wrote:
>
> Bugs are only "legitimate" when they are harming someone somewhere.
> This hook has been there for a number of years, and noone has complained
> that I can remember. If you have a use for the on-shutdown, then it's
> a bug.  It'd help to know about this use case. If you don't have a use,
> it's just a doc bug, and patches welcome.
>

The use case is that I was experimenting with updating the
buffer-local indent-region-function (and indirectly
indent-line-function) to be based on eglot-format when the buffer was
connected to the language server.  I was attempting to use the
eglot-managed-mode-hook so I could update these variables when the
Eglot buffer management changed.  Since the hook wasn't being called
on shutdown it would still attempt to call eglot-format when it was no
longer managing the buffer.  The workaround was to use a mode-specific
function for indent-region-function and then having that call
eglot-managed-p to determine if it should call eglot-format or
something else (e.g., indent-relative).

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[João Távora]

merge 70958 70835
thanks

On Mon, May 27, 2024 at 3:32 PM Troy Brown <brownts@troybrown.dev> wrote:
>
> On Mon, May 27, 2024 at 10:09 AM João Távora <joaotavora@gmail.com> wrote:
> >
> > Bugs are only "legitimate" when they are harming someone somewhere.
> > This hook has been there for a number of years, and noone has complained
> > that I can remember. If you have a use for the on-shutdown, then it's
> > a bug.  It'd help to know about this use case. If you don't have a use,
> > it's just a doc bug, and patches welcome.

Actually I was wrong.  I have been recently warned of this exact
same issue.  I thought you and that person were the same.
bug#70835, which this bug is a duplicate of (so I've merged them,
hopefully)

> The use case is that I was experimenting with updating the
> buffer-local indent-region-function (and indirectly
> indent-line-function) to be based on eglot-format when the buffer was
> connected to the language server.  I was attempting to use the
> eglot-managed-mode-hook so I could update these variables when the
> Eglot buffer management changed.  Since the hook wasn't being called
> on shutdown it would still attempt to call eglot-format when it was no
> longer managing the buffer.  The workaround was to use a mode-specific
> function for indent-region-function and then having that call
> eglot-managed-p to determine if it should call eglot-format or
> something else (e.g., indent-relative).

Anyway, to your use case.  The "off" hook would solve your problem,
but not as well as your solution.  When setting variables, there's
no clean solution to the "undo problem", unless the variable in
question is a hook.  Think:

 var is originally X
 activate minor mode foo, saves var value of X, sets to Y,
 activate minor mode bar, that also sets var, saves Y, sets to Z
 deactivate foo, sets var to X
 deactivate bar, sets var to Y
 now both modes are inactive, variable is set to Y, in error

When the variable being affected is a hook with certain rules, this
problem doesn't exist.

Anyway, it's not a problem for Eglot to solve.  So given there is
also bug#70835 requesting the same, I think we can risk just running
eglot-managed-mode-hook like so let's try this patch:

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 6896baf30ce..2fab9e7f38b 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -2059,6 +2059,7 @@ eglot--managed-mode
     (when eglot--current-flymake-report-fn
       (eglot--report-to-flymake nil)
       (setq eglot--current-flymake-report-fn nil))
+    (run-hooks 'eglot-managed-mode-hook)
     (let ((server eglot--cached-server))
       (setq eglot--cached-server nil)
       (when server

There will possibly be people complaining we broke their configs,
so this might not be the end of the story. But it's reasonable to try it
since this is how  the documentation says it _should_ work and is
consistent with the normal minor-mode hooks.

João

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[João Távora]

By the way Troy, can you show your indent-region-function based
on eglot-format? I once tried this and it didn't work very well.  With the
clangd server.  Would like to see your attempt.

João

On Mon, May 27, 2024 at 4:45 PM João Távora <joaotavora@gmail.com> wrote:
>
> merge 70958 70835
> thanks
>
> On Mon, May 27, 2024 at 3:32 PM Troy Brown <brownts@troybrown.dev> wrote:
> >
> > On Mon, May 27, 2024 at 10:09 AM João Távora <joaotavora@gmail.com> wrote:
> > >
> > > Bugs are only "legitimate" when they are harming someone somewhere.
> > > This hook has been there for a number of years, and noone has complained
> > > that I can remember. If you have a use for the on-shutdown, then it's
> > > a bug.  It'd help to know about this use case. If you don't have a use,
> > > it's just a doc bug, and patches welcome.
>
> Actually I was wrong.  I have been recently warned of this exact
> same issue.  I thought you and that person were the same.
> bug#70835, which this bug is a duplicate of (so I've merged them,
> hopefully)
>
> > The use case is that I was experimenting with updating the
> > buffer-local indent-region-function (and indirectly
> > indent-line-function) to be based on eglot-format when the buffer was
> > connected to the language server.  I was attempting to use the
> > eglot-managed-mode-hook so I could update these variables when the
> > Eglot buffer management changed.  Since the hook wasn't being called
> > on shutdown it would still attempt to call eglot-format when it was no
> > longer managing the buffer.  The workaround was to use a mode-specific
> > function for indent-region-function and then having that call
> > eglot-managed-p to determine if it should call eglot-format or
> > something else (e.g., indent-relative).
>
> Anyway, to your use case.  The "off" hook would solve your problem,
> but not as well as your solution.  When setting variables, there's
> no clean solution to the "undo problem", unless the variable in
> question is a hook.  Think:
>
>  var is originally X
>  activate minor mode foo, saves var value of X, sets to Y,
>  activate minor mode bar, that also sets var, saves Y, sets to Z
>  deactivate foo, sets var to X
>  deactivate bar, sets var to Y
>  now both modes are inactive, variable is set to Y, in error
>
> When the variable being affected is a hook with certain rules, this
> problem doesn't exist.
>
> Anyway, it's not a problem for Eglot to solve.  So given there is
> also bug#70835 requesting the same, I think we can risk just running
> eglot-managed-mode-hook like so let's try this patch:
>
> diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
> index 6896baf30ce..2fab9e7f38b 100644
> --- a/lisp/progmodes/eglot.el
> +++ b/lisp/progmodes/eglot.el
> @@ -2059,6 +2059,7 @@ eglot--managed-mode
>      (when eglot--current-flymake-report-fn
>        (eglot--report-to-flymake nil)
>        (setq eglot--current-flymake-report-fn nil))
> +    (run-hooks 'eglot-managed-mode-hook)
>      (let ((server eglot--cached-server))
>        (setq eglot--cached-server nil)
>        (when server
>
> There will possibly be people complaining we broke their configs,
> so this might not be the end of the story. But it's reasonable to try it
> since this is how  the documentation says it _should_ work and is
> consistent with the normal minor-mode hooks.
>
> João



-- 
João Távora

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[Felician Nemeth]

João Távora <joaotavora@gmail.com> writes:

> So when e-m-m-hook was added it was made to run only on "turn on"

If I remember correctly, the original intention was to use the hook when
Eglot "turns off" as well.  Since e-m-m-hook is not a normal hook, it is
not straightforward to determine in which case the hook is called, so
the docstring helps: "Use `eglot-managed-p' to determine if current
buffer is managed."

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[João Távora]

On Mon, May 27, 2024 at 5:21 PM Felician Nemeth
<felician.nemeth@gmail.com> wrote:
>
> João Távora <joaotavora@gmail.com> writes:
>
> > So when e-m-m-hook was added it was made to run only on "turn on"
>
> If I remember correctly, the original intention was to use the hook when
> Eglot "turns off" as well.

By "it was made to" I was just stating that that's how it turned
out and that's where it probably solved practical problems that
some user was facing.  IOW, I don't think there was ever a time
when it was ever run at "off" time.  But you're right the docstring
seems to indicate the intention was to replicate
eglot--managed-mode-hook. So that lends a bit more credibility
to the fix I proposed, I guess.

João

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[Felician Nemeth]

João Távora <joaotavora@gmail.com> writes:

>> > So when e-m-m-hook was added it was made to run only on "turn on"
>>
> IOW, I don't think there was ever a time when it was ever run at "off"
> time.

I don't think it matters much, but originally it did run in both cases:
https://github.com/joaotavora/eglot/commit/8d8c90d0d654d262e0ff43f45444ec6a512b8185

So the current situation can be regarded as a regression.

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[João Távora]

On Mon, May 27, 2024 at 6:35 PM Felician Nemeth
<felician.nemeth@gmail.com> wrote:
>
> João Távora <joaotavora@gmail.com> writes:
>
> >> > So when e-m-m-hook was added it was made to run only on "turn on"
> >>
> > IOW, I don't think there was ever a time when it was ever run at "off"
> > time.
>
> I don't think it matters much, but originally it did run in both cases:
> https://github.com/joaotavora/eglot/commit/8d8c90d0d654d262e0ff43f45444ec6a512b8185
>
> So the current situation can be regarded as a regression.

I think it does matter, it's one less thing to worry about if we
push that one-liner patch.  Can you comment on it, is it doing
more or less the same in your opinion?  I opted to put it before
before the server is shutdown, so that, like the on case, it has
access to a presumably fully setup server.

João

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[João Távora]

On Mon, May 27, 2024 at 10:05 PM João Távora <joaotavora@gmail.com> wrote:

> > So the current situation can be regarded as a regression.
>
> I think it does matter, it's one less thing to worry about if we
> push that one-liner patch.  Can you comment on it, is it doing
> more or less the same in your opinion?  I opted to put it before
> before the server is shutdown, so that, like the on case, it has
> access to a presumably fully setup server.

I've just pushed the one-line patch.  I hope someone can test it
and then we can probably close this bug.

João

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[Troy Brown]

On Mon, May 27, 2024 at 6:21 PM João Távora <joaotavora@gmail.com> wrote:
>
> I've just pushed the one-line patch.  I hope someone can test it
> and then we can probably close this bug.
>

I ran the original test case I provided with this change and it now passes.

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[Eli Zaretskii]

> From: Troy Brown <brownts@troybrown.dev>
> Date: Tue, 28 May 2024 09:00:47 -0400
> Cc: Felician Nemeth <felician.nemeth@gmail.com>, Eli Zaretskii <eliz@gnu.org>, 
> 	Stefan Monnier <monnier@iro.umontreal.ca>, 70958@debbugs.gnu.org
> 
> On Mon, May 27, 2024 at 6:21 PM João Távora <joaotavora@gmail.com> wrote:
> >
> > I've just pushed the one-line patch.  I hope someone can test it
> > and then we can probably close this bug.
> >
> 
> I ran the original test case I provided with this change and it now passes.

João, should this bug be closed now?

bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Sat, 1 Jun 2024 16:48:47 +0100
> 
> Yes, thanks

Thanks, done.