bug#58839: [Patch] Re: bug#58839: 29.0.50; project-kill-buffer fails when Eglot is running

[Philip Kaludercic]

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

> On Fri, Oct 28, 2022 at 1:58 PM Philip Kaludercic <philipk@posteo.net>
> wrote:
>
>> When wanting to clean up behind a project I like to use C-x p k to get
>> rid of everything I have opened related to it.  If I was using Eglot and
>> there is still an active LSP server running in the background, killing
>> the project fails with these messages:
>
> Thanks Philip.  This was discussed at
>
> https://github.com/joaotavora/eglot/discussions/822
>
> Some more information is needed:
>
> 1. The error only happens when eglot-autoshutdown has been set to t by
>    the user.
>
> 2. When it has not been set to t, then the behavior is still not
>    correct, but the user may not notice it.
>
> 3. According to Manuel Uberti, the problem also happens with CIDER, a
>    Clojure IDE for Emacs.  So it seems it is not exclusive to Eglot.
>
> The problem happens because `project-kill-buffers` uses project.el's
> sense of a project buffer, and then endeavours to kill all such buffers.
>
> So far so good, but the determination of project buffers according
> to `project-buffers` considers all buffers whose buffer-local default
> directory starts with a given root of some project.
>
> This is subtly wrong because it also considers buffers whose name starts
> with space and without buffer-file-names, so-called "hidden buffers" which
> are deemed "uninteresting" to the user (according to the Elisp manual).
> They commonly function as implementation details of other packages, such
> as Eglot (and possibly CIDER).  These buffers are not normally visible
> to the user in M-x ibuffer, switch-to-buffer, etc.
>
> In Eglot's case, the buffer whose name is " EGLOT process..." is
> created by eglot.el and then handed over to jsonrpc.el, which becomes
> responsible for it.
>
> Killing this buffer from Lisp using `kill-buffer` is incorrect because
> it contradicts Eglot's user preferences eglot-autoreconnect and
> eglot-autoshutdown:
>
> 1. If eglot-autoshutdown is t, killing the buffer from Lisp kills the
>    process and confuses the LSP shutdown logic, which is a polite
>    "teardown" conversation with the LSP server.  This is Philip's error.

> 2. If eglot-autoshutdown is nil but eglot-autoreconnect is non-nil (in
>    fact, these are the defaults), killing the buffer has the effect of
>    immediately restarting the connection, and thus re-creating the
>    buffer.  The best that can happen is that nothing was achieved
>    and only time was wasted.
>
> The fact is that the buffer in question is an internal Eglot implementation
> detail that other packages should stay clear of.
>
> In fact, I think that all hidden buffers can be considered thusly.
> They're just like `--` symbols in obarray or in other symbol's plists:
> they're visible to all Lisp packages but they are implementation details
> that shouldn't be messed with except by the owner of such details.
>
> Dmitry tells me that there was some discussion where it was determined
> that it's somehow useful in project-kill-buffers to also target buffers
> that the
> user isn't aware of.
>
> But I've not seen evidence of this usefulness.  If there is indeed some,
> I propose we come up with some convention so that it is possible for
> packages to create buffers which are "definitely hidden and private and
> not to me tinkered with".  Such a convention could be starting the
> buffer name with two spaces.
>
> Whatever the convention, currently I think that the patch after my
> signature is the correct approach to fix this bug.
>
> Thanks,
> João
>
> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index ac278edd40..4f542137a8 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -352,14 +352,18 @@ project--remote-file-names
>                  (concat remote-id file))
>                local-files))))
>
> +(defun project--buffer-uninteresting-p (buf)
> +  (and (string-prefix-p " " (buffer-name buf)) (null (buffer-file-name
> buf))))
> +
>  (cl-defgeneric project-buffers (project)
>    "Return the list of all live buffers that belong to PROJECT."
>    (let ((root (expand-file-name (file-name-as-directory (project-root
> project))))
>          bufs)
>      (dolist (buf (buffer-list))
> -      (when (string-prefix-p root (expand-file-name
> -                                   (buffer-local-value 'default-directory
> buf)))
> -        (push buf bufs)))
> +      (unless (project--buffer-uninteresting-p buf)
> +        (when (string-prefix-p root (expand-file-name
> +                                     (buffer-local-value
> 'default-directory buf)))
> +          (push buf bufs))))
>      (nreverse bufs)))
>
>  (defgroup project-vc nil
> @@ -680,11 +684,12 @@ project-buffers
>           dd
>           bufs)
>      (dolist (buf (buffer-list))
> -      (setq dd (expand-file-name (buffer-local-value 'default-directory
> buf)))
> -      (when (and (string-prefix-p root dd)
> -                 (not (cl-find-if (lambda (module) (string-prefix-p module
> dd))
> -                                  modules)))
> -        (push buf bufs)))
> +      (unless (project--buffer-uninteresting-p buf)
> +        (setq dd (expand-file-name (buffer-local-value 'default-directory
> buf)))
> +        (when (and (string-prefix-p root dd)
> +                   (not (cl-find-if (lambda (module) (string-prefix-p
> module dd))
> +                                    modules)))
> +          (push buf bufs))))
>      (nreverse bufs)))

I still don't agree that this is the right interpretation of the issue
or solution, but wouldn't it be better to add this to
`project-kill-buffer-conditions'?

The solution I would prefer is if project.el would define a sort of
project-kill-hook, that Eglot would modify and make sure that
`eglot-shutdown' is invoked before any buffer is killed.