bug#58839: 29.0.50; project-kill-buffer fails when Eglot is running

bug#58839: 29.0.50; project-kill-buffer fails when Eglot is running

[Philip Kaludercic]

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:

--8<---------------cut here---------------start------------->8---
Kill 7 buffers in ~/Source/sgo/? (y or n) y
[eglot] error sending textDocument/didClose: (error Process EGLOT (sgo/(c-mode c++-mode)) not running: hangup
)
[eglot] error sending textDocument/didClose: (error Process EGLOT (sgo/(c-mode c++-mode)) not running: hangup
)
[eglot] error sending textDocument/didClose: (error Process EGLOT (sgo/(c-mode c++-mode)) not running: hangup
)
[eglot] Asking EGLOT (sgo/(c-mode c++-mode)) politely to terminate
[jsonrpc] Server exited with status 1
down-list: Process EGLOT (sgo/(c-mode c++-mode)) not running: hangup
--8<---------------cut here---------------end--------------->8---

Running C-x p k a second time does succeed.

The issue was previously also discussed here:
https://github.com/joaotavora/eglot/discussions/822#discussioncomment-3986357


In GNU Emacs 29.0.50 (build 23, x86_64-pc-linux-gnu, GTK+ Version
 3.24.34, cairo version 1.17.6) of 2022-10-22 built on rhea
Repository revision: ab283bddb2505e767bdf08b063c648b87d71d33a
Repository branch: feature/package+vc
System Description: Fedora Linux 36 (Workstation Edition)

Configured using:
 'configure --with-pgtk --with-imagemagick'

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ IMAGEMAGICK
JPEG JSON LIBSELINUX LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER
PGTK PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS XIM
GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: ELisp/d

Minor modes in effect:
  TeX-PDF-mode: t
  rcirc-color-mode: t
  rcirc-track-minor-mode: t
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  outline-minor-mode: t
  flymake-mode: t
  yas-minor-mode: t
  flyspell-mode: t
  repeat-mode: t
  display-battery-mode: t
  display-time-mode: t
  diff-hl-flydiff-mode: t
  diff-hl-mode: t
  winner-mode: t
  windmove-mode: t
  corfu-history-mode: t
  corfu-mode: t
  vertico-multiform-mode: t
  vertico-mode: t
  electric-pair-mode: t
  shell-dirtrack-mode: t
  recentf-mode: t
  save-place-mode: t
  savehist-mode: t
  pixel-scroll-precision-mode: t
  pixel-scroll-mode: t
  xterm-mouse-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tab-bar-mode: t
  file-name-shadow-mode: t
  context-menu-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/home/philip/.config/emacs/site-lisp/flymake-proselint/flymake-proselint hides /home/philip/.config/emacs/elpa/flymake-proselint-0.3.0/flymake-proselint
/home/philip/.config/emacs/elpa/transient-0.3.7/transient hides /home/philip/Source/emacs/lisp/transient
/home/philip/.config/emacs/elpa/xref-1.5.1/xref hides /home/philip/Source/emacs/lisp/progmodes/xref

Features:
(shadow emacsbug go-mode find-file etags fileloop nroff-mode dictionary
dictionary-connection vertico-buffer loadhist embark ffap url-cache
url-http url-auth url-gw display-line-numbers vc-annotate reposition
multi-prompt two-column shortdoc rect gnus-draft ox-odt rng-loc rng-uri
rng-parse rng-match rng-dt rng-util rng-pttrn nxml-parse nxml-ns
nxml-enc xmltok nxml-util ox-latex ox-icalendar org-agenda org-refile
ox-html table ox-ascii ox-publish ox reftex-sel cursor-sensor
reftex-parse reftex-toc consult-vertico consult compat-28 magit-bookmark
bookmark em-unix em-term term ehelp em-script em-prompt em-ls em-hist
em-pred em-glob em-extpipe em-cmpl em-dirs esh-var em-basic em-banner
em-alias esh-mode eshell esh-cmd esh-ext esh-opt esh-proc esh-io esh-arg
esh-module esh-groups esh-util ange-ftp whitespace preview reporter
desktop frameset tex-fold reftex-dcr reftex-auc reftex reftex-loaddefs
reftex-vars font-latex latex latex-flymake tex-ispell tex-style tex
texmathp tex-mode latexenc mhtml-mode css-mode js sgml-mode facemenu
gnus-search eieio-opt mailalias smtpmail rcirc-color rcirc
autocrypt-message ecomplete flow-fill mm-archive sh-script smie
executable quail org-element avl-tree generator ol-eww eww url-queue
mm-url ol-rmail ol-mhe ol-irc ol-info ol-gnus nnselect ol-docview
doc-view jka-compr image-mode exif ol-bibtex ol-bbdb ol-w3m ol-doi
org-link-doi org ob ob-tangle ob-ref ob-lob ob-table ob-exp org-macro
org-footnote org-src ob-comint org-pcomplete org-list org-faces
org-entities org-version ob-emacs-lisp ob-core ob-eval org-table
oc-basic bibtex ol org-keys oc org-compat org-macs org-loaddefs cal-menu
calendar cal-loaddefs ef-bio-theme ef-autumn-theme ef-frost-theme
ef-night-theme ietf-drums-date sort smiley gnus-cite mail-extr textsec
uni-scripts idna-mapping ucs-normalize uni-confusable textsec-check qp
gnus-async gnus-bcklg gnus-ml disp-table autocrypt-gnus autocrypt
nndraft nnmh utf-7 nnfolder epa-file network-stream nsm gnus-agent
gnus-srvr gnus-score score-mode nnvirtual gnus-msg gnus-art mm-uu
mml2015 mm-view mml-smime smime gnutls dig nntp gnus-cache gnus-sum shr
pixel-fill kinsoku url-file svg dom gnus-group gnus-undo gnus-start
gnus-dbus gnus-cloud nnimap nnmail mail-source utf7 nnoo gnus-spec
gnus-int gnus-range gnus-win ef-duo-dark-theme ef-trio-dark-theme
ef-trio-light-theme ef-themes cus-theme flymake-proselint xdg apropos
tabify cus-start cl-print mule-util pulse markdown-mode color profiler
avy copyright cc-mode advice cc-fonts cc-guess cc-menus cc-cmds
cc-styles cc-align cc-engine cc-langs cc-vars cc-defs cc-bytecomp
magit-extras goto-addr face-remap magit-submodule magit-obsolete
magit-blame magit-stash magit-reflog magit-bisect magit-push magit-pull
magit-fetch magit-clone magit-remote magit-commit magit-sequence
magit-notes magit-worktree magit-tag magit-merge magit-branch
magit-reset magit-files magit-refs magit-status magit magit-repos
magit-apply magit-wip magit-log which-func edebug magit-diff smerge-mode
git-commit log-edit message yank-media puny rfc822 mml mml-sec epa
mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils
mailheader add-log magit-core magit-autorevert magit-margin
magit-transient magit-process with-editor server magit-mode transient
edmacro kmacro magit-git magit-section magit-utils crm dash dired-aux
gnus-dired autorevert char-fold misearch multi-isearch bug-reference
buffer-env compat vc-backup vc-fossil vc-hg vc-git vc-bzr vc-src vc-sccs
vc-svn vc-cvs vc-rcs eglot array filenotify jsonrpc ert debug backtrace
xref imenu find-func vertico-directory orderless vertico-flat noutline
outline so-long checkdoc flymake-proc flymake warnings
yasnippet-snippets yasnippet flyspell ispell init auth-source-pass
repeat project battery dbus xml shell-command+ thingatpt dired-x time
sendmail rfc2047 rfc2045 ietf-drums gnus nnheader gnus-util mail-utils
range mm-util mail-prsvr finder-inf diff-hl-flydiff diff diff-hl
log-view pcvs-util vc-dir ewoc vc dired dired-loaddefs vc-dispatcher
diff-mode hippie-exp winner windmove corfu-history corfu
vertico-multiform vertico elec-pair tramp-sh tramp tramp-cache
time-stamp tramp-loaddefs trampver tramp-integration cus-edit pp icons
tramp-compat shell pcomplete parse-time iso8601 time-date ls-lisp
format-spec recentf tree-widget wid-edit saveplace savehist pixel-scroll
cua-base xt-mouse modus-operandi-theme modus-themes cus-load setup
site-lisp auto-site loaddefs-gen lisp-mnt auctex-autoloads tex-site
ef-themes-autoloads xr-autoloads consult-autoloads corfu-autoloads
focus-autoloads vertico-autoloads debbugs-autoloads
rcirc-color-autoloads inspector-autoloads flymake-proselint-autoloads
geiser-guile-autoloads xref-autoloads vc-fossil-autoloads
diff-hl-autoloads crdt-autoloads embark-autoloads magit-autoloads
buffer-env-autoloads compat-autoloads geiser-chibi-autoloads geiser-impl
help-fns radix-tree geiser-custom geiser-base geiser-autoloads
slime-autoloads transient-autoloads info speedbar ezimage dframe package
let-alist cl-extra help-mode browse-url url url-proxy url-privacy
url-expand url-methods url-history url-cookie generate-lisp-file
url-domsuf url-util mailcap url-handlers url-parse auth-source eieio
eieio-core password-cache json map byte-opt bytecomp byte-compile
derived cl-seq compile easy-mmode files-x rx text-property-search comint
ansi-osc ansi-color ring cconv cl-macs gv pcase url-vars inline epg
rfc6068 epg-config subr-x cl-loaddefs cl-lib rmc iso-transl tooltip
eldoc paren electric uniquify ediff-hook vc-hooks lisp-float-type
elisp-mode mwheel term/pgtk-win pgtk-win term/common-win pgtk-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer nadvice seq
simple cl-generic indonesian philippine cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite emoji-zwj charscript charprop case-table
epa-hook jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button
loaddefs theme-loaddefs faces cus-face macroexp files window
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget keymap hashtable-print-readable backquote threads dbusbind
inotify dynamic-setting system-font-setting font-render-setting cairo
gtk pgtk multi-tty make-network-process emacs)

Memory information:
((conses 16 1976854 260727)
 (symbols 48 63144 155)
 (strings 32 275874 35428)
 (string-bytes 1 8519084)
 (vectors 16 169530)
 (vector-slots 8 3195578 164029)
 (floats 8 1123 670)
 (intervals 56 51553 1925)
 (buffers 1000 89))

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

[João Távora]

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

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

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

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.

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

[João Távora]

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

On Fri, Oct 28, 2022 at 6:28 PM Philip Kaludercic <philipk@posteo.net>
wrote:

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

I believe we have been over this.  Eglot's preference eglot-autoshutdown
and eglot-autoreconnect already control this behaviour.  You should use
those: eglot-autoshutdown does _exactly_ what you want: shut the process
down (in a controlled fashion) if all manged buffers are killed.

You may argue (in a separate issue) that eglot-autoshutdown's default should
be t.  Just like any other user preference, we can weigh the pros and cons.

That buffer is Eglot's property, just like internal symbols or global data
structures.
Just because it's a buffer, it doesn't mean you may touch it.   It is
hidden because
you may not.  What would even be the advantage here?

You can of course show a patch, but I doubt it's going to be much simpler
than
what I have proposed.  For starters, your idea increases coupling between
two packages, which is generally not good.

João

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

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

[Dmitry Gutov]

On 28.10.2022 20:28, Philip Kaludercic wrote:
> 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'?

I don't mind a new variable specifically, but 
kill-buffer-query-functions seems to serve this purpose just fine.

And no extra "coupling between two packages" will be added.

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

[Philip Kaludercic]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 28.10.2022 20:28, Philip Kaludercic wrote:
>> 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'?
>
> I don't mind a new variable specifically, but
> kill-buffer-query-functions seems to serve this purpose just fine.
>
> And no extra "coupling between two packages" will be added.

How would you imagine it being used in this case?  

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

[João Távora]

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

On Fri, Oct 28, 2022, 19:20 Philip Kaludercic <philipk@posteo.net> wrote:

> Dmitry Gutov <dgutov@yandex.ru> writes:
>
> > On 28.10.2022 20:28, Philip Kaludercic wrote:
> >> 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'?
> >
> > I don't mind a new variable specifically, but
> > kill-buffer-query-functions seems to serve this purpose just fine.
> >
> > And no extra "coupling between two packages" will be added.
>
> How would you imagine it being used in this case?


May I suggest that we jump over the imagination directly to the reality?

The patch provided lives in the latter realm.

João


>

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

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

[Dmitry Gutov]

On 28.10.2022 21:20, Philip Kaludercic wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>> On 28.10.2022 20:28, Philip Kaludercic wrote:
>>> 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'?
>> I don't mind a new variable specifically, but
>> kill-buffer-query-functions seems to serve this purpose just fine.
>>
>> And no extra "coupling between two packages" will be added.
> How would you imagine it being used in this case?

(defun eglot-before-kill-special ()
   (eglot-shutdown)
   t)

;; somewhere inside that special buffer's setup:
(add-hook 'kill-buffer-query-functions #'eglot-before-kill-special nil t)

Or use kill-buffer-hook, no need to watch the return value then.

In either case, it will also cover the scenario of the user killing the 
background buffer some other way.

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> (defun eglot-before-kill-special ()
>   (eglot-shutdown)
>   t)
>
> ;; somewhere inside that special buffer's setup:
> (add-hook 'kill-buffer-query-functions #'eglot-before-kill-special nil t)
>
> Or use kill-buffer-hook, no need to watch the return value then.

Thanks, but this is simply wrong.  Besides missing a required argument,
it doesn't respect the preference of eglot-autoshutdown: I explained
that in the second message of this thread replying to Philip.

I also explained in that message that Eglot doesn't "own" the process
buffer, jsonrpc.el does.  Clients of jsonrpc.el don't even know there's
a process buffer, they only own a handle to a jsonrpc "connection",
which may or may not use buffers underneath.  So your "somewhere
inside..." is a big problem.

> In either case, it will also cover the scenario of the user killing
> the background buffer some other way.

The "background buffer" is hidden.  Users don't see it unless they go
considerably out of their way.  Even M-x project-switch-to-buffer
somehow doesn't list it.

Let me ask you this: can you conceive to that some buffers in Emacs's
buffer list simply don't belong to _any_ project?  If you agree that
there are such cases, then it should become clear that the buffer in
question must be at the top of that list.

There are more hints that the concept of "buffer belonging to a project"
was not fully thought through, even in cases unrelated to this bug
report.

* Take the *scratch* buffer.  It has a default-directory.  Does this
  also make *scratch* belong to a project?  It doesn't make any sense to
  me that it would.  Yet it is caught by project-buffers.

* project-buffers also catches the one-time *Completions* buffers, the
  kind produced by hitting TAB after C-x p b.  If you type C-x p b
  again, it quite comically offers the stale *Completions* buffer as a
  candidate to switch to.

But back to *scratch*.  Somehow *scratch* is not killed by M-x
project-kill-buffers.  I think it's because it doesn't have a
buffer-file-name.  But then neither does the Eglot/Jsonrpc's "background
buffers"!  It seems it is being targeted merely because it uses
fundamental-mode, a most reasonable mode to use for exchanging messages
via standard streams.

I guess this means that the hack below is enough to fix the issue, but
it is also decidedly silly.

So please consider fixing this in project.el.  As Manuel pointed out,
the venerable ibuffer.el's ibuffer-kill-filter-group also kills project
buffers and handles this whole thing very well.  We should just take a
hint from it.

João

diff --git a/lisp/jsonrpc.el b/lisp/jsonrpc.el
index 90833e1c1d..2914d380e9 100644
--- a/lisp/jsonrpc.el
+++ b/lisp/jsonrpc.el
@@ -365,6 +365,9 @@ jsonrpc-process-connection
 :ON-SHUTDOWN (optional), a function of one argument, the
 connection object, called when the process dies.")
 
+(define-derived-mode jsonrpc--fundamental-mode fundamental-mode ""
+  "Make project.el's stay out of our internal buffers.")
+
 (cl-defmethod initialize-instance ((conn jsonrpc-process-connection) slots)
   (cl-call-next-method)
   (cl-destructuring-bind (&key ((:process proc)) name &allow-other-keys) slots
@@ -377,6 +380,7 @@ initialize-instance
     ;; (but maybe not a slot).
     (let ((calling-buffer (current-buffer)))
       (with-current-buffer (get-buffer-create (format "*%s stderr*" name))
+        (jsonrpc--fundamental-mode)
         (let ((inhibit-read-only t)
               (hidden-name (concat " " (buffer-name))))
           (erase-buffer)
@@ -411,6 +415,7 @@ initialize-instance
     (set-process-filter proc #'jsonrpc--process-filter)
     (set-process-sentinel proc #'jsonrpc--process-sentinel)
     (with-current-buffer (process-buffer proc)
+      (jsonrpc--fundamental-mode)
       (buffer-disable-undo)
       (set-marker (process-mark proc) (point-min))
       (let ((inhibit-read-only t))

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

[Dmitry Gutov]

On 29.10.2022 03:15, João Távora wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> (defun eglot-before-kill-special ()
>>    (eglot-shutdown)
>>    t)
>>
>> ;; somewhere inside that special buffer's setup:
>> (add-hook 'kill-buffer-query-functions #'eglot-before-kill-special nil t)
>>
>> Or use kill-buffer-hook, no need to watch the return value then.
> 
> Thanks, but this is simply wrong.  Besides missing a required argument,
> it doesn't respect the preference of eglot-autoshutdown: I explained
> that in the second message of this thread replying to Philip.

Sorry, I don't have Eglot checked out, or the time to test out the patch.

> I also explained in that message that Eglot doesn't "own" the process
> buffer, jsonrpc.el does.  Clients of jsonrpc.el don't even know there's
> a process buffer, they only own a handle to a jsonrpc "connection",
> which may or may not use buffers underneath.  So your "somewhere
> inside..." is a big problem.
> 
>> In either case, it will also cover the scenario of the user killing
>> the background buffer some other way.
> 
> The "background buffer" is hidden.  Users don't see it unless they go
> considerably out of their way.  Even M-x project-switch-to-buffer
> somehow doesn't list it.
> 
> Let me ask you this: can you conceive to that some buffers in Emacs's
> buffer list simply don't belong to _any_ project?

I suppose. But the current criterion depends on the value of 
default-directory, and that makes it a match.

> If you agree that
> there are such cases, then it should become clear that the buffer in
> question must be at the top of that list.

I'm not sure. Intuitively, I'd say that this buffer belongs to the 
project because it "services" the project. But if it were to work for 
several projects at the same time, I suppose I could say it doesn't 
belong to any particular one.

> There are more hints that the concept of "buffer belonging to a project"
> was not fully thought through, even in cases unrelated to this bug
> report.
> 
> * Take the *scratch* buffer.  It has a default-directory.  Does this
>    also make *scratch* belong to a project?  It doesn't make any sense to
>    me that it would.  Yet it is caught by project-buffers.

*scratch* is not that special - you can create similar buffers at will. 
So there are two ways of looking at that question. One can create a 
"scratch" for a project, and it will be part of that project.

If "~" (the usual value of default-directory in the original *scratch*) 
belongs to a project, then *scratch* also does.

> * project-buffers also catches the one-time *Completions* buffers, the
>    kind produced by hitting TAB after C-x p b.  If you type C-x p b
>    again, it quite comically offers the stale *Completions* buffer as a
>    candidate to switch to.

We could make an exception for that too.

> But back to *scratch*.  Somehow *scratch* is not killed by M-x
> project-kill-buffers.  I think it's because it doesn't have a
> buffer-file-name.  But then neither does the Eglot/Jsonrpc's "background
> buffers"!  It seems it is being targeted merely because it uses
> fundamental-mode, a most reasonable mode to use for exchanging messages
> via standard streams.
> 
> I guess this means that the hack below is enough to fix the issue, but
> it is also decidedly silly.

It's not much better than adding a function to 
kill-buffer-query-functions that returns nil. And/or behaves accordingly 
to eglot-autoshutdown.

> So please consider fixing this in project.el.  As Manuel pointed out,
> the venerable ibuffer.el's ibuffer-kill-filter-group also kills project
> buffers and handles this whole thing very well.  We should just take a
> hint from it.

I'm unable to find that message.

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Sorry, I don't have Eglot checked out, or the time to test out the
> patch.

If you missed it, Eglot has been merged in lisp/progmodes/eglot.el.
There's not much point in testing that your patch: I can tell you right
away it doesn't work.

> I suppose. But the current criterion depends on the value of
> default-directory, and that makes it a match.

This criterion is wrong.  It makes mistakes.  But a criterion that is
"default-directory and is not hidden", though probably still not ideal,
but is definitely better.

>> If you agree that
>> there are such cases, then it should become clear that the buffer in
>> question must be at the top of that list.
>
> I'm not sure. Intuitively, I'd say that this buffer belongs to the
> project because it "services" the project. But if it were to work for
> several projects at the same time, I suppose I could say it doesn't
> belong to any particular one.

It indeed indirectly services just that one project: but it's also just
another object.  Eglot has lots of objects, variables etc., that
"service the project" and project.el fortunately isn't crazy to to touch
them.  The buffer in question is an implementation detail of jsonrpc.el.
It's not a buffer of interest in any way for the user or project.el's
manipulations.  And it's only a buffer because buffer's are Emacs'
common way of communicating with external entities, and jsonrpc.el uses
that technique.  But it could use some other way, say another
process-filter or function calls into C code of a dynamic library.
There would also be objects that indirectly "service" the project, but
not buffers.

>> There are more hints that the concept of "buffer belonging to a project"
>> was not fully thought through, even in cases unrelated to this bug
>> report.
>> * Take the *scratch* buffer.  It has a default-directory.  Does this
>>    also make *scratch* belong to a project?  It doesn't make any sense to
>>    me that it would.  Yet it is caught by project-buffers.
>
> *scratch* is not that special - you can create similar buffers at
>  will. So there are two ways of looking at that question. One can
>  create a "scratch" for a project, and it will be part of that
> project.
>
> If "~" (the usual value of default-directory in the original
> *scratch*) belongs to a project, then *scratch* also does.

I M-x cd in *scratch* all the time.  It's a global scratch pad,
now accessible via scratch-buffer everywhere.

>> * project-buffers also catches the one-time *Completions* buffers, the
>>    kind produced by hitting TAB after C-x p b.  If you type C-x p b
>>    again, it quite comically offers the stale *Completions* buffer as a
>>    candidate to switch to.
>
> We could make an exception for that too.
>
>> But back to *scratch*.  Somehow *scratch* is not killed by M-x
>> project-kill-buffers.  I think it's because it doesn't have a
>> buffer-file-name.  But then neither does the Eglot/Jsonrpc's "background
>> buffers"!  It seems it is being targeted merely because it uses
>> fundamental-mode, a most reasonable mode to use for exchanging messages
>> via standard streams.
>> I guess this means that the hack below is enough to fix the issue,
>> but
>> it is also decidedly silly.
>
> It's not much better than adding a function to
> kill-buffer-query-functions that returns nil. And/or behaves
> accordingly to eglot-autoshutdown.

You should think your solution through before comparing with the ones
provided so far, which have been tested.  Where in the source code would
you even set kill-buffer-query-functions?  Eglot code in jsonrpc.el??
Not to mention duplicating the eglot-autoshutdown check in unrelated
places is pretty ugly.

>> So please consider fixing this in project.el.  As Manuel pointed out,
>> the venerable ibuffer.el's ibuffer-kill-filter-group also kills project
>> buffers and handles this whole thing very well.  We should just take a
>> hint from it.
>
> I'm unable to find that message.

In the original conversation: 

https://github.com/joaotavora/eglot/discussions/822#discussioncomment-2053395

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

[Philip Kaludercic]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> If you agree that
>> there are such cases, then it should become clear that the buffer in
>> question must be at the top of that list.
>
> I'm not sure. Intuitively, I'd say that this buffer belongs to the
> project because it "services" the project. 

This is my perspective too.  I am under the impression that João or
looking at this from a too technical perspective, and is missing the way
users perceive the situation.

If it is currently not possible, then this is an issue not an excuse
that we should tackle.  IMO having Eglot integrate with project.el ought
to be fine, since project is an Eglot dependency.

>                                            But if it were to work for
> several projects at the same time, I suppose I could say it doesn't
> belong to any particular one.

In that case I think a kind of "project reference counter" should ensure
that the server is only then killed when the last project using it is
killed (and the right user options are set).

(Btw., the tone of this discussion is a bit unpleasant, could we try and
focus on the issue at hand and do our best to understand each other?)

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

[Dmitry Gutov]

On 29.10.2022 09:38, Philip Kaludercic wrote:
>>                                             But if it were to work for
>> several projects at the same time, I suppose I could say it doesn't
>> belong to any particular one.
> In that case I think a kind of "project reference counter" should ensure
> that the server is only then killed when the last project using it is
> killed (and the right user options are set).

I suppose. But that kind of arrangement would require a project-specific 
hook.

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

[João Távora]

Philip Kaludercic <philipk@posteo.net> writes:

> This is my perspective too.  I am under the impression that João or
> looking at this from a too technical perspective, and is missing the way
> users perceive the situation.

You're not reading what I am writing.  Here's a summary again:

1. buffer is implementation detail.  Users don't see it any more than
   they see values of internal global variables.  In fact I think
   lsp-mode, perfectly legitimately, uses (or used to use) a process,
   but doesn't make use of its buffer at all.  It uses a special filter
   that processes and stores strings in global variables.  These
   variables also "service a project" and occupy resources but I doubt
   project.el has a claim to their ownership.  jsonrpc.el use buffers
   internally, just because that was deemed more convenient and
   efficient for parsing.

2. Users like you who are interested in controlling the RAM and CPU
   resources taken by the running a LSP server, even when Emacs is not
   visiting any files in a project should use the variable
   eglot-autoshutdown.  This variable is exactly what you want.  There
   is no other "user perception" here that I have been made aware of.

3. Even if we were to neglect point 1, as you seem to be, fixing things
   with kill-buffer-query-functions duplicates programming logic, which
   is very bad.  And most important of all, there's no place to put the
   reference to kill-buffer-query-functions short of many lines
   completely throwing away encapsulation and unique resource ownership.

If this is too technical, then I'm sorry: this is a technical mailing
list.

João

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

[João Távora]

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

On Sat, Oct 29, 2022 at 11:59 AM Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 29.10.2022 09:38, Philip Kaludercic wrote:
> >>                                             But if it were to work for
> >> several projects at the same time, I suppose I could say it doesn't
> >> belong to any particular one.
> > In that case I think a kind of "project reference counter" should ensure
> > that the server is only then killed when the last project using it is
> > killed (and the right user options are set).
>
> I suppose. But that kind of arrangement would require a project-specific
> hook.
>

There isn't any one-to-many correspondence between LSP servers
and projects. I don't envision adding that to Eglot anytime soon: it seems
completely useless. So what problem are you even solving with this
"reference counter" talk?

João

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

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

[Dmitry Gutov]

On 29.10.2022 04:39, João Távora wrote:

> If you missed it, Eglot has been merged in lisp/progmodes/eglot.el.
> There's not much point in testing that your patch: I can tell you right
> away it doesn't work.

I was thinking that somebody motivated could easily update it to be 
functional.

>> I suppose. But the current criterion depends on the value of
>> default-directory, and that makes it a match.
> 
> This criterion is wrong.  It makes mistakes.  But a criterion that is
> "default-directory and is not hidden", though probably still not ideal,
> but is definitely better.

This whole discussion is about different shades of OCD. One party wants 
to clean up as much as possible, another says don't touch my things.

I don't think there is an objective "right" way to do things, only 
something we're able to agree on in the end. I don't really use this 
feature much myself: if you're able to come to an agreement with Philip 
(who took the initiative on adding that command), I'll be happy.

>>> If you agree that
>>> there are such cases, then it should become clear that the buffer in
>>> question must be at the top of that list.
>>
>> I'm not sure. Intuitively, I'd say that this buffer belongs to the
>> project because it "services" the project. But if it were to work for
>> several projects at the same time, I suppose I could say it doesn't
>> belong to any particular one.
> 
> It indeed indirectly services just that one project: but it's also just
> another object.  Eglot has lots of objects, variables etc., that
> "service the project" and project.el fortunately isn't crazy to to touch
> them.  The buffer in question is an implementation detail of jsonrpc.el.
> It's not a buffer of interest in any way for the user or project.el's
> manipulations.  And it's only a buffer because buffer's are Emacs'
> common way of communicating with external entities, and jsonrpc.el uses
> that technique.  But it could use some other way, say another
> process-filter or function calls into C code of a dynamic library.
> There would also be objects that indirectly "service" the project, but
> not buffers.

Most object types are garbage-collected when no live reference to them 
remains. That's not the case for buffers.

Is the buffer in question killed when the user calls 'M-x 
eglot-shutdown'? If so, consider that, after the user calls 
project-kill-buffers, there won't be any buffers remaining that belong 
to that project, that the user would be able to call 'M-x 
eglot-shutdown' from.

>>> There are more hints that the concept of "buffer belonging to a project"
>>> was not fully thought through, even in cases unrelated to this bug
>>> report.
>>> * Take the *scratch* buffer.  It has a default-directory.  Does this
>>>     also make *scratch* belong to a project?  It doesn't make any sense to
>>>     me that it would.  Yet it is caught by project-buffers.
>>
>> *scratch* is not that special - you can create similar buffers at
>>   will. So there are two ways of looking at that question. One can
>>   create a "scratch" for a project, and it will be part of that
>> project.
>>
>> If "~" (the usual value of default-directory in the original
>> *scratch*) belongs to a project, then *scratch* also does.
> 
> I M-x cd in *scratch* all the time.  It's a global scratch pad,
> now accessible via scratch-buffer everywhere.

And I don't have any projects that "~" belongs to.

>>> * project-buffers also catches the one-time *Completions* buffers, the
>>>     kind produced by hitting TAB after C-x p b.  If you type C-x p b
>>>     again, it quite comically offers the stale *Completions* buffer as a
>>>     candidate to switch to.
>>
>> We could make an exception for that too.
>>
>>> But back to *scratch*.  Somehow *scratch* is not killed by M-x
>>> project-kill-buffers.  I think it's because it doesn't have a
>>> buffer-file-name.  But then neither does the Eglot/Jsonrpc's "background
>>> buffers"!  It seems it is being targeted merely because it uses
>>> fundamental-mode, a most reasonable mode to use for exchanging messages
>>> via standard streams.
>>> I guess this means that the hack below is enough to fix the issue,
>>> but
>>> it is also decidedly silly.
>>
>> It's not much better than adding a function to
>> kill-buffer-query-functions that returns nil. And/or behaves
>> accordingly to eglot-autoshutdown.
> 
> You should think your solution through before comparing with the ones
> provided so far, which have been tested.  Where in the source code would
> you even set kill-buffer-query-functions?  Eglot code in jsonrpc.el??

Same place you changed the major mode in the last patch: jsonrpc.el. If 
jsonrpc.el doesn't want its buffers to be killed, it could set that up 
as described above, through kill-buffer-query-functions.

> Not to mention duplicating the eglot-autoshutdown check in unrelated
> places is pretty ugly.
> 
>>> So please consider fixing this in project.el.  As Manuel pointed out,
>>> the venerable ibuffer.el's ibuffer-kill-filter-group also kills project
>>> buffers and handles this whole thing very well.  We should just take a
>>> hint from it.
>>
>> I'm unable to find that message.
> 
> In the original conversation:
> 
> https://github.com/joaotavora/eglot/discussions/822#discussioncomment-2053395

It's a reasonable approach too. Just not the one we took here. It would 
make sense to try to make it work first.

And if we're comparing different commands similar to this one, how does 
`projectile-kill-buffers' behave?

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> This whole discussion is about different shades of OCD. One party
> wants to clean up as much as possible, another says don't touch my
> things.

I think this discussion is about mistaking resource access for resource
ownership.  Just because project.el or any Lisp package can access the
full list of buffers, doesn't mean it can do whatever it wants with
them.  Just like a routine in a C program can see the full memory
address space of the process, and possibly even form pointers to these
bytes, but it shouldn't rely on their values and certainly can't free()
what it didn't malloc().

> I don't think there is an objective "right" way to do things, only
> something we're able to agree on in the end. I don't really use this
> feature much myself: if you're able to come to an agreement with
> Philip (who took the initiative on adding that command), I'll be
> happy.

I think the command is pretty useful.  But perhaps, I'm just guessing
here, Philip is focusing too much it as if it were the only sanctioned
way for Emacs users to stop working on a given set of files in a
programming project and clean up.

So project-k-buffers is useful but it doesn't have that exclusive. If it
did (but I don't think it will ever have) then Eglot could indeed attach
connection management to it.

> Most object types are garbage-collected when no live reference to them
> remains. That's not the case for buffers.

Because there is always at least one live reference to them, obviously.
But why does this matter?  In this buffer's case there are probably even
more.  One of these references is the one that Eglot and Jsonrpc control
the program or network process.  This is held in global variable.  There
are no resource leaks, as far as I know.

> Is the buffer in question killed when the user calls 'M-x
> eglot-shutdown'? If so, consider that, after the user calls
> project-kill-buffers, there won't be any buffers remaining that belong
> to that project, that the user would be able to call 'M-x
> eglot-shutdown' from.

Are you sure?  Well you should actually try M-x eglot-shutdown and see
what happens then :-)

>> I M-x cd in *scratch* all the time.  It's a global scratch pad,
>> now accessible via scratch-buffer everywhere.
> And I don't have any projects that "~" belongs to.

Neither do I.  But when I M-x cd to other places, project.el thinks that
scratch belongs to that project.  It doesn't, I keep stuff of various
projects in it.

> Same place you changed the major mode in the last patch:
> jsonrpc.el. If jsonrpc.el doesn't want its buffers to be killed, it
> could set that up as described above, through
> kill-buffer-query-functions.

Why should resource owners go to the hassle of whitelisting themselves
from other packages' newfound disregard for private property?  Not to
mention jsonrpc.el wants its buffers to be killed in controlled
circunstances.  So now it would have to "unprotect itself" in those
places.  I can't even think of adjectifying this design, let alone deal
with the corner cases.

>>>> So please consider fixing this in project.el.  As Manuel pointed out,
>>>> the venerable ibuffer.el's ibuffer-kill-filter-group also kills project
>>>> buffers and handles this whole thing very well.  We should just take a
>>>> hint from it.
>>> I'm unable to find that message.
>> In the original conversation:
>> https://github.com/joaotavora/eglot/discussions/822#discussioncomment-2053395
>
> It's a reasonable approach too. Just not the one we took here. It
> would make sense to try to make it work first.

Ibuffer understands ownership, so it comes with bug-free and
hassle-free.  Sounds more than "reasonable" to me.

Just commit the original tested patch I gave you that exempts hidden
buffers without buffer-file-name from project-buffers.  Philip's command
will keep working perfectly and we can close this bug.

João

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:

> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> This whole discussion is about different shades of OCD. One party
>> wants to clean up as much as possible, another says don't touch my
>> things.
>
> I think this discussion is about mistaking resource access for resource
> ownership.  Just because project.el or any Lisp package can access the
> full list of buffers, doesn't mean it can do whatever it wants with
> them.  Just like a routine in a C program can see the full memory
> address space of the process, and possibly even form pointers to these
> bytes, but it shouldn't rely on their values and certainly can't free()
> what it didn't malloc().

But to extend this metaphor, if I kill a programm that allocated malloc,
I would expect that memory to be cleaned up.

>> I don't think there is an objective "right" way to do things, only
>> something we're able to agree on in the end. I don't really use this
>> feature much myself: if you're able to come to an agreement with
>> Philip (who took the initiative on adding that command), I'll be
>> happy.
>
> I think the command is pretty useful.  But perhaps, I'm just guessing
> here, Philip is focusing too much it as if it were the only sanctioned
> way for Emacs users to stop working on a given set of files in a
> programming project and clean up.

Of course it isn't, it is just my prefered way and Eglot breaks it.

> So project-k-buffers is useful but it doesn't have that exclusive. If it
> did (but I don't think it will ever have) then Eglot could indeed attach
> connection management to it.
>
>> Most object types are garbage-collected when no live reference to them
>> remains. That's not the case for buffers.
>
> Because there is always at least one live reference to them, obviously.
> But why does this matter?  In this buffer's case there are probably even
> more.  One of these references is the one that Eglot and Jsonrpc control
> the program or network process.  This is held in global variable.  There
> are no resource leaks, as far as I know.
>
>> Is the buffer in question killed when the user calls 'M-x
>> eglot-shutdown'? If so, consider that, after the user calls
>> project-kill-buffers, there won't be any buffers remaining that belong
>> to that project, that the user would be able to call 'M-x
>> eglot-shutdown' from.
>
> Are you sure?  Well you should actually try M-x eglot-shutdown and see
> what happens then :-)
>
>>> I M-x cd in *scratch* all the time.  It's a global scratch pad,
>>> now accessible via scratch-buffer everywhere.
>> And I don't have any projects that "~" belongs to.
>
> Neither do I.  But when I M-x cd to other places, project.el thinks that
> scratch belongs to that project.  It doesn't, I keep stuff of various
> projects in it.

I agree, this is bad, but that can easily be solved by adding
`lisp-interaction-mode' to the list of major modes that are not killed.

>> Same place you changed the major mode in the last patch:
>> jsonrpc.el. If jsonrpc.el doesn't want its buffers to be killed, it
>> could set that up as described above, through
>> kill-buffer-query-functions.
>
> Why should resource owners go to the hassle of whitelisting themselves
> from other packages' newfound disregard for private property?  Not to
> mention jsonrpc.el wants its buffers to be killed in controlled
> circunstances.  So now it would have to "unprotect itself" in those
> places.  I can't even think of adjectifying this design, let alone deal
> with the corner cases.
>
>>>>> So please consider fixing this in project.el.  As Manuel pointed out,
>>>>> the venerable ibuffer.el's ibuffer-kill-filter-group also kills project
>>>>> buffers and handles this whole thing very well.  We should just take a
>>>>> hint from it.
>>>> I'm unable to find that message.
>>> In the original conversation:
>>> https://github.com/joaotavora/eglot/discussions/822#discussioncomment-2053395
>>
>> It's a reasonable approach too. Just not the one we took here. It
>> would make sense to try to make it work first.
>
> Ibuffer understands ownership, so it comes with bug-free and
> hassle-free.  Sounds more than "reasonable" to me.
>
> Just commit the original tested patch I gave you that exempts hidden
> buffers without buffer-file-name from project-buffers.  Philip's command
> will keep working perfectly and we can close this bug.

So if I understand correctly, with `eglot-autoshutdown' enabled as soon
as all the buffers have been killed, the server will also shut down,
right?

Regarding the patch itself, I think it would be better to use
`project-kill-buffer-conditions', so that if a project.el backend
defines a new implementation for `project-buffers', the issue doesn't
pop up again.

> João

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

[João Távora]

Philip Kaludercic <philipk@posteo.net> writes:

>> bytes, but it shouldn't rely on their values and certainly can't free()
>> what it didn't malloc().
> But to extend this metaphor, if I kill a programm that allocated malloc,
> I would expect that memory to be cleaned up.

Yes, but to kill a process you have to own it.  project.el is not the
owner (not even a co-owner) of Eglot LSP internal buffers, and so it
can't kill them.

>> I think the command is pretty useful.  But perhaps, I'm just guessing
>> here, Philip is focusing too much it as if it were the only sanctioned
>> way for Emacs users to stop working on a given set of files in a
>> programming project and clean up.
> Of course it isn't, it is just my prefered way and Eglot breaks it.

I don't think we should play the who-broke-what game.  It doesn't help,
and if we decided to look up the dates of introduction of your
project-kill-buffers way and eglot's process management, we might reach
a different conclusion.

>> Neither do I.  But when I M-x cd to other places, project.el thinks that
>> scratch belongs to that project.  It doesn't, I keep stuff of various
>> projects in it.
>
> I agree, this is bad, but that can easily be solved by adding
> `lisp-interaction-mode' to the list of major modes that are not
> killed.

Also *ielm*, the global Elisp repl created by M-x ielm. has the same
problem.  And *Completions*.  I'm quite sure that *sly-scratch* in the
SLY CL IDE would also be targeted.  And the CIDER Clojure IDE, as
someone has already reported.  And probably many more.  This blanket
default-directory heuristic is practical in some common cases but flawed
in many others.

>> Just commit the original tested patch I gave you that exempts hidden
>> buffers without buffer-file-name from project-buffers.  Philip's command
>> will keep working perfectly and we can close this bug.
>
> So if I understand correctly, with `eglot-autoshutdown' enabled as soon
> as all the buffers have been killed, the server will also shut down,
> right?

Yes! This is exactly what the docstring says:

   eglot-autoshutdown is a variable defined in `eglot.el'.

   If non-nil, shut down server after killing last managed buffer.

> Regarding the patch itself, I think it would be better to use
> `project-kill-buffer-conditions', so that if a project.el backend
> defines a new implementation for `project-buffers', the issue doesn't
> pop up again.

Your concern is quite valid.  Fortunately, CLOS generic functions have
us covered.  This is even simpler than the first patch:

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index ac278edd40..f8190eb1fc 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -362,6 +362,13 @@ project-buffers
         (push buf bufs)))
     (nreverse bufs)))
 
+(cl-defmethod project-buffers :around (_project)
+  "Ensure hidden/private buffers do not belong to PROJECT."
+  (cl-remove-if-not (lambda (b)
+                      (and (string-prefix-p " " (buffer-name b))
+                           (not (buffer-file-name b))))
+                    (cl-call-next-method)))
+
 (defgroup project-vc nil
   "Project implementation based on the VC package."
   :version "25.1"


Note this still leaves the *scratch* and *ielm* and other things
uncovered.  It addresses this specific bug and most importantly doesn't
blow up in the users.

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:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>>> bytes, but it shouldn't rely on their values and certainly can't free()
>>> what it didn't malloc().
>> But to extend this metaphor, if I kill a programm that allocated malloc,
>> I would expect that memory to be cleaned up.
>
> Yes, but to kill a process you have to own it.  project.el is not the
> owner (not even a co-owner) of Eglot LSP internal buffers, and so it
> can't kill them.

What would be a co-owner?

My view is that project.el is a manager of buffers, but that isn't
relevant anymore.

>>> I think the command is pretty useful.  But perhaps, I'm just guessing
>>> here, Philip is focusing too much it as if it were the only sanctioned
>>> way for Emacs users to stop working on a given set of files in a
>>> programming project and clean up.
>> Of course it isn't, it is just my prefered way and Eglot breaks it.
>
> I don't think we should play the who-broke-what game.  It doesn't help,
> and if we decided to look up the dates of introduction of your
> project-kill-buffers way and eglot's process management, we might reach
> a different conclusion.

I really just meant "break" as in works until Eglot is enabled, and
nothing more than that.

>>> Neither do I.  But when I M-x cd to other places, project.el thinks that
>>> scratch belongs to that project.  It doesn't, I keep stuff of various
>>> projects in it.
>>
>> I agree, this is bad, but that can easily be solved by adding
>> `lisp-interaction-mode' to the list of major modes that are not
>> killed.
>
> Also *ielm*, the global Elisp repl created by M-x ielm. has the same
> problem.  And *Completions*.  I'm quite sure that *sly-scratch* in the
> SLY CL IDE would also be targeted.  And the CIDER Clojure IDE, as
> someone has already reported.  And probably many more.  This blanket
> default-directory heuristic is practical in some common cases but flawed
> in many others.

Project.el uses the same condition format like `buffer-match-p', which
is quite flexible.  Maybe all earmuffed buffers should be spared
("\\`\\*.+\\*\\'"), but in my experience that can be too lenient.

>>> Just commit the original tested patch I gave you that exempts hidden
>>> buffers without buffer-file-name from project-buffers.  Philip's command
>>> will keep working perfectly and we can close this bug.
>>
>> So if I understand correctly, with `eglot-autoshutdown' enabled as soon
>> as all the buffers have been killed, the server will also shut down,
>> right?
>
> Yes! This is exactly what the docstring says:
>
>    eglot-autoshutdown is a variable defined in `eglot.el'.
>
>    If non-nil, shut down server after killing last managed buffer.

Ok, great.

>> Regarding the patch itself, I think it would be better to use
>> `project-kill-buffer-conditions', so that if a project.el backend
>> defines a new implementation for `project-buffers', the issue doesn't
>> pop up again.
>
> Your concern is quite valid.  Fortunately, CLOS generic functions have
> us covered.  This is even simpler than the first patch:
>
> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index ac278edd40..f8190eb1fc 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -362,6 +362,13 @@ project-buffers
>          (push buf bufs)))
>      (nreverse bufs)))
>  
> +(cl-defmethod project-buffers :around (_project)
> +  "Ensure hidden/private buffers do not belong to PROJECT."
> +  (cl-remove-if-not (lambda (b)
> +                      (and (string-prefix-p " " (buffer-name b))
> +                           (not (buffer-file-name b))))
> +                    (cl-call-next-method)))
> +
>  (defgroup project-vc nil
>    "Project implementation based on the VC package."
>    :version "25.1"

I have to still admit that I am uncertain if the general ignoring of all
hidden buffers is justified.  I have certainly used hidden buffers
frequently enough but never assumed that they were outside of anyone's
control.  They are just regular buffers with a special kind of name
after all.

We ought to be able to define a cleaner way of clarifying what buffers
can and cannot belong to projects.  Personally I think a buffer-local
variable/predicate would be a better approach.

> Note this still leaves the *scratch* and *ielm* and other things
> uncovered.  It addresses this specific bug and most importantly doesn't
> blow up in the users.

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

[João Távora]

Philip Kaludercic <philipk@posteo.net> writes:

> What would be a co-owner?
>
> My view is that project.el is a manager of buffers, but that isn't
> relevant anymore.

You almost answered the question.  There is more than one manager of
buffers.  Ibuffer manages buffer, M-x buffer-list, the user.  Eglot
manages some normal source code buffers in a co-owned way.

In contrast, the JSONRPC stdout and stderr hidden buffers are managed by
no-one else other than jsonrpc.el: they're not co-owned.

> Project.el uses the same condition format like `buffer-match-p', which
> is quite flexible.  Maybe all earmuffed buffers should be spared
> ("\\`\\*.+\\*\\'"), but in my experience that can be too lenient.

Yes, I think some earmuffed buffers can clearly belong to projects.
Others probably not.  You haven't seen me complain about
project-kill-buffers killing the *EGLOT events* buffer, for example ;-)

That is not a hidden/private buffer, so I accept that other buffer
managers kill it.

> I have to still admit that I am uncertain if the general ignoring of all
> hidden buffers is justified.

Notice that the patch is more tame: only hidden buffers without
buffer-file-names are exempted from project-buffers.

> I have certainly used hidden buffers
> frequently enough but never assumed that they were outside of anyone's
> control.

What have you used them for?

> They are just regular buffers with a special kind of name
> after all.

I don't know any "irregular" buffers, but yes, that is their
representation.  This representation is an old convention in Emacs for
"out of bounds" and "out of sight".

> We ought to be able to define a cleaner way of clarifying what buffers
> can and cannot belong to projects.

Agreed.

> Personally I think a buffer-local variable/predicate would be a better
> approach.

You can think about new conventions and protocols but you force it down
existing package's throats.  That doesn't go well as this bug shows.
That's because there are many diverse packages out there, that you can't
possibly know about and they likely aren't tracking your developments
and your thinking.

Have you considered the converse approach which is to be conservative?
It doesn't have these drawbacks.  In your project buffer's bucket put
only non-earmuffed, non-hidden, file-visiting buffers automatically.
Those are relatively safe.  Then have a buffer-local variable for
packages to opt into -- not opt out of -- your scheme.

As project.el and its new commands gain popularity (I hope it does) then
more and more packages, major-modes, etc will want to buy into its API
and add a little (setq-local project-owned t) when they setup their
helper buffers.  It takes a bit more marketing work, but in the end it's
better.

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

[Eli Zaretskii]

> Cc: Manuel Uberti <manuel.uberti@inventati.org>, 58839@debbugs.gnu.org,
>  Dmitry Gutov <dgutov@yandex.ru>
> From: João Távora <joaotavora@gmail.com>
> Date: Sat, 29 Oct 2022 23:49:01 +0100
> 
> Have you considered the converse approach which is to be conservative?
> It doesn't have these drawbacks.  In your project buffer's bucket put
> only non-earmuffed, non-hidden, file-visiting buffers automatically.
> Those are relatively safe.  Then have a buffer-local variable for
> packages to opt into -- not opt out of -- your scheme.

I believe I made a similar argument with Dmitry back when project.el
was added to Emacs.  Dmitry didn't like my suggestion back then, and I
have doubts that he changed his mind.

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

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> Have you considered the converse approach which is to be conservative?
>> It doesn't have these drawbacks.  In your project buffer's bucket put
>> only non-earmuffed, non-hidden, file-visiting buffers automatically.
>> Those are relatively safe.  Then have a buffer-local variable for
>> packages to opt into -- not opt out of -- your scheme.
> I believe I made a similar argument with Dmitry back when project.el
> was added to Emacs.  Dmitry didn't like my suggestion back then, and I
> have doubts that he changed his mind.

I'd just like state, if there was any doubt, that project.el is a
fantastic and long-awaited addition to Emacs.  The design is generally
great and for one it enabled Eglot to integrate very well.  So I am
quite thankful to Dmitry and its maintainer.  The new project-wide
commands (project-find-file, project-switch-to-buffer,
project-kill-buffers) are really very useful, and I'm learning to use
them.  But this point of "which buffers belong to projects" seems off.
Not by a terrible lot, but definitely off the way it is.  This
discussion has revealed some of its flaws.  There's still time to adjust
the design before Emacs 29.

João

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

[Dmitry Gutov]

On 30.10.2022 08:28, Eli Zaretskii wrote:
>> Cc: Manuel Uberti<manuel.uberti@inventati.org>,58839@debbugs.gnu.org,
>>   Dmitry Gutov<dgutov@yandex.ru>
>> From: João Távora<joaotavora@gmail.com>
>> Date: Sat, 29 Oct 2022 23:49:01 +0100
>>
>> Have you considered the converse approach which is to be conservative?
>> It doesn't have these drawbacks.  In your project buffer's bucket put
>> only non-earmuffed, non-hidden, file-visiting buffers automatically.
>> Those are relatively safe.  Then have a buffer-local variable for
>> packages to opt into -- not opt out of -- your scheme.
> I believe I made a similar argument with Dmitry back when project.el
> was added to Emacs.  Dmitry didn't like my suggestion back then, and I
> have doubts that he changed his mind.

Do you have a link to that message? Details matter.

Anyway, if we do decide to flip the switch, it should be through 
project-kill-buffer-conditions, so the user can make a different choice 
through customization.

So far we have 2:1 votes against that, though.

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

[Eli Zaretskii]

> Date: Sun, 30 Oct 2022 17:58:18 +0200
> Cc: philipk@posteo.net, 58839@debbugs.gnu.org, manuel.uberti@inventati.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> On 30.10.2022 08:28, Eli Zaretskii wrote:
> >> Those are relatively safe.  Then have a buffer-local variable for
> >> packages to opt into -- not opt out of -- your scheme.
> > I believe I made a similar argument with Dmitry back when project.el
> > was added to Emacs.  Dmitry didn't like my suggestion back then, and I
> > have doubts that he changed his mind.
> 
> Do you have a link to that message? Details matter.

Not anymore, sorry.  It doesn't really matter, if you are now okay
with making buffers that don't visit files by default not belong to a
project, regardless of their default-directory.

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

[Dmitry Gutov]

On 30.10.2022 18:39, Eli Zaretskii wrote:
>> Date: Sun, 30 Oct 2022 17:58:18 +0200
>> Cc: philipk@posteo.net, 58839@debbugs.gnu.org, manuel.uberti@inventati.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>>
>> On 30.10.2022 08:28, Eli Zaretskii wrote:
>>>> Those are relatively safe.  Then have a buffer-local variable for
>>>> packages to opt into -- not opt out of -- your scheme.
>>> I believe I made a similar argument with Dmitry back when project.el
>>> was added to Emacs.  Dmitry didn't like my suggestion back then, and I
>>> have doubts that he changed his mind.
>>
>> Do you have a link to that message? Details matter.
> 
> Not anymore, sorry.  It doesn't really matter, if you are now okay
> with making buffers that don't visit files by default not belong to a
> project, regardless of their default-directory.

It would help remind you of the original explanation, instead of putting 
me on the defensive. And recall the supporting voices from the others in 
the same discussion. And maybe add the explanation to the docs, if it 
looks that non-obvious.

To reiterate: we usually want to consider VC-Dir, Diff, Dired and 
Compilation buffers as part of the project. Even though they are not 
associated with a file. Because 'C-x p b' can be handy to switch to 
particular one, and 'C-x p k' can clean them up (when you're "closing" 
the project).

People who think differently, can use project-ignore-buffer-conditions 
and/or use some special backend (probably defined by themselves) that 
overrides 'project-buffers' with a different logic.

The issue Joao is having, however, is with particular "hidden" 
non-file-visiting buffers (in fundamental-mode). And here the argument 
could be made both ways.

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

[Eli Zaretskii]

> Date: Sun, 30 Oct 2022 21:13:51 +0200
> Cc: joaotavora@gmail.com, philipk@posteo.net, 58839@debbugs.gnu.org,
>  manuel.uberti@inventati.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> >> Do you have a link to that message? Details matter.
> > 
> > Not anymore, sorry.  It doesn't really matter, if you are now okay
> > with making buffers that don't visit files by default not belong to a
> > project, regardless of their default-directory.
> 
> It would help remind you of the original explanation, instead of putting 
> me on the defensive.

You don't need to be defensive, I did not intend to restart the
argument.  I just wanted to inform João that your opinion was and
remains that non-file visiting buffers should be part of the project,
that's all.

> The issue Joao is having, however, is with particular "hidden" 
> non-file-visiting buffers (in fundamental-mode). And here the argument 
> could be made both ways.

He was making a more general argument about buffers that don't visit
files, AFAIU.  I wanted to inform him that this idea was already
considered in the past.

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

[Dmitry Gutov]

On 30.10.2022 21:54, Eli Zaretskii wrote:
> You don't need to be defensive, I did not intend to restart the
> argument.  I just wanted to inform João that your opinion was and
> remains that non-file visiting buffers should be part of the project,
> that's all.

Thanks.

>> The issue Joao is having, however, is with particular "hidden"
>> non-file-visiting buffers (in fundamental-mode). And here the argument
>> could be made both ways.
> He was making a more general argument about buffers that don't visit
> files, AFAIU.  I wanted to inform him that this idea was already
> considered in the past.

I don't know if that was his point. To quote:

   Yes, I think some earmuffed buffers can clearly belong to projects.
   Others probably not.  You haven't seen me complain about
   project-kill-buffers killing the *EGLOT events* buffer, for example 😉

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Anyway, if we do decide to flip the switch, it should be through
> project-kill-buffer-conditions, so the user can make a different
> choice through customization.

project-kill-buffer-conditions doesn't work, I've tried it, it has this
fundamental-mode thing there that makes it impossible.  Supposedly it is
there to serve some purpose that no-one seems to be able to find a
argumentative basis for.

It's quite clear that _some_ non-file-visiting buffers can be considered
as belonging to a project's working set.  But it's very very easy to
come up with many that cannot be considered so.

Because "killing buffers" is a destructive operation, being greedy here
is a really bad design decision, as it catches an arbitrary number of
unsuspecting extensions off-guard, which have been using earmuffed
buffers for many years.  

All in all, it's like you're making a gun that only backfires 5% of the
time.

In the little time I've used this feature since the start of this
discussion I have discovered it backfires no small number of occasions:
Eglot, CIDER, *scratch*, *ielm*, *sly-scratch*, *Completions*,...  Heck
even *ibuffer* itself is targeted by this.

Project-kill-buffers is off. Its intention pretty useful, but its
implementation is a blunder.  The root cause is this overgreedy
project-buffers.  When "killing a project" the echo area asks me if I
want to kill a number of buffers that I didn't even know I had, because
of hidden buffers.  This cannot be logical and the only way the
"argument can be made both ways" is out of stubborness.

JSONRPC's buffers are hidden implementation details: the argument that
they are somehow under the responsibility of project.el just because it
can see them through (buffer-list) is blind tiranny.

The mini-languages invented in project-kill-buffers-conditions and
project-ignore-buffer-conditions are abominations.  If project-buffers
just been conservatively designed, we'd need nothing more than the
existing hooks for the exceptions.  *earmuffed* buffers interested in
opting in could declare if it belonged or not in one line.  Just like

diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index dc3ed52650..718bebc7cd 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -179,6 +179,7 @@ vc-setup-buffer
   (let ((camefrom (current-buffer))
 	(olddir default-directory))
     (set-buffer (get-buffer-create buf))
+    (setq-local project-owned t)
     (let ((oldproc (get-buffer-process (current-buffer))))
       ;; If we wanted to wait for oldproc to finish before doing
       ;; something, we'd have used vc-eval-after.

To name one.  The above is just the converse of the solution proposed by
Philip before.

Anyway, I've now suggested and presented 2 actually tested, actually
working patches to project.el.  I don't have anything more to add.

João

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

[João Távora]

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

On Mon, Oct 31, 2022 at 9:52 AM João Távora <joaotavora@gmail.com> wrote:

> In the little time I've used this feature since the start of this
> discussion I have discovered it backfires no small number of occasions:
> Eglot, CIDER, *scratch*, *ielm*, *sly-scratch*, *Completions*,...  Heck
> even *ibuffer* itself is targeted by this.

And you can add the gnus mail buffers to this list. If you are unlucky
enough to start M-x gnus from a project file to read your email, then
closing that project in the future will take your gnus session, your
summary buffers, messages, etc. with it.  This can't possibly be considered
an exotic use case, and can't be right.

João

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

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:

> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> Anyway, if we do decide to flip the switch, it should be through
>> project-kill-buffer-conditions, so the user can make a different
>> choice through customization.
>
> project-kill-buffer-conditions doesn't work, I've tried it, it has this
> fundamental-mode thing there that makes it impossible.  Supposedly it is
> there to serve some purpose that no-one seems to be able to find a
> argumentative basis for.

The condition can be written entirely, if we are willing to accept a
breaking change.  In the case of `project-kill-buffer', this ought to be
acceptable if fewer buffers are killed.

> It's quite clear that _some_ non-file-visiting buffers can be considered
> as belonging to a project's working set.  But it's very very easy to
> come up with many that cannot be considered so.

I have to admit that I am more and more inclined to make the list a
opt-in thing, where  we explicitly mark those major modes that are tied
to a project.

> Because "killing buffers" is a destructive operation, being greedy here
> is a really bad design decision, as it catches an arbitrary number of
> unsuspecting extensions off-guard, which have been using earmuffed
> buffers for many years.  
>
> All in all, it's like you're making a gun that only backfires 5% of the
> time.
>
> In the little time I've used this feature since the start of this
> discussion I have discovered it backfires no small number of occasions:
> Eglot, CIDER, *scratch*, *ielm*, *sly-scratch*, *Completions*,...  Heck
> even *ibuffer* itself is targeted by this.
>
> Project-kill-buffers is off. Its intention pretty useful, but its
> implementation is a blunder.  The root cause is this overgreedy
> project-buffers.  When "killing a project" the echo area asks me if I
> want to kill a number of buffers that I didn't even know I had, because
> of hidden buffers.  This cannot be logical and the only way the
> "argument can be made both ways" is out of stubborness.
>
> JSONRPC's buffers are hidden implementation details: the argument that
> they are somehow under the responsibility of project.el just because it
> can see them through (buffer-list) is blind tiranny.
>
> The mini-languages invented in project-kill-buffers-conditions and
> project-ignore-buffer-conditions are abominations.  If project-buffers
> just been conservatively designed, we'd need nothing more than the
> existing hooks for the exceptions.  *earmuffed* buffers interested in
> opting in could declare if it belonged or not in one line.  

What existing hooks?

>                                                             Just like
>
> diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
> index dc3ed52650..718bebc7cd 100644
> --- a/lisp/vc/vc-dispatcher.el
> +++ b/lisp/vc/vc-dispatcher.el
> @@ -179,6 +179,7 @@ vc-setup-buffer
>    (let ((camefrom (current-buffer))
>  	(olddir default-directory))
>      (set-buffer (get-buffer-create buf))
> +    (setq-local project-owned t)
>      (let ((oldproc (get-buffer-process (current-buffer))))
>        ;; If we wanted to wait for oldproc to finish before doing
>        ;; something, we'd have used vc-eval-after.
>
> To name one.  The above is just the converse of the solution proposed by
> Philip before.

I would be fine with this in principle, my only worry is backwards
compatibility for those who use project.el from ELPA.

> Anyway, I've now suggested and presented 2 actually tested, actually
> working patches to project.el.  I don't have anything more to add.
>
> João

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

[Dmitry Gutov]

On 31.10.2022 13:56, João Távora wrote:
> On Mon, Oct 31, 2022 at 9:52 AM João Távora <joaotavora@gmail.com 
> <mailto:joaotavora@gmail.com>> wrote:
> 
>  > In the little time I've used this feature since the start of this
>  > discussion I have discovered it backfires no small number of occasions:
>  > Eglot, CIDER, *scratch*, *ielm*, *sly-scratch*, *Completions*,...  Heck
>  > even *ibuffer* itself is targeted by this.
> 
> And you can add the gnus mail buffers to this list. If you are unlucky
> enough to start M-x gnus from a project file to read your email, then
> closing that project in the future will take your gnus session, your
> summary buffers, messages, etc. with it.  This can't possibly be considered
> an exotic use case, and can't be right.

Does Gnus use fundamental-mode for some of its important buffers?

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

[Dmitry Gutov]

On 31.10.2022 11:53, João Távora wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> Anyway, if we do decide to flip the switch, it should be through
>> project-kill-buffer-conditions, so the user can make a different
>> choice through customization.
> 
> project-kill-buffer-conditions doesn't work, I've tried it, it has this
> fundamental-mode thing there that makes it impossible.  Supposedly it is
> there to serve some purpose that no-one seems to be able to find a
> argumentative basis for.

What have you tried?

This should take care of the specific complaint about unknown 
"invisible" buffers:

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index ac278edd40..1e7573c740 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1223,7 +1223,9 @@ project-display-buffer-other-frame
  (defcustom project-kill-buffer-conditions
    '(buffer-file-name    ; All file-visiting buffers are included.
      ;; Most of the temp buffers in the background:
-    (major-mode . fundamental-mode)
+    (and
+     (major-mode . fundamental-mode)
+     (not "\\` "))
      ;; non-text buffer such as xref, occur, vc, log, ...
      (and (derived-mode . special-mode)
           (not (major-mode . help-mode)))


> It's quite clear that _some_ non-file-visiting buffers can be considered
> as belonging to a project's working set.  But it's very very easy to
> come up with many that cannot be considered so.
> 
> Because "killing buffers" is a destructive operation, being greedy here
> is a really bad design decision, as it catches an arbitrary number of
> unsuspecting extensions off-guard, which have been using earmuffed
> buffers for many years.
> 
> All in all, it's like you're making a gun that only backfires 5% of the
> time.

Yours is the first instance so far.

> In the little time I've used this feature since the start of this
> discussion I have discovered it backfires no small number of occasions:
> Eglot, CIDER, *scratch*, *ielm*, *sly-scratch*, *Completions*,...  Heck
> even *ibuffer* itself is targeted by this.

Of course it is targeted: we want ibuffer buffers to be killed just as 
well when killing a project. And sly-scratch, and etc.

> Project-kill-buffers is off. Its intention pretty useful, but its
> implementation is a blunder.  The root cause is this overgreedy
> project-buffers.  When "killing a project" the echo area asks me if I
> want to kill a number of buffers that I didn't even know I had, because
> of hidden buffers.  This cannot be logical and the only way the
> "argument can be made both ways" is out of stubborness.
> 
> JSONRPC's buffers are hidden implementation details: the argument that
> they are somehow under the responsibility of project.el just because it
> can see them through (buffer-list) is blind tiranny.
> 
> The mini-languages invented in project-kill-buffers-conditions and
> project-ignore-buffer-conditions are abominations.

This is the point where I'd normally blacklist you again.

> diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
> index dc3ed52650..718bebc7cd 100644
> --- a/lisp/vc/vc-dispatcher.el
> +++ b/lisp/vc/vc-dispatcher.el
> @@ -179,6 +179,7 @@ vc-setup-buffer
>     (let ((camefrom (current-buffer))
>   	(olddir default-directory))
>       (set-buffer (get-buffer-create buf))
> +    (setq-local project-owned t)
>       (let ((oldproc (get-buffer-process (current-buffer))))
>         ;; If we wanted to wait for oldproc to finish before doing
>         ;; something, we'd have used vc-eval-after.
> 
> To name one.  The above is just the converse of the solution proposed by
> Philip before.
> 
> Anyway, I've now suggested and presented 2 actually tested, actually
> working patches to project.el.  I don't have anything more to add.

They are not much better than the "patch" I showed for Eglot, 
correctness-wise.

And mine would make it safe against any kill-buffer calls, including 
ones issued by the user.

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

[Dmitry Gutov]

On 31.10.2022 16:35, Philip Kaludercic wrote:
>> It's quite clear that_some_  non-file-visiting buffers can be considered
>> as belonging to a project's working set.  But it's very very easy to
>> come up with many that cannot be considered so.
> I have to admit that I am more and more inclined to make the list a
> opt-in thing, where  we explicitly mark those major modes that are tied
> to a project.

The current contents of project-kill-buffer-conditions are more or less 
that already.

With the exception of the 'fundamental-mode' entry which creates an 
open-ended set. That one we could remove, or tweak by adding a buffer 
name condition, or a variable lookup.

My guess is that nobody will bother with setting the new variable, 
though (call it 'project-owned' or whatever), because killing project 
buffers is not an essential need for any 3rd party package, just 
something nice to have.

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 31.10.2022 13:56, João Távora wrote:
>> On Mon, Oct 31, 2022 at 9:52 AM João Távora <joaotavora@gmail.com
>> <mailto:joaotavora@gmail.com>> wrote:
>>  > In the little time I've used this feature since the start of this
>>  > discussion I have discovered it backfires no small number of occasions:
>>  > Eglot, CIDER, *scratch*, *ielm*, *sly-scratch*, *Completions*,...  Heck
>>  > even *ibuffer* itself is targeted by this.
>> And you can add the gnus mail buffers to this list. If you are
>> unlucky
>> enough to start M-x gnus from a project file to read your email, then
>> closing that project in the future will take your gnus session, your
>> summary buffers, messages, etc. with it.  This can't possibly be considered
>> an exotic use case, and can't be right.
>
> Does Gnus use fundamental-mode for some of its important buffers?

No idea.  gnus-summary-modes is one of the modes

Try:

emacs -Q
C-x C-f some/file/in/a/project/foo.c
M-x  gnus ; Remember to check your email
; read mail, make searches, etc
C-x b foo.c ; go back to the file
C-x p k ; Get preposterous confirmation prompt about tens of buffers about to be killed
        ; say yes, bye all those emails 

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index ac278edd40..1e7573c740 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -1223,7 +1223,9 @@ project-display-buffer-other-frame
>  (defcustom project-kill-buffer-conditions
>    '(buffer-file-name    ; All file-visiting buffers are included.
>      ;; Most of the temp buffers in the background:
> -    (major-mode . fundamental-mode)
> +    (and
> +     (major-mode . fundamental-mode)
> +     (not "\\` "))
>      ;; non-text buffer such as xref, occur, vc, log, ...
>      (and (derived-mode . special-mode)
>           (not (major-mode . help-mode)))

Thanks.  If that works, go ahead and push it.

This should work around this specific bug and then we can open another
one to follow up on all the disaster that has unfolded since.

>> In the little time I've used this feature since the start of this
>> discussion I have discovered it backfires no small number of occasions:
>> Eglot, CIDER, *scratch*, *ielm*, *sly-scratch*, *Completions*,...  Heck
>> even *ibuffer* itself is targeted by this.
> Of course it is targeted: we want ibuffer buffers to be killed just as
> well when killing a project. And sly-scratch, and etc.

No, we don't want.  *sly-scratch* is a global scratchpad for the Common
Lisp connection that can "service" many Common Lisp projects, to use
your own terminology.

Do you know what M-x ibuffer does? It's a manager for all the buffers in
all the projects in Emacs.  In the earmuffed *ibuffer*, it gives you an
overview of all visited buffers, sorted and organized by various
criteria.  Again, *ibuffer* can't possibly be taken to "service a
project".  Destroying this buffer's state because the user decided to
kill a project is simply wrong.  It's very plain to see that.

And *Shell Command Output* where it is impossible to know in advance if
the contents refer to a specific project or not.  It depends on what the
user typed after M-|!

And the Gnus buffers?  And the CIDER report?

>> you're making a gun that only backfires 5% of the time.
> Yours is the first instance so far.

We seem to use different algebraic systems.

>> The mini-languages invented in project-kill-buffers-conditions and
>> project-ignore-buffer-conditions are abominations.
> This is the point where I'd normally blacklist you again.

I had no idea who authored those variables.  If you are among the
authors, I'm very sorry, I was referring merely to code.  I said before
I'm quite happy with project.el, but this (small) part of it is very
badly done.

> They are not much better than the "patch" I showed for Eglot,
> correctness-wise.

Of course they are, they are opt-in.  So project.el's C-x p k doesn't
destroy packages' essential buffers just because of some overly greedy
heuristic.  Using this idea, we make a conservative heuristic better, on
a case by case basis.

> And mine would make it safe against any kill-buffer calls, including
> ones issued by the user.

Should I really to explain again that a hidden buffer is hidden from the
user and thus he can't reasonably M-x kill-buffer on it?

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

[Dmitry Gutov]

On 31.10.2022 22:36, João Távora wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>> On 31.10.2022 13:56, João Távora wrote:
>>> On Mon, Oct 31, 2022 at 9:52 AM João Távora <joaotavora@gmail.com
>>> <mailto:joaotavora@gmail.com>> wrote:
>>>   > In the little time I've used this feature since the start of this
>>>   > discussion I have discovered it backfires no small number of occasions:
>>>   > Eglot, CIDER,*scratch*,*ielm*,*sly-scratch*,*Completions*,...  Heck
>>>   > even*ibuffer*  itself is targeted by this.
>>> And you can add the gnus mail buffers to this list. If you are
>>> unlucky
>>> enough to start M-x gnus from a project file to read your email, then
>>> closing that project in the future will take your gnus session, your
>>> summary buffers, messages, etc. with it.  This can't possibly be considered
>>> an exotic use case, and can't be right.
>> Does Gnus use fundamental-mode for some of its important buffers?
> No idea.  gnus-summary-modes is one of the modes
> 
> Try:
> 
> emacs -Q
> C-x C-f some/file/in/a/project/foo.c
> M-x  gnus ; Remember to check your email
> ; read mail, make searches, etc
> C-x b foo.c ; go back to the file
> C-x p k ; Get preposterous confirmation prompt about tens of buffers about to be killed
>          ; say yes, bye all those emails

I suppose it might match

   (and (derived-mode . special-mode)
        (not (major-mode . help-mode)))

then.

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

[Dmitry Gutov]

On 31.10.2022 22:58, João Távora wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
>> index ac278edd40..1e7573c740 100644
>> --- a/lisp/progmodes/project.el
>> +++ b/lisp/progmodes/project.el
>> @@ -1223,7 +1223,9 @@ project-display-buffer-other-frame
>>   (defcustom project-kill-buffer-conditions
>>     '(buffer-file-name    ; All file-visiting buffers are included.
>>       ;; Most of the temp buffers in the background:
>> -    (major-mode . fundamental-mode)
>> +    (and
>> +     (major-mode . fundamental-mode)
>> +     (not "\\` "))
>>       ;; non-text buffer such as xref, occur, vc, log, ...
>>       (and (derived-mode . special-mode)
>>            (not (major-mode . help-mode)))
> 
> Thanks.  If that works, go ahead and push it.

I suggest you try it first.

Last time I launched Eglot was around several months ago.

> This should work around this specific bug and then we can open another
> one to follow up on all the disaster that has unfolded since.

Disaster, really?

>>> In the little time I've used this feature since the start of this
>>> discussion I have discovered it backfires no small number of occasions:
>>> Eglot, CIDER, *scratch*, *ielm*, *sly-scratch*, *Completions*,...  Heck
>>> even *ibuffer* itself is targeted by this.
>> Of course it is targeted: we want ibuffer buffers to be killed just as
>> well when killing a project. And sly-scratch, and etc.
> 
> No, we don't want.  *sly-scratch* is a global scratchpad for the Common
> Lisp connection that can "service" many Common Lisp projects, to use
> your own terminology.

Okay, you're probably right about at least some of those.

> Do you know what M-x ibuffer does? It's a manager for all the buffers in
> all the projects in Emacs.  In the earmuffed *ibuffer*, it gives you an
> overview of all visited buffers, sorted and organized by various
> criteria.  Again, *ibuffer* can't possibly be taken to "service a
> project".  Destroying this buffer's state because the user decided to
> kill a project is simply wrong.  It's very plain to see that.

I must have been thinking about project-* or projectile-* counterparts.

Like projectile-ibuffer or the homemade version for project.el made by a 
user in a bug report nearby.

> And *Shell Command Output* where it is impossible to know in advance if
> the contents refer to a specific project or not.  It depends on what the
> user typed after M-|!
> 
> And the Gnus buffers?  And the CIDER report?

Do you know whether CIDER will be satisfied by the same patch I sent 
previously?

>>> you're making a gun that only backfires 5% of the time.
>> Yours is the first instance so far.
> 
> We seem to use different algebraic systems.

This is literally the first bug report on the subject.

>>> The mini-languages invented in project-kill-buffers-conditions and
>>> project-ignore-buffer-conditions are abominations.
>> This is the point where I'd normally blacklist you again.
> 
> I had no idea who authored those variables.  If you are among the
> authors, I'm very sorry, I was referring merely to code.  I said before
> I'm quite happy with project.el, but this (small) part of it is very
> badly done.

I'm not the only author. Regardless, it's not a good language to use no 
matter who wrote it.

What you are doing is pressuring all other participants into your POV by 
means of an insult. That usually works better if the offending code was 
written by somebody who already left (the project/the discussion/the 
company/etc), or is a little younger.

>> They are not much better than the "patch" I showed for Eglot,
>> correctness-wise.
> 
> Of course they are, they are opt-in.  So project.el's C-x p k doesn't
> destroy packages' essential buffers just because of some overly greedy
> heuristic.  Using this idea, we make a conservative heuristic better, on
> a case by case basis.
> 
>> And mine would make it safe against any kill-buffer calls, including
>> ones issued by the user.
> 
> Should I really to explain again that a hidden buffer is hidden from the
> user and thus he can't reasonably M-x kill-buffer on it?

They can, though, even if odds are low. It can also happen through some 
other automation, which Emacs lets the users do freely.

I'm fairly sure that the solution I offered would be easy enough 
implement, to actually protect the vulnerable buffer.

I suppose we are not doing that, however.

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 31.10.2022 22:36, João Távora wrote:
>> Dmitry Gutov<dgutov@yandex.ru>  writes:
>> 
>>> On 31.10.2022 13:56, João Távora wrote:
>>>> On Mon, Oct 31, 2022 at 9:52 AM João Távora <joaotavora@gmail.com
>>>> <mailto:joaotavora@gmail.com>> wrote:
>>>>   > In the little time I've used this feature since the start of this
>>>>   > discussion I have discovered it backfires no small number of occasions:
>>>>   > Eglot, CIDER,*scratch*,*ielm*,*sly-scratch*,*Completions*,...  Heck
>>>>   > even*ibuffer*  itself is targeted by this.
>>>> And you can add the gnus mail buffers to this list. If you are
>>>> unlucky
>>>> enough to start M-x gnus from a project file to read your email, then
>>>> closing that project in the future will take your gnus session, your
>>>> summary buffers, messages, etc. with it.  This can't possibly be considered
>>>> an exotic use case, and can't be right.
>>> Does Gnus use fundamental-mode for some of its important buffers?
>> No idea.  gnus-summary-modes is one of the modes
>> Try:
>> emacs -Q
>> C-x C-f some/file/in/a/project/foo.c
>> M-x  gnus ; Remember to check your email
>> ; read mail, make searches, etc
>> C-x b foo.c ; go back to the file
>> C-x p k ; Get preposterous confirmation prompt about tens of buffers about to be killed
>>          ; say yes, bye all those emails
>
> I suppose it might match
>
>   (and (derived-mode . special-mode)
>        (not (major-mode . help-mode)))
>
> then.

Whatever it's doing, it's completely wrong and must be fixed.  It
probably break other email clients too.

João

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

[João Távora]

Philip Kaludercic <philipk@posteo.net> writes:

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

> What existing hooks?

The major-mode hooks, naturally.

Using major-mode hooks and buffer-local variables is the standard Emacs
way of attaching characteristics to buffers.  The Elisp required is
trivial and there are thousands of examples for doing it out there.
There's no need to introduce new less powerful and more awkward
languages in variables.

>> It's quite clear that _some_ non-file-visiting buffers can be considered
>> as belonging to a project's working set.  But it's very very easy to
>> come up with many that cannot be considered so.
>
> I have to admit that I am more and more inclined to make the list a
> opt-in thing, where  we explicitly mark those major modes that are tied
> to a project.

Yes, and we can always later overhaul project-owned to be function for
doing more complex calculations based on the name of the buffer, for
example.  But a simple boolean would probably suffice for vc buffers,
and other earmuffed buffers created by packages in Emacs.

>> To name one.  The above is just the converse of the solution proposed by
>> Philip before.
>
> I would be fine with this in principle, my only worry is backwards
> compatibility for those who use project.el from ELPA.

We can try to make some backward compatible way, but this seems like the
perfect occasion to activate this most reasonable header in project.el:

    ;; NOTE: The project API is still experimental and can change in major,
    ;; backward-incompatible ways.  Everyone is encouraged to try it, and
    ;; report to us any problems or use cases we hadn't anticipated, by
    ;; sending an email to emacs-devel, or `M-x report-emacs-bug'.

It sure seems like there were things that project.el's authors didn't
anticipate.  A mistake is a mistake there should be no shame in
rectifying it.

In practice, I doubt many people are setting these variables.  A GitHub
code search, where it is normally easy to find customizations of
variables since users version their configurations there, turns up 0
results for customizations of project-ignore-buffer-conditions and not
more than a handful for project-kill-buffer-conditions, for example.  In
contrast you can see hundreds of people setting things like
eglot-autoshutdown or ibuffer-filter-groups (and settings of
scroll-bar-mode are in the thousands).  So I'd say the risk is minimal
and the benefit considerable.

João

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

[Philip Kaludercic]

Dmitry Gutov <dgutov@yandex.ru> writes:

>>>> The mini-languages invented in project-kill-buffers-conditions and
>>>> project-ignore-buffer-conditions are abominations.
>>> This is the point where I'd normally blacklist you again.
>> I had no idea who authored those variables.  If you are among the
>> authors, I'm very sorry, I was referring merely to code.  I said before
>> I'm quite happy with project.el, but this (small) part of it is very
>> badly done.
>
> I'm not the only author. Regardless, it's not a good language to use
> no matter who wrote it.

I believe that was me...

BTW, if there are major objections to the language, I should point out
that the new `buffer-match-p' in Emacs 29 uses the same language and has
already found usage in a number of spots in core Emacs.  There would
still be time to address any issues you might have, and avoid a
long-term mistake.

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:

> In practice, I doubt many people are setting these variables.  A GitHub
> code search, where it is normally easy to find customizations of
> variables since users version their configurations there, turns up 0
> results for customizations of project-ignore-buffer-conditions and not
> more than a handful for project-kill-buffer-conditions, for example.  In
> contrast you can see hundreds of people setting things like
> eglot-autoshutdown or ibuffer-filter-groups (and settings of
> scroll-bar-mode are in the thousands).  So I'd say the risk is minimal
> and the benefit considerable.

If people are customising the variable everything is fine, the issue I
was thinking about were those who are relying on the default behaviour
that might change without them expecting it to.

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

[João Távora]

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

On Tue, Nov 1, 2022 at 10:48 AM Philip Kaludercic <philipk@posteo.net>
wrote:

>
> BTW, if there are major objections to the language, I should point out
> that the new `buffer-match-p' in Emacs 29 uses the same language and has
> already found usage in a number of spots in core Emacs.  There would
> still be time to address any issues you might have, and avoid a
> long-term mistake.
>

For me, it looks like match-buffers is reinventing
cl-remove-if-not and match-buffer-p is reinventing ... unary predicate
function of a buffer?

I'm not fond of these mini-languages because they're less expressive, they
end up being only minimally less complicated and bug-prone, they can't
automatically be byte-compiled for efficiency, and they can't automatically
be byte-compiled for correctness/diagnostics.  If one makes a mistake,
the backtrace is much more complicated.

So these mini-languages may make sense to define filters in thunderbird or
something, but throwing Elisp away here generally doesn't make sense to me.

But there may be exceptions (although this project.el one doesn't seem one
of them) so why don't you show examples of use of these new helpers and
so we can compare side by side with the Elisp-only alternative.

João

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

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

[Dmitry Gutov]

On 01.11.2022 12:59, João Távora wrote:
> For me, it looks like match-buffers is reinventing
> cl-remove-if-not and match-buffer-p is reinventing ... unary predicate
> function of a buffer?

And Eglot is reinventing url-http.

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

[Philip Kaludercic]

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

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

> On Tue, Nov 1, 2022 at 10:48 AM Philip Kaludercic <philipk@posteo.net>
> wrote:
>
>>
>> BTW, if there are major objections to the language, I should point out
>> that the new `buffer-match-p' in Emacs 29 uses the same language and has
>> already found usage in a number of spots in core Emacs.  There would
>> still be time to address any issues you might have, and avoid a
>> long-term mistake.
>>
>
> For me, it looks like match-buffers is reinventing
> cl-remove-if-not and match-buffer-p is reinventing ... unary predicate
> function of a buffer?

You could say that, though I would argue it is an easier way to express
common predicates, while not making anything else more complicated.

E.g. `display-buffer-alist' makes use of it to associate display-buffer
rules with buffers.  Now you can add

      ((major-mode . help-mode) display-buffer-in-side-window)

instead of trying to match being a regular expression to catch all
*Help* buffer names of a function along the lines of

      (lambda (buf _alist)
        (with-current-buffer buf
          (derived-mode-p 'help-mode)))

> I'm not fond of these mini-languages because they're less expressive, they
> end up being only minimally less complicated and bug-prone, they can't
> automatically be byte-compiled for efficiency, and they can't automatically
> be byte-compiled for correctness/diagnostics.  If one makes a mistake,
> the backtrace is much more complicated.

I agree in principle, but this should be alleviated by using a lambda
function as a predicate.  The above check still works and can be used
anywhere you would use `buffer-match-p'.

> So these mini-languages may make sense to define filters in thunderbird or
> something, but throwing Elisp away here generally doesn't make sense to me.
>
> But there may be exceptions (although this project.el one doesn't seem one
> of them) so why don't you show examples of use of these new helpers and
> so we can compare side by side with the Elisp-only alternative.

I am biased, but I believe that the language could even find more use in
project.el, by having `project-buffers' just call `match-buffers' with a
special `buffer-match-p' predicate.  Here is a sketch of how that could
look like (I haven't tested it yet):


[-- Attachment #2: Type: text/plain, Size: 2001 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index ac278edd40..b55c245368 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -352,15 +352,28 @@ project--remote-file-names
                 (concat remote-id file))
               local-files))))
 
+(defun project-includes-buffer-p (buffer dir)
+  "Return non-nil if the `default-directory' of BUFFER is below DIR."
+  (file-in-directory-p
+   (buffer-local-value 'default-directory buffer)
+   dir))
+
+(defcustom project-buffer-conditions
+  '(and (or buffer-file-name
+            (derived-mode . compilation-mode)
+            (derived-mode . dired-mode)
+            (derived-mode . diff-mode)
+            (derived-mode . comint-mode)
+            (derived-mode . eshell-mode)
+            (derived-mode . change-log-mode))
+        project-includes-buffer-p)
+  "A buffer predicate for matching what buffers belong to a project."
+  :type 'buffer-predicate)
+
 (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)))
-    (nreverse bufs)))
+  (let ((root (expand-file-name (file-name-as-directory (project-root project)))))
+    (match-buffers project-buffer-conditions nil root)))
 
 (defgroup project-vc nil
   "Project implementation based on the VC package."
@@ -679,7 +692,7 @@ project-buffers
                      (project--git-submodules))))
          dd
          bufs)
-    (dolist (buf (buffer-list))
+    (dolist (buf (cl-call-next-method))
       (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))

[-- Attachment #3: Type: text/plain, Size: 11 bytes --]


> João

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

[João Távora]

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

On Mon, Oct 31, 2022 at 10:51 PM Dmitry Gutov <dgutov@yandex.ru> wrote:

> I suggest you try it first.

It works in my test

> Disaster, really?

The reason I came about the Gnus problem was when using it
to reply to some emails here and trying out the C-x p k and finding out
all my Gnus buffers were gone.

> Do you know whether CIDER will be satisfied by the same patch I sent
> previously?

No.  I don't use it. You should ask Manuel, who reported this in
the original discussion.

> >>> you're making a gun that only backfires 5% of the time.
> >> Yours is the first instance so far.
> > We seem to use different algebraic systems.
> This is literally the first bug report on the subject.

It's Philip's bug report.  I don't use C-x p k, in fact I learned about it
here.  It's then I started trying it, because in principle it is very
useful,
that I found out how broken it is for many other situations: ibuffer,
*gnus*,
and it's a shame we can't use it.

> what you are doing is pressuring all other participants into your POV by
> means of an insult. That usually works better if the offending code was
> written by somebody who already left (the project/the discussion/the
> company/etc), or is a little younger.

You are attributing ill-intent to me from a moral high-ground you can't
really
claim.  I had no idea as to the authorship, so I can't have been engaging
in those
perfidious tactics.  But do I apologize for the word "abomination". If it
helps
I'll show you round to plenty such things written by myself in the past.

I've explained to Philip objective reasons why I think evaluated
mini-languages are almost always inferior to a decent Lisp such as Elisp.
You could perfectly reasonably deprecate these two variables.

> They can, though, even if odds are low. It can also happen through some
> other automation, which Emacs lets the users do freely.

That automation is just as buggy as C-x p k, and should be fixed, but
the only such automation we know is C-x p k.

> I'm fairly sure that the solution I offered would be easy enough
> implement, to actually protect the vulnerable buffer.
> I suppose we are not doing that, however.

You sketched an untested code-less idea and I explained how flawed it was.
Not to mention it's just [insert acceptably derogatory word here] to protect
implementation details from misbehaving code that is under our control
and can just be fixed.

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

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

[João Távora]

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

On Tue, Nov 1, 2022 at 11:23 AM Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 01.11.2022 12:59, João Távora wrote:
> > For me, it looks like match-buffers is reinventing
> > cl-remove-if-not and match-buffer-p is reinventing ... unary predicate
> > function of a buffer?
>
> And Eglot is reinventing url-http.
>

Really? Where? I don't understand how, might it might very well be.
Can suggest an improvement? I love to kill code, so let 'er rip.

João

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

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

[João Távora]

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

On Tue, Nov 1, 2022 at 11:27 AM Philip Kaludercic <philipk@posteo.net>
wrote:


> E.g. `display-buffer-alist' makes use of it to associate display-buffer
> rules with buffers.  Now you can add
>
>       ((major-mode . help-mode) display-buffer-in-side-window)
>
> instead of trying to match being a regular expression to catch all
> *Help* buffer names of a function along the lines of
>
>       (lambda (buf _alist)
>         (with-current-buffer buf
>           (derived-mode-p 'help-mode)))
>

If you really want to save up on this typing, it's better to define
a reusable helper function, or even a higher order function.

  (defun buffer-mode-matcher (mode)
    (lambda (b _alist)
      (with-current-buffer b (derived-mode-p 'help-mode))))

You can add buffer-mode-matcher to the library if it becomes
useful enough.  Then you add:

  `(,(buffer-mode-matcher 'help-mode) display-buffer-in-side-window)

to display-buffer-alist.

But if you really want a new language your language, then I suggest
a simple adapter buffer-matcher utility that merges the two.  That way one
doesn't couple existing utilities to the new mini-language and
simultaneously
the new mini-language become useful in a much wider setting for those who
appreciate such things.

  (defun buffer-matcher (condition)
     "Return unary predicate of a buffer matching the CONDITION
mini-language."
    (lambda (buf &rest _whatever) ; make it even more lax
       (buffer-match-p condition)))

Later on, you might even pass an (... &optional compiled) so that the
return value
is syntax checked and optimized in some way at compile time.

IOW, (E)Lisp already gives you the tools for these composition without
needing to invent new languages with the drawbacks I listed.

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

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 Tue, Nov 1, 2022 at 11:27 AM Philip Kaludercic <philipk@posteo.net>
> wrote:
>
>
>> E.g. `display-buffer-alist' makes use of it to associate display-buffer
>> rules with buffers.  Now you can add
>>
>>       ((major-mode . help-mode) display-buffer-in-side-window)
>>
>> instead of trying to match being a regular expression to catch all
>> *Help* buffer names of a function along the lines of
>>
>>       (lambda (buf _alist)
>>         (with-current-buffer buf
>>           (derived-mode-p 'help-mode)))
>>
>
> If you really want to save up on this typing, it's better to define
> a reusable helper function, or even a higher order function.
>
>   (defun buffer-mode-matcher (mode)
>     (lambda (b _alist)
>       (with-current-buffer b (derived-mode-p 'help-mode))))
>
> You can add buffer-mode-matcher to the library if it becomes
> useful enough.  Then you add:
>
>   `(,(buffer-mode-matcher 'help-mode) display-buffer-in-side-window)
>
> to display-buffer-alist.
>
> But if you really want a new language your language, then I suggest
> a simple adapter buffer-matcher utility that merges the two.  That way one
> doesn't couple existing utilities to the new mini-language and
> simultaneously
> the new mini-language become useful in a much wider setting for those who
> appreciate such things.
>
>   (defun buffer-matcher (condition)
>      "Return unary predicate of a buffer matching the CONDITION
> mini-language."
>     (lambda (buf &rest _whatever) ; make it even more lax
>        (buffer-match-p condition)))
>
> Later on, you might even pass an (... &optional compiled) so that the
> return value
> is syntax checked and optimized in some way at compile time.
>
> IOW, (E)Lisp already gives you the tools for these composition without
> needing to invent new languages with the drawbacks I listed.

I was curious to try this out, and implemented something along the lines
of your suggestion.  The bad news is that it is at least 10 times slower
than the current implementation, that isn't even really optimised.
Perhaps I did something native and didn't see what is wrong, but here
are my notes:

--8<---------------cut here---------------start------------->8---
(defun translate-buffer-condition (condition)
  "Compile a CONDITION into a predicate function."
  (pcase-exhaustive condition
    ((or 't 'nil)
     (lambda (_buffer _arg)
       condition))
    ((pred stringp)
     (lambda (buffer _arg)
       (string-match-p condition (buffer-name buffer))))
    ((pred functionp)
     (if (eq 1 (cdr (func-arity condition)))
	 (lambda (buffer _arg)
	   (funcall condition buffer))
       condition))
    (`(major-mode . ,mode)
     (lambda (buffer _arg)
       (eq
	(buffer-local-value 'major-mode buffer)
	mode)))
    (`(derived-mode . ,mode)
     (lambda (buffer _arg)
       (provided-mode-derived-p
	(buffer-local-value 'major-mode buffer)
	mode)))
    (`(not . ,cond)
     (lambda (buffer arg)
       (not (funcall (translate-buffer-condition cond) buffer arg))))
    (`(or . ,conds)
     (lambda (buffer arg)
       (catch 'match
	 (dolist (cond conds)
	   (when (funcall (translate-buffer-condition cond) buffer arg)
	     (throw 'match t))))))
    (`(and . ,conds)
     (lambda (buffer arg)
       (catch 'match
	 (dolist (cond conds t)
	   (when (funcall (translate-buffer-condition cond) buffer arg)
	     (throw 'match nil))))))))

(defvar buffer-match-p-cache (make-hash-table :test 'eq))

(defun buffer-match-p/compiled (condition buffer-or-name &optional arg)
  "Return non-nil if BUFFER-OR-NAME matches CONDITION.
CONDITION is either:
- the symbol t, to always match,
- the symbol nil, which never matches,
- a regular expression, to match a buffer name,
- a predicate function that takes a buffer object and ARG as
  arguments, and returns non-nil if the buffer matches,
- a cons-cell, where the car describes how to interpret the cdr.
  The car can be one of the following:
  * `derived-mode': the buffer matches if the buffer's major mode
    is derived from the major mode in the cons-cell's cdr.
  * `major-mode': the buffer matches if the buffer's major mode
    is eq to the cons-cell's cdr.  Prefer using `derived-mode'
    instead when both can work.
  * `not': the cdr is interpreted as a negation of a condition.
  * `and': the cdr is a list of recursive conditions, that all have
    to be met.
  * `or': the cdr is a list of recursive condition, of which at
    least one has to be met."
  (funcall (or (gethash condition buffer-match-p-cache)
	       (puthash condition
			(byte-compile (translate-buffer-condition condition))
			buffer-match-p-cache))
	   (get-buffer buffer-or-name)
	   arg))

(defun match-buffers/compiled (condition &optional buffers arg)
  "Return a list of buffers that match CONDITION.
See `buffer-match' for details on CONDITION.  By default all
buffers are checked, this can be restricted by passing an
optional argument BUFFERS, set to a list of buffers to check.
ARG is passed to `buffer-match', for predicate conditions in
CONDITION."
  (let (bufs)
    (dolist (buf (or buffers (buffer-list)))
      (when (buffer-match-p/compiled condition (get-buffer buf) arg)
        (push buf bufs)))
    bufs))

;; Here we will test a moderately complicated condition and time how
;; long it takes with the current implementation and with the proposed
;; alternative.

(defvar sample-condition
  '(and (or buffer-file-name
	    (derived-mode . compilation-mode)
	    (derived-mode . dired-mode)
	    (derived-mode . diff-mode)
	    (derived-mode . comint-mode)
	    (derived-mode . eshell-mode)
	    (derived-mode . change-log-mode))
	"\\*.+\\*"
	(not . "\\` ")))

(benchmark-run 100
  (match-buffers sample-condition pr))
;; => (1.7045469830000002 20 1.1418286690000023)


(benchmark-run 1000
  (match-buffers/compiled project-buffer-conditions pr))
;; => (17.646938126000002 219 12.428946030999999)
--8<---------------cut here---------------end--------------->8---

I guess this just goes to show that one shouldn't underestimate the cost
of a function call...

    LISP programmers know the value of everything and the cost of nothing.
         --  Alan Perlis 

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

[Dmitry Gutov]

On 01.11.2022 01:19, João Távora wrote:
> We can try to make some backward compatible way, but this seems like the
> perfect occasion to activate this most reasonable header in project.el:
> 
>      ;; NOTE: The project API is still experimental and can change in major,
>      ;; backward-incompatible ways.  Everyone is encouraged to try it, and
>      ;; report to us any problems or use cases we hadn't anticipated, by
>      ;; sending an email to emacs-devel, or `M-x report-emacs-bug'.

This is about the API.

project-kills-buffers is not part of it. It's just a regular user-level 
command, to be evolved how we do with regular commands.

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

[João Távora]

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

I haven't studied your code in depth, but it seems like you're giving
`match-buffers/compiled` benchmark 10 times the work you're giving to
the other function, which would explain why it's 10x slower.

The byte-compiler (or the native compiler) can't really optimize the
mini-language more magically.  It can only optimize elisp.

My idea of using the byte-compiler to do this is different: it entails
translating the mini-language to elisp first and then byte-compiling
that.  But it is a technique that I think your code isn't applying
or at least not correctly (though I haven't read all of it: I will soon).

You can see eglot's "glob matching" section for the application of
such a technique the "glob" minilanguage that is required by LSP (iow
it wasn't "invented by me" ;-) )

João

On Tue, Nov 1, 2022 at 1:03 PM Philip Kaludercic <philipk@posteo.net> wrote:

> João Távora <joaotavora@gmail.com> writes:
>
> > On Tue, Nov 1, 2022 at 11:27 AM Philip Kaludercic <philipk@posteo.net>
> > wrote:
> >
> >
> >> E.g. `display-buffer-alist' makes use of it to associate display-buffer
> >> rules with buffers.  Now you can add
> >>
> >>       ((major-mode . help-mode) display-buffer-in-side-window)
> >>
> >> instead of trying to match being a regular expression to catch all
> >> *Help* buffer names of a function along the lines of
> >>
> >>       (lambda (buf _alist)
> >>         (with-current-buffer buf
> >>           (derived-mode-p 'help-mode)))
> >>
> >
> > If you really want to save up on this typing, it's better to define
> > a reusable helper function, or even a higher order function.
> >
> >   (defun buffer-mode-matcher (mode)
> >     (lambda (b _alist)
> >       (with-current-buffer b (derived-mode-p 'help-mode))))
> >
> > You can add buffer-mode-matcher to the library if it becomes
> > useful enough.  Then you add:
> >
> >   `(,(buffer-mode-matcher 'help-mode) display-buffer-in-side-window)
> >
> > to display-buffer-alist.
> >
> > But if you really want a new language your language, then I suggest
> > a simple adapter buffer-matcher utility that merges the two.  That way
> one
> > doesn't couple existing utilities to the new mini-language and
> > simultaneously
> > the new mini-language become useful in a much wider setting for those who
> > appreciate such things.
> >
> >   (defun buffer-matcher (condition)
> >      "Return unary predicate of a buffer matching the CONDITION
> > mini-language."
> >     (lambda (buf &rest _whatever) ; make it even more lax
> >        (buffer-match-p condition)))
> >
> > Later on, you might even pass an (... &optional compiled) so that the
> > return value
> > is syntax checked and optimized in some way at compile time.
> >
> > IOW, (E)Lisp already gives you the tools for these composition without
> > needing to invent new languages with the drawbacks I listed.
>
> I was curious to try this out, and implemented something along the lines
> of your suggestion.  The bad news is that it is at least 10 times slower
> than the current implementation, that isn't even really optimised.
> Perhaps I did something native and didn't see what is wrong, but here
> are my notes:
>
> --8<---------------cut here---------------start------------->8---
> (defun translate-buffer-condition (condition)
>   "Compile a CONDITION into a predicate function."
>   (pcase-exhaustive condition
>     ((or 't 'nil)
>      (lambda (_buffer _arg)
>        condition))
>     ((pred stringp)
>      (lambda (buffer _arg)
>        (string-match-p condition (buffer-name buffer))))
>     ((pred functionp)
>      (if (eq 1 (cdr (func-arity condition)))
>          (lambda (buffer _arg)
>            (funcall condition buffer))
>        condition))
>     (`(major-mode . ,mode)
>      (lambda (buffer _arg)
>        (eq
>         (buffer-local-value 'major-mode buffer)
>         mode)))
>     (`(derived-mode . ,mode)
>      (lambda (buffer _arg)
>        (provided-mode-derived-p
>         (buffer-local-value 'major-mode buffer)
>         mode)))
>     (`(not . ,cond)
>      (lambda (buffer arg)
>        (not (funcall (translate-buffer-condition cond) buffer arg))))
>     (`(or . ,conds)
>      (lambda (buffer arg)
>        (catch 'match
>          (dolist (cond conds)
>            (when (funcall (translate-buffer-condition cond) buffer arg)
>              (throw 'match t))))))
>     (`(and . ,conds)
>      (lambda (buffer arg)
>        (catch 'match
>          (dolist (cond conds t)
>            (when (funcall (translate-buffer-condition cond) buffer arg)
>              (throw 'match nil))))))))
>
> (defvar buffer-match-p-cache (make-hash-table :test 'eq))
>
> (defun buffer-match-p/compiled (condition buffer-or-name &optional arg)
>   "Return non-nil if BUFFER-OR-NAME matches CONDITION.
> CONDITION is either:
> - the symbol t, to always match,
> - the symbol nil, which never matches,
> - a regular expression, to match a buffer name,
> - a predicate function that takes a buffer object and ARG as
>   arguments, and returns non-nil if the buffer matches,
> - a cons-cell, where the car describes how to interpret the cdr.
>   The car can be one of the following:
>   * `derived-mode': the buffer matches if the buffer's major mode
>     is derived from the major mode in the cons-cell's cdr.
>   * `major-mode': the buffer matches if the buffer's major mode
>     is eq to the cons-cell's cdr.  Prefer using `derived-mode'
>     instead when both can work.
>   * `not': the cdr is interpreted as a negation of a condition.
>   * `and': the cdr is a list of recursive conditions, that all have
>     to be met.
>   * `or': the cdr is a list of recursive condition, of which at
>     least one has to be met."
>   (funcall (or (gethash condition buffer-match-p-cache)
>                (puthash condition
>                         (byte-compile (translate-buffer-condition
> condition))
>                         buffer-match-p-cache))
>            (get-buffer buffer-or-name)
>            arg))
>
> (defun match-buffers/compiled (condition &optional buffers arg)
>   "Return a list of buffers that match CONDITION.
> See `buffer-match' for details on CONDITION.  By default all
> buffers are checked, this can be restricted by passing an
> optional argument BUFFERS, set to a list of buffers to check.
> ARG is passed to `buffer-match', for predicate conditions in
> CONDITION."
>   (let (bufs)
>     (dolist (buf (or buffers (buffer-list)))
>       (when (buffer-match-p/compiled condition (get-buffer buf) arg)
>         (push buf bufs)))
>     bufs))
>
> ;; Here we will test a moderately complicated condition and time how
> ;; long it takes with the current implementation and with the proposed
> ;; alternative.
>
> (defvar sample-condition
>   '(and (or buffer-file-name
>             (derived-mode . compilation-mode)
>             (derived-mode . dired-mode)
>             (derived-mode . diff-mode)
>             (derived-mode . comint-mode)
>             (derived-mode . eshell-mode)
>             (derived-mode . change-log-mode))
>         "\\*.+\\*"
>         (not . "\\` ")))
>
> (benchmark-run 100
>   (match-buffers sample-condition pr))
> ;; => (1.7045469830000002 20 1.1418286690000023)
>
>
> (benchmark-run 1000
>   (match-buffers/compiled project-buffer-conditions pr))
> ;; => (17.646938126000002 219 12.428946030999999)
> --8<---------------cut here---------------end--------------->8---
>
> I guess this just goes to show that one shouldn't underestimate the cost
> of a function call...
>
>     LISP programmers know the value of everything and the cost of nothing.
>          --  Alan Perlis
>


-- 
João Távora

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

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

[João Távora]

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

On Tue, Nov 1, 2022 at 1:22 PM Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 01.11.2022 01:19, João Távora wrote:
> > We can try to make some backward compatible way, but this seems like the
> > perfect occasion to activate this most reasonable header in project.el:
> >
> >      ;; NOTE: The project API is still experimental and can change in
> major,
> >      ;; backward-incompatible ways.  Everyone is encouraged to try it,
> and
> >      ;; report to us any problems or use cases we hadn't anticipated, by
> >      ;; sending an email to emacs-devel, or `M-x report-emacs-bug'.
>
> This is about the API.
>
> project-kills-buffers is not part of it. It's just a regular user-level
> command, to be evolved how we do with regular commands.


project-kill-buffers is great: I don't mean to deprecate it change it,
its protocol etc.  If "evolution" means fixing what are demonstrably
its very salient bugs, that's great.

Philip's worry was about project-kill-buffer-conditions.  That variable
contains a programming language and could be deprecated (if we
arrive at the conclusion that it is easily replaced by something else).
We shouldn't be shy of doing so a backward-incompatible way, given that
its usage is estimated to be minimal (and this estimation is backed up
by real data, not just a guess).

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

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:

> I haven't studied your code in depth, but it seems like you're giving
> `match-buffers/compiled` benchmark 10 times the work you're giving to
> the other function, which would explain why it's 10x slower.
>

You are absolutely right, I re-ran the tests the right way and got these
results:

--8<---------------cut here---------------start------------->8---
(benchmark-run 1000
  (match-buffers sample-condition))
;; => (25.052781603 265 16.678798381999997)

(benchmark-run 1000
  (match-buffers/compiled sample-condition))
;; (30.021295067 335 21.01291473699999)
--8<---------------cut here---------------end--------------->8---

> The byte-compiler (or the native compiler) can't really optimize the
> mini-language more magically.  It can only optimize elisp.

Of course, I don't get why it should?

> My idea of using the byte-compiler to do this is different: it entails
> translating the mini-language to elisp first and then byte-compiling
> that.  But it is a technique that I think your code isn't applying
> or at least not correctly (though I haven't read all of it: I will soon).

What I am doing is translating it into lambda expressions, but I could
also try out translating it into an s-expression and passing that to
`eval'...

> You can see eglot's "glob matching" section for the application of
> such a technique the "glob" minilanguage that is required by LSP (iow
> it wasn't "invented by me" ;-) )

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

[João Távora]

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

On Tue, Nov 1, 2022 at 2:00 PM Philip Kaludercic <philipk@posteo.net> wrote:


> > My idea of using the byte-compiler to do this is different: it entails
> > translating the mini-language to elisp first and then byte-compiling
> > that.  But it is a technique that I think your code isn't applying
> > or at least not correctly (though I haven't read all of it: I will soon).
>
> What I am doing is translating it into lambda expressions, but I could
> also try out translating it into an s-expression and passing that to
> `eval'...
>

Yes, do that, but use byte-compile instead, not eval.

João Távora

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

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 Tue, Nov 1, 2022 at 2:00 PM Philip Kaludercic <philipk@posteo.net> wrote:
>
>
>> > My idea of using the byte-compiler to do this is different: it entails
>> > translating the mini-language to elisp first and then byte-compiling
>> > that.  But it is a technique that I think your code isn't applying
>> > or at least not correctly (though I haven't read all of it: I will soon).
>>
>> What I am doing is translating it into lambda expressions, but I could
>> also try out translating it into an s-expression and passing that to
>> `eval'...
>>
>
> Yes, do that, but use byte-compile instead, not eval.

I have tried both, and it doesn't appear to be a particular advantage
one way or another.  That being said, this approach is *a lot* faster,
to the point that I first assumed it was broken:

--8<---------------cut here---------------start------------->8---
(benchmark-run 1000
  (match-buffers/compiled sample-condition))
;; (0.469515746 5 0.3328363100000047)
--8<---------------cut here---------------end--------------->8---

but it works:

--8<---------------cut here---------------start------------->8---
(equal (match-buffers sample-condition) (match-buffers/compiled sample-condition))
;; => t
--8<---------------cut here---------------end--------------->8---

So this is certainly something worth considering as a replacement!

Here is the implementation:

--8<---------------cut here---------------start------------->8---
(defvar translate-buffer-condition-buffer-sym (make-symbol "buffer"))
(defvar translate-buffer-condition-arg-sym (make-symbol "arg"))

(defun translate-buffer-condition (condition)
  "Compile a CONDITION into a predicate function."
  (pcase-exhaustive condition
    ((or 't 'nil)
     condition)
    ((pred stringp)
     `(string-match-p ,condition (buffer-name ,translate-buffer-condition-buffer-sym)))
    ((pred functionp)
     (if (eq 1 (cdr (func-arity condition)))
         ;; FIXME: is `funcall' necessary here?
	 `(funcall #',condition ,translate-buffer-condition-buffer-sym)
       `(funcall ,condition
		 ,translate-buffer-condition-buffer-sym
		 ,translate-buffer-condition-arg-sym)))
    (`(major-mode . ,mode)
     `(eq (buffer-local-value 'major-mode ,translate-buffer-condition-buffer-sym)
	  ',mode))
    (`(derived-mode . ,mode)
     `(provided-mode-derived-p
       (buffer-local-value 'major-mode ,translate-buffer-condition-buffer-sym)
       ',mode))
    (`(not . ,cond)
     `(not ,(translate-buffer-condition cond)))
    (`(or . ,conds)
     `(or ,@(mapcar #'translate-buffer-condition conds)))
    (`(and . ,conds)
     `(and ,@(mapcar #'translate-buffer-condition conds)))))

(defvar buffer-match-p-cache (make-hash-table :test 'eq))

(defun buffer-match-p/evaled (condition buffer-or-name &optional arg)
  "Return non-nil if BUFFER-OR-NAME matches CONDITION.
CONDITION is either:
- the symbol t, to always match,
- the symbol nil, which never matches,
- a regular expression, to match a buffer name,
- a predicate function that takes a buffer object and ARG as
  arguments, and returns non-nil if the buffer matches,
- a cons-cell, where the car describes how to interpret the cdr.
  The car can be one of the following:
  * `derived-mode': the buffer matches if the buffer's major mode
    is derived from the major mode in the cons-cell's cdr.
  * `major-mode': the buffer matches if the buffer's major mode
    is eq to the cons-cell's cdr.  Prefer using `derived-mode'
    instead when both can work.
  * `not': the cdr is interpreted as a negation of a condition.
  * `and': the cdr is a list of recursive conditions, that all have
    to be met.
  * `or': the cdr is a list of recursive condition, of which at
    least one has to be met."
  (eval (or (gethash condition buffer-match-p-cache)
	    (puthash condition
		     (translate-buffer-condition condition)
		     buffer-match-p-cache))
	`((,translate-buffer-condition-buffer-sym . ,(get-buffer buffer-or-name))
	  (,translate-buffer-condition-arg-sym . ,arg))))

;; Alternative implementation using `byte-compile':
;;
;; (funcall (or (gethash condition buffer-match-p-cache)
;; 	       (puthash condition
;; 			(byte-compile
;; 			 `(lambda (,translate-buffer-condition-buffer-sym
;; 				   ,translate-buffer-condition-arg-sym)
;; 			    ,(translate-buffer-condition condition)))
;; 			buffer-match-p-cache))
;; 	   (get-buffer buffer-or-name) arg)
--8<---------------cut here---------------end--------------->8---

In the end, the second implementation using `byte-compile' might be
preferable as it avoids using `eval'.

> João Távora

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

[Dmitry Gutov]

On 01.11.2022 13:27, Philip Kaludercic wrote:

> I am biased, but I believe that the language could even find more use in
> project.el, by having `project-buffers' just call `match-buffers' with a
> special `buffer-match-p' predicate.  Here is a sketch of how that could
> look like (I haven't tested it yet):

Sure, but only after we're ready to drop project.el support for Emacs 
older than 29.

> 
> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index ac278edd40..b55c245368 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -352,15 +352,28 @@ project--remote-file-names
>                   (concat remote-id file))
>                 local-files))))
>   
> +(defun project-includes-buffer-p (buffer dir)
> +  "Return non-nil if the `default-directory' of BUFFER is below DIR."
> +  (file-in-directory-p
> +   (buffer-local-value 'default-directory buffer)
> +   dir))

Not an optimal name, given that we have made project-buffers a generic 
function, so that a custom project backend can define their own 
buffer-listing strategy. And this one implies that matching by 
default-directory is universal.

> +(defcustom project-buffer-conditions

Why not keep considering unknown buffers as part of project by default?

We'll just stop killing them on cleanup.

Otherwise, we'll really need an extensible mechanism for major modes all 
around the ecosystem to tag themselves as project-visible.

> +  '(and (or buffer-file-name
> +            (derived-mode . compilation-mode)
> +            (derived-mode . dired-mode)
> +            (derived-mode . diff-mode)
> +            (derived-mode . comint-mode)
> +            (derived-mode . eshell-mode)
> +            (derived-mode . change-log-mode))
> +        project-includes-buffer-p)
> +  "A buffer predicate for matching what buffers belong to a project."
> +  :type 'buffer-predicate)

Let's not forget Xref, Occur, VC-Dir, Log-View. Maybe some others.

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

[Dmitry Gutov]

On 01.11.2022 13:39, João Távora wrote:
> On Tue, Nov 1, 2022 at 11:23 AM Dmitry Gutov <dgutov@yandex.ru 
> <mailto:dgutov@yandex.ru>> wrote:
> 
>     On 01.11.2022 12:59, João Távora wrote:
>      > For me, it looks like match-buffers is reinventing
>      > cl-remove-if-not and match-buffer-p is reinventing ... unary
>     predicate
>      > function of a buffer?
> 
>     And Eglot is reinventing url-http.
> 
> 
> Really? Where? I don't understand how, might it might very well be.
> Can suggest an improvement? I love to kill code, so let 'er rip.

That's an unfair expectation: you didn't suggest an improvement either.

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

[João Távora]

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

On Tue, Nov 1, 2022, 15:27 Dmitry Gutov <dgutov@yandex.ru> wrote:

That's an unfair expectation: you didn't suggest an improvement either.
>

Oh, I'm sorry, and is it fair to inquire about the subliminally compressed:
"and eglot reinvents http-url"? Can you generously grant a few more
sentences as to its meaning and relevance?

Much obliged,
João

>

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

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

[Philip Kaludercic]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 01.11.2022 13:27, Philip Kaludercic wrote:
>
>> I am biased, but I believe that the language could even find more use in
>> project.el, by having `project-buffers' just call `match-buffers' with a
>> special `buffer-match-p' predicate.  Here is a sketch of how that could
>> look like (I haven't tested it yet):
>
> Sure, but only after we're ready to drop project.el support for Emacs
> older than 29.

The functionality can be provided using Compat[0], as is already done for a
few core package that are distributed on GNU ELPA (ERC, python-mode).

[0] https://elpa.gnu.org/packages/compat.html

>> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
>> index ac278edd40..b55c245368 100644
>> --- a/lisp/progmodes/project.el
>> +++ b/lisp/progmodes/project.el
>> @@ -352,15 +352,28 @@ project--remote-file-names
>>                   (concat remote-id file))
>>                 local-files))))
>>   +(defun project-includes-buffer-p (buffer dir)
>> +  "Return non-nil if the `default-directory' of BUFFER is below DIR."
>> +  (file-in-directory-p
>> +   (buffer-local-value 'default-directory buffer)
>> +   dir))
>
> Not an optimal name, given that we have made project-buffers a generic
> function, so that a custom project backend can define their own
> buffer-listing strategy. And this one implies that matching by
> default-directory is universal.

Right, as I said this is just a sketch.

But as the diff showed, I think any more specific implementation ought
to extend the generic implementation, by using `cl-call-next-method'
instead of `buffer-list'.

>> +(defcustom project-buffer-conditions
>
> Why not keep considering unknown buffers as part of project by default?

What are "unknown buffers"?

> We'll just stop killing them on cleanup.
>
> Otherwise, we'll really need an extensible mechanism for major modes
> all around the ecosystem to tag themselves as project-visible.

Wouldn't a simple buffer local variable suffice?

>> +  '(and (or buffer-file-name
>> +            (derived-mode . compilation-mode)
>> +            (derived-mode . dired-mode)
>> +            (derived-mode . diff-mode)
>> +            (derived-mode . comint-mode)
>> +            (derived-mode . eshell-mode)
>> +            (derived-mode . change-log-mode))
>> +        project-includes-buffer-p)
>> +  "A buffer predicate for matching what buffers belong to a project."
>> +  :type 'buffer-predicate)
>
> Let's not forget Xref, Occur, VC-Dir, Log-View. Maybe some others.

This is my point, I think João is right that this ought to be an
enumeration of major modes that are related to projects.  As this is a
user option, users can add or remove whatever modes they disagree on and
that behaviour would ideally be propagated to all projects.

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

[Dmitry Gutov]

On 01.11.2022 20:44, Philip Kaludercic wrote:

>> Sure, but only after we're ready to drop project.el support for Emacs
>> older than 29.
> 
> The functionality can be provided using Compat[0], as is already done for a
> few core package that are distributed on GNU ELPA (ERC, python-mode).
> 
> [0] https://elpa.gnu.org/packages/compat.html

I suppose if the performance improvement is shown to be significant, we 
could. I'm a little hesitant to add a new dependency: I haven't been 
following this package, not sure how stable it is.

>>> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
>>> index ac278edd40..b55c245368 100644
>>> --- a/lisp/progmodes/project.el
>>> +++ b/lisp/progmodes/project.el
>>> @@ -352,15 +352,28 @@ project--remote-file-names
>>>                    (concat remote-id file))
>>>                  local-files))))
>>>    +(defun project-includes-buffer-p (buffer dir)
>>> +  "Return non-nil if the `default-directory' of BUFFER is below DIR."
>>> +  (file-in-directory-p
>>> +   (buffer-local-value 'default-directory buffer)
>>> +   dir))
>>
>> Not an optimal name, given that we have made project-buffers a generic
>> function, so that a custom project backend can define their own
>> buffer-listing strategy. And this one implies that matching by
>> default-directory is universal.
> 
> Right, as I said this is just a sketch.
> 
> But as the diff showed, I think any more specific implementation ought
> to extend the generic implementation, by using `cl-call-next-method'
> instead of `buffer-list'.

Or apply the same filters some other way, I guess. But yes, the 
cl-call-next-method call makes sense in your patch.

>>> +(defcustom project-buffer-conditions
>>
>> Why not keep considering unknown buffers as part of project by default?
> 
> What are "unknown buffers"?

Take whatever special buffer belonging to jsonrpc that was the cause of 
this bug report. It can still be useful to be able switch to it using 
project-switch-to-buffer (if, say, one was looking for the specific 
buffer to try to debug a problem with some Eglot feature in their 
project). We don't want to kill it with the rest of the buffers, 
however. Apparently.

>> We'll just stop killing them on cleanup.
>>
>> Otherwise, we'll really need an extensible mechanism for major modes
>> all around the ecosystem to tag themselves as project-visible.
> 
> Wouldn't a simple buffer local variable suffice?

I guess it will. Only with a more meaningful name than 'project-owned' 
and some proper documentation.

>>> +  '(and (or buffer-file-name
>>> +            (derived-mode . compilation-mode)
>>> +            (derived-mode . dired-mode)
>>> +            (derived-mode . diff-mode)
>>> +            (derived-mode . comint-mode)
>>> +            (derived-mode . eshell-mode)
>>> +            (derived-mode . change-log-mode))
>>> +        project-includes-buffer-p)
>>> +  "A buffer predicate for matching what buffers belong to a project."
>>> +  :type 'buffer-predicate)
>>
>> Let's not forget Xref, Occur, VC-Dir, Log-View. Maybe some others.
> 
> This is my point, I think João is right that this ought to be an
> enumeration of major modes that are related to projects.  As this is a
> user option, users can add or remove whatever modes they disagree on and
> that behaviour would ideally be propagated to all projects.

Being to customize it is a good thing.

But either we provide a reasonably complete list which we regularly 
update, or we say its completeness is up to the user.

And in the latter case, as soon as the user customizes the var, they 
stop getting any updates we might make to it later (to the default value).

And if we take the strict "whitelist" approach, I'm pretty sure the list 
will require updating in the future, it will never be complete.

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

[Philip Kaludercic]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 01.11.2022 20:44, Philip Kaludercic wrote:
>
>>> Sure, but only after we're ready to drop project.el support for Emacs
>>> older than 29.
>> The functionality can be provided using Compat[0], as is already
>> done for a
>> few core package that are distributed on GNU ELPA (ERC, python-mode).
>> [0] https://elpa.gnu.org/packages/compat.html
>
> I suppose if the performance improvement is shown to be significant,
> we could. I'm a little hesitant to add a new dependency: I haven't
> been following this package, not sure how stable it is.

I am the maintainer, so I am biased, but stability is a high priority,
which is why the library is extensively tested, where-ever possible:

https://git.sr.ht/~pkal/compat/tree/master/item/compat-tests.el

Fun fact, I came up with the idea for this library when working on
project-kill-buffer over two years ago, as a means of extracting the
language out of project.el, as has been done with buffer-match-p.

>>>> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
>>>> index ac278edd40..b55c245368 100644
>>>> --- a/lisp/progmodes/project.el
>>>> +++ b/lisp/progmodes/project.el
>>>> @@ -352,15 +352,28 @@ project--remote-file-names
>>>>                    (concat remote-id file))
>>>>                  local-files))))
>>>>    +(defun project-includes-buffer-p (buffer dir)
>>>> +  "Return non-nil if the `default-directory' of BUFFER is below DIR."
>>>> +  (file-in-directory-p
>>>> +   (buffer-local-value 'default-directory buffer)
>>>> +   dir))
>>>
>>> Not an optimal name, given that we have made project-buffers a generic
>>> function, so that a custom project backend can define their own
>>> buffer-listing strategy. And this one implies that matching by
>>> default-directory is universal.
>> Right, as I said this is just a sketch.
>> But as the diff showed, I think any more specific implementation
>> ought
>> to extend the generic implementation, by using `cl-call-next-method'
>> instead of `buffer-list'.
>
> Or apply the same filters some other way, I guess. But yes, the
> cl-call-next-method call makes sense in your patch.
>
>>>> +(defcustom project-buffer-conditions
>>>
>>> Why not keep considering unknown buffers as part of project by default?
>> What are "unknown buffers"?
>
> Take whatever special buffer belonging to jsonrpc that was the cause
> of this bug report. It can still be useful to be able switch to it
> using project-switch-to-buffer (if, say, one was looking for the
> specific buffer to try to debug a problem with some Eglot feature in
> their project). We don't want to kill it with the rest of the buffers,
> however. Apparently.

Ah, right.  I have to admit that I rarely use project-switch-to-buffer,
so I forgot about that.  In that case, project-kill-buffer-conditions
cannot be deprecated, as these are just a subset of the project buffers.

>>> We'll just stop killing them on cleanup.
>>>
>>> Otherwise, we'll really need an extensible mechanism for major modes
>>> all around the ecosystem to tag themselves as project-visible.
>> Wouldn't a simple buffer local variable suffice?
>
> I guess it will. Only with a more meaningful name than 'project-owned'
> and some proper documentation.

Right.  Does it have to have a "project-" prefix, or would
"belongs-to-project" (hypothetically) be fine too?

>>>> +  '(and (or buffer-file-name
>>>> +            (derived-mode . compilation-mode)
>>>> +            (derived-mode . dired-mode)
>>>> +            (derived-mode . diff-mode)
>>>> +            (derived-mode . comint-mode)
>>>> +            (derived-mode . eshell-mode)
>>>> +            (derived-mode . change-log-mode))
>>>> +        project-includes-buffer-p)
>>>> +  "A buffer predicate for matching what buffers belong to a project."
>>>> +  :type 'buffer-predicate)
>>>
>>> Let's not forget Xref, Occur, VC-Dir, Log-View. Maybe some others.
>> This is my point, I think João is right that this ought to be an
>> enumeration of major modes that are related to projects.  As this is a
>> user option, users can add or remove whatever modes they disagree on and
>> that behaviour would ideally be propagated to all projects.
>
> Being to customize it is a good thing.
>
> But either we provide a reasonably complete list which we regularly
> update, or we say its completeness is up to the user.

I would want to argue that the complete list is preferable.

> And in the latter case, as soon as the user customizes the var, they
> stop getting any updates we might make to it later (to the default
> value).
>
> And if we take the strict "whitelist" approach, I'm pretty sure the
> list will require updating in the future, it will never be complete.

Which is fair, but which can also be combined with 

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

[Dmitry Gutov]

On 01.11.2022 13:36, João Távora wrote:

>  > what you are doing is pressuring all other participants into your POV by
>  > means of an insult. That usually works better if the offending code was
>  > written by somebody who already left (the project/the discussion/the
>  > company/etc), or is a little younger.
> 
> You are attributing ill-intent to me from a moral high-ground you can't 
> really
> claim.  I had no idea as to the authorship, so I can't have been 
> engaging in those
> perfidious tactics.  But do I apologize for the word "abomination". If 
> it helps
> I'll show you round to plenty such things written by myself in the past.

Not at all, trying to get people to agree to your own (obviously correct 
and beneficial) POV is not ill intent.

The means are not great, though.

> I've explained to Philip objective reasons why I think evaluated
> mini-languages are almost always inferior to a decent Lisp such as Elisp.
> You could perfectly reasonably deprecate these two variables.

Not where this discussion is going, is it?

>  > I'm fairly sure that the solution I offered would be easy enough
>  > implement, to actually protect the vulnerable buffer.
>  > I suppose we are not doing that, however.
> 
> You sketched an untested code-less idea and I explained how flawed it was.

Back atcha. Modulo "code-less".

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

[Dmitry Gutov]

On 01.11.2022 18:23, João Távora wrote:
> On Tue, Nov 1, 2022, 15:27 Dmitry Gutov <dgutov@yandex.ru 
> <mailto:dgutov@yandex.ru>> wrote:
> 
>     That's an unfair expectation: you didn't suggest an improvement either.
> 
> 
> Oh, I'm sorry, and is it fair to inquire about the subliminally 
> compressed: "and eglot reinvents http-url"? Can you generously grant a 
> few more sentences as to its meaning and relevance?

Your compressed the meaning and usefulness of a feature to something 
trivial (and only semi-relevant), and so did I.

I thought that would be obvious.

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

[Dmitry Gutov]

On 01.11.2022 22:10, Philip Kaludercic wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> On 01.11.2022 20:44, Philip Kaludercic wrote:
>>
>>>> Sure, but only after we're ready to drop project.el support for Emacs
>>>> older than 29.
>>> The functionality can be provided using Compat[0], as is already
>>> done for a
>>> few core package that are distributed on GNU ELPA (ERC, python-mode).
>>> [0] https://elpa.gnu.org/packages/compat.html
>>
>> I suppose if the performance improvement is shown to be significant,
>> we could. I'm a little hesitant to add a new dependency: I haven't
>> been following this package, not sure how stable it is.
> 
> I am the maintainer, so I am biased, but stability is a high priority,
> which is why the library is extensively tested, where-ever possible:
> 
> https://git.sr.ht/~pkal/compat/tree/master/item/compat-tests.el
> 
> Fun fact, I came up with the idea for this library when working on
> project-kill-buffer over two years ago, as a means of extracting the
> language out of project.el, as has been done with buffer-match-p.

Oh ok. I suppose I don't mind, then.

It would be nice to have some benchmark, however, at what number of 
buffers the optimization brings some perceptible difference (like, the 
delay is reduced by 50ms at least).

>>>>> +(defcustom project-buffer-conditions
>>>>
>>>> Why not keep considering unknown buffers as part of project by default?
>>> What are "unknown buffers"?
>>
>> Take whatever special buffer belonging to jsonrpc that was the cause
>> of this bug report. It can still be useful to be able switch to it
>> using project-switch-to-buffer (if, say, one was looking for the
>> specific buffer to try to debug a problem with some Eglot feature in
>> their project). We don't want to kill it with the rest of the buffers,
>> however. Apparently.
> 
> Ah, right.  I have to admit that I rarely use project-switch-to-buffer,
> so I forgot about that.  In that case, project-kill-buffer-conditions
> cannot be deprecated, as these are just a subset of the project buffers.

There can be other uses for 'project-buffers' as well, like the 
reimplementation of projectile-ibuffer in another bug report.

>>>> We'll just stop killing them on cleanup.
>>>>
>>>> Otherwise, we'll really need an extensible mechanism for major modes
>>>> all around the ecosystem to tag themselves as project-visible.
>>> Wouldn't a simple buffer local variable suffice?
>>
>> I guess it will. Only with a more meaningful name than 'project-owned'
>> and some proper documentation.
> 
> Right.  Does it have to have a "project-" prefix, or would
> "belongs-to-project" (hypothetically) be fine too?

Let's keep to the module-prefixing strategy. I think it makes sense, and 
the rest of the symbols in project.el are named that way.

>>>>> +  '(and (or buffer-file-name
>>>>> +            (derived-mode . compilation-mode)
>>>>> +            (derived-mode . dired-mode)
>>>>> +            (derived-mode . diff-mode)
>>>>> +            (derived-mode . comint-mode)
>>>>> +            (derived-mode . eshell-mode)
>>>>> +            (derived-mode . change-log-mode))
>>>>> +        project-includes-buffer-p)
>>>>> +  "A buffer predicate for matching what buffers belong to a project."
>>>>> +  :type 'buffer-predicate)
>>>>
>>>> Let's not forget Xref, Occur, VC-Dir, Log-View. Maybe some others.
>>> This is my point, I think João is right that this ought to be an
>>> enumeration of major modes that are related to projects.  As this is a
>>> user option, users can add or remove whatever modes they disagree on and
>>> that behaviour would ideally be propagated to all projects.
>>
>> Being to customize it is a good thing.
>>
>> But either we provide a reasonably complete list which we regularly
>> update, or we say its completeness is up to the user.
> 
> I would want to argue that the complete list is preferable.

Then we'll have to keep updating it from time to time.

>> And in the latter case, as soon as the user customizes the var, they
>> stop getting any updates we might make to it later (to the default
>> value).
>>
>> And if we take the strict "whitelist" approach, I'm pretty sure the
>> list will require updating in the future, it will never be complete.
> 
> Which is fair, but which can also be combined with

(unfinished)

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

[João Távora]

Philip Kaludercic <philipk@posteo.net> writes:

>> Yes, do that, but use byte-compile instead, not eval.
> I have tried both, and it doesn't appear to be a particular advantage
> one way or another.  That being said, this approach is *a lot* faster,
> to the point that I first assumed it was broken:

Yes, this approach is always going to be much faster than the "naive"
approach.  Now I've taken your code as a starting point, simplified it,
and I get a reasonable/typical 3.5x speedup when I use a
byte-compilation strategy, so one of us isn't measuring

```elisp
(defun translate-buffer-condition-1 (condition)
  (pcase-exhaustive condition
    ((or 't 'nil)
     condition)
    ((pred stringp)
     `(string-match-p ,condition (buffer-name buffer)))
    ((pred functionp)
     `(,condition buffer))
    (`(major-mode . ,mode)
     `(eq (buffer-local-value 'major-mode buffer)
	  ',mode))
    (`(derived-mode . ,mode)
     `(provided-mode-derived-p
       (buffer-local-value 'major-mode buffer)
       ',mode))
    (`(not . ,cond)
     `(not ,(translate-buffer-condition-1 cond)))
    (`(or . ,conds)
     `(or ,@(mapcar #'translate-buffer-condition-1 conds)))
    (`(and . ,conds)
     `(and ,@(mapcar #'translate-buffer-condition-1 conds)))))

(defun translate-buffer-condition (condition)
  `(lambda (buffer) ,(translate-buffer-condition-1 condition)))

(defvar sample-condition
  '(and (or buffer-file-name
	    (derived-mode . compilation-mode)
	    (derived-mode . dired-mode)
	    (derived-mode . diff-mode)
	    (derived-mode . comint-mode)
	    (derived-mode . eshell-mode)
	    (derived-mode . change-log-mode))
	"\\*.+\\*"
	(not . "\\` ")))

(defvar form (translate-buffer-condition sample-condition))
(defvar compiled (byte-compile form))

(benchmark-run 100000 (funcall (eval form) (current-buffer))) ;; (0.397404883 3 0.18942550900000032)
(benchmark-run 100000 (funcall compiled (current-buffer))) ;; (0.113651836 0 0.0)
```

I couldn't understand the need for a hash table or special symbol vars
or what that "arg" was, so I took it out, but it shouldn't make a
difference.

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:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>>> Yes, do that, but use byte-compile instead, not eval.
>> I have tried both, and it doesn't appear to be a particular advantage
>> one way or another.  That being said, this approach is *a lot* faster,
>> to the point that I first assumed it was broken:
>
> Yes, this approach is always going to be much faster than the "naive"
> approach.  Now I've taken your code as a starting point, simplified it,
> and I get a reasonable/typical 3.5x speedup when I use a
> byte-compilation strategy, so one of us isn't measuring
>
> ```elisp
> (defun translate-buffer-condition-1 (condition)
>   (pcase-exhaustive condition
>     ((or 't 'nil)
>      condition)
>     ((pred stringp)
>      `(string-match-p ,condition (buffer-name buffer)))
>     ((pred functionp)
>      `(,condition buffer))

This would break the current behaviour, because `buffer-match-p'
requires the function be called with an additional argument if possible.

This is fundamental for `display-buffer-alist' to work.

>     (`(major-mode . ,mode)
>      `(eq (buffer-local-value 'major-mode buffer)
> 	  ',mode))
>     (`(derived-mode . ,mode)
>      `(provided-mode-derived-p
>        (buffer-local-value 'major-mode buffer)
>        ',mode))
>     (`(not . ,cond)
>      `(not ,(translate-buffer-condition-1 cond)))
>     (`(or . ,conds)
>      `(or ,@(mapcar #'translate-buffer-condition-1 conds)))
>     (`(and . ,conds)
>      `(and ,@(mapcar #'translate-buffer-condition-1 conds)))))
>
> (defun translate-buffer-condition (condition)
>   `(lambda (buffer) ,(translate-buffer-condition-1 condition)))
>
> (defvar sample-condition
>   '(and (or buffer-file-name
> 	    (derived-mode . compilation-mode)
> 	    (derived-mode . dired-mode)
> 	    (derived-mode . diff-mode)
> 	    (derived-mode . comint-mode)
> 	    (derived-mode . eshell-mode)
> 	    (derived-mode . change-log-mode))
> 	"\\*.+\\*"
> 	(not . "\\` ")))
>
> (defvar form (translate-buffer-condition sample-condition))
> (defvar compiled (byte-compile form))
>
> (benchmark-run 100000 (funcall (eval form) (current-buffer))) ;; (0.397404883 3 0.18942550900000032)
> (benchmark-run 100000 (funcall compiled (current-buffer))) ;; (0.113651836 0 0.0)
> ```
>
> I couldn't understand the need for a hash table or special symbol vars
> or what that "arg" was, so I took it out, but it shouldn't make a
> difference.

The hash table makes a significant different, try evaluating

   (benchmark-run 100000 (funcall (byte-compile compiled) (current-buffer))) ;; (0.113651836 0 0.0)

I have created a new bug report for this issue bug#58950, so that this
one can return to the topic of project.el.

The fresh symbols are used to keep the code clean and avoid possible
naming conflicts. 

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> I've explained to Philip objective reasons why I think evaluated
>> mini-languages are almost always inferior to a decent Lisp such as Elisp.
>> You could perfectly reasonably deprecate these two variables.
> Not where this discussion is going, is it?

Not sure.  This started has a report of hidden buffer being incorrectly
killed by project.el.  After much insistence, you've agreed to plug the
hole in that library.  During tests I also discovered that project.el is
killing other buffers nonsensically, like Gnus buffers, *ibuffer*, and
many other global.  It was actually easier to find false-positives of
your heuristic than true ones.  Again, after some insistence, you seem
to have come around that these things represent bugs.

Three people here have suggested an opt-in approach for the true
positives.  Now your strategy seems to be "OK: let all these false
positives remain nonsensically associated with a project in
project-buffers but let's have global databases of exceptions for
specific operations, using a (largely redundant) mini-language for
buffer-matching".

If that doesn't sound like a bad idea in the face of much better other
ideas, I don't know what else to tell you.

>>  > I'm fairly sure that the solution I offered would be easy enough
>>  > implement, to actually protect the vulnerable buffer.
>>  > I suppose we are not doing that, however.
>> You sketched an untested code-less idea and I explained how flawed
>> it was.
>
> Back atcha. Modulo "code-less".

Not only did I provide code, I also verified that it works.  Anyone can
see my messages to verify that.

João

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 01.11.2022 18:23, João Távora wrote:
>> On Tue, Nov 1, 2022, 15:27 Dmitry Gutov <dgutov@yandex.ru
>> <mailto:dgutov@yandex.ru>> wrote:
>>     That's an unfair expectation: you didn't suggest an improvement
>> either.
>> Oh, I'm sorry, and is it fair to inquire about the subliminally
>> compressed: "and eglot reinvents http-url"? Can you generously grant
>> a few more sentences as to its meaning and relevance?
>> Your compressed the meaning and usefulness of a feature to something
> trivial (and only semi-relevant), and so did I.
> I thought that would be obvious.

Not only it is not, but I still have no idea what on earth you're
talking about.  You said "Eglot reinvents http-url", leading anyone to
believe there is something in http-url that Eglot can reuse.  Apparently
there isn't: so you're either too clever or just wasting time.

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

[João Távora]

Philip Kaludercic <philipk@posteo.net> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>>> Yes, do that, but use byte-compile instead, not eval.
>>> I have tried both, and it doesn't appear to be a particular advantage
>>> one way or another.  That being said, this approach is *a lot* faster,
>>> to the point that I first assumed it was broken:
>>
>> Yes, this approach is always going to be much faster than the "naive"
>> approach.  Now I've taken your code as a starting point, simplified it,
>> and I get a reasonable/typical 3.5x speedup when I use a
>> byte-compilation strategy, so one of us isn't measuring
>>
>> ```elisp
>> (defun translate-buffer-condition-1 (condition)
>>   (pcase-exhaustive condition
>>     ((or 't 'nil)
>>      condition)
>>     ((pred stringp)
>>      `(string-match-p ,condition (buffer-name buffer)))
>>     ((pred functionp)
>>      `(,condition buffer))
>
> This would break the current behaviour, because `buffer-match-p'
> requires the function be called with an additional argument if possible.
>
> This is fundamental for `display-buffer-alist' to work.

I couldn't figure out where this argument arise or who should provides
it (the condition?).  It wasn't clear.  At any rate, if you understand
this you can probably re-add it and I'm sure it won't make any
difference to the time.

>>     (`(major-mode . ,mode)
>>      `(eq (buffer-local-value 'major-mode buffer)
>> 	  ',mode))
>>     (`(derived-mode . ,mode)
>>      `(provided-mode-derived-p
>>        (buffer-local-value 'major-mode buffer)
>>        ',mode))
>>     (`(not . ,cond)
>>      `(not ,(translate-buffer-condition-1 cond)))
>>     (`(or . ,conds)
>>      `(or ,@(mapcar #'translate-buffer-condition-1 conds)))
>>     (`(and . ,conds)
>>      `(and ,@(mapcar #'translate-buffer-condition-1 conds)))))
>>
>> (defun translate-buffer-condition (condition)
>>   `(lambda (buffer) ,(translate-buffer-condition-1 condition)))
>>
>> (defvar sample-condition
>>   '(and (or buffer-file-name
>> 	    (derived-mode . compilation-mode)
>> 	    (derived-mode . dired-mode)
>> 	    (derived-mode . diff-mode)
>> 	    (derived-mode . comint-mode)
>> 	    (derived-mode . eshell-mode)
>> 	    (derived-mode . change-log-mode))
>> 	"\\*.+\\*"
>> 	(not . "\\` ")))
>>
>> (defvar form (translate-buffer-condition sample-condition))
>> (defvar compiled (byte-compile form))
>>
>> (benchmark-run 100000 (funcall (eval form) (current-buffer))) ;; (0.397404883 3 0.18942550900000032)
>> (benchmark-run 100000 (funcall compiled (current-buffer))) ;; (0.113651836 0 0.0)
>> ```
>>
>> I couldn't understand the need for a hash table or special symbol vars
>> or what that "arg" was, so I took it out, but it shouldn't make a
>> difference.
>
> The hash table makes a significant different, try evaluating
>
>    (benchmark-run 100000 (funcall (byte-compile compiled) (current-buffer))) ;; (0.113651836 0 0.0)
>

You seem to be suggesting to byte-compile a compiled object which is
odd.  Did you mean (byte-compile form)?  More importantly, you're
forcing the byte-compilation process to run every one of those 100000
repetitions, which is not something we want to measure: the point of any
code compilation is to do it once and then reuse the results of
compilation many times.

(and I'm still confused by the purpose of the hash table usage)

> The fresh symbols are used to keep the code clean and avoid possible
> naming conflicts.

Can you explain what naming conflict would arise or how the code I
provided is somehow unclean?

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:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> João Távora <joaotavora@gmail.com> writes:
>>
>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>
>>>>> Yes, do that, but use byte-compile instead, not eval.
>>>> I have tried both, and it doesn't appear to be a particular advantage
>>>> one way or another.  That being said, this approach is *a lot* faster,
>>>> to the point that I first assumed it was broken:
>>>
>>> Yes, this approach is always going to be much faster than the "naive"
>>> approach.  Now I've taken your code as a starting point, simplified it,
>>> and I get a reasonable/typical 3.5x speedup when I use a
>>> byte-compilation strategy, so one of us isn't measuring
>>>
>>> ```elisp
>>> (defun translate-buffer-condition-1 (condition)
>>>   (pcase-exhaustive condition
>>>     ((or 't 'nil)
>>>      condition)
>>>     ((pred stringp)
>>>      `(string-match-p ,condition (buffer-name buffer)))
>>>     ((pred functionp)
>>>      `(,condition buffer))
>>
>> This would break the current behaviour, because `buffer-match-p'
>> requires the function be called with an additional argument if possible.
>>
>> This is fundamental for `display-buffer-alist' to work.
>
> I couldn't figure out where this argument arise or who should provides
> it (the condition?).  It wasn't clear.  At any rate, if you understand
> this you can probably re-add it and I'm sure it won't make any
> difference to the time.

See `display-buffer-assq-regexp' in window.el.

>>>     (`(major-mode . ,mode)
>>>      `(eq (buffer-local-value 'major-mode buffer)
>>> 	  ',mode))
>>>     (`(derived-mode . ,mode)
>>>      `(provided-mode-derived-p
>>>        (buffer-local-value 'major-mode buffer)
>>>        ',mode))
>>>     (`(not . ,cond)
>>>      `(not ,(translate-buffer-condition-1 cond)))
>>>     (`(or . ,conds)
>>>      `(or ,@(mapcar #'translate-buffer-condition-1 conds)))
>>>     (`(and . ,conds)
>>>      `(and ,@(mapcar #'translate-buffer-condition-1 conds)))))
>>>
>>> (defun translate-buffer-condition (condition)
>>>   `(lambda (buffer) ,(translate-buffer-condition-1 condition)))
>>>
>>> (defvar sample-condition
>>>   '(and (or buffer-file-name
>>> 	    (derived-mode . compilation-mode)
>>> 	    (derived-mode . dired-mode)
>>> 	    (derived-mode . diff-mode)
>>> 	    (derived-mode . comint-mode)
>>> 	    (derived-mode . eshell-mode)
>>> 	    (derived-mode . change-log-mode))
>>> 	"\\*.+\\*"
>>> 	(not . "\\` ")))
>>>
>>> (defvar form (translate-buffer-condition sample-condition))
>>> (defvar compiled (byte-compile form))
>>>
>>> (benchmark-run 100000 (funcall (eval form) (current-buffer))) ;; (0.397404883 3 0.18942550900000032)
>>> (benchmark-run 100000 (funcall compiled (current-buffer))) ;; (0.113651836 0 0.0)
>>> ```
>>>
>>> I couldn't understand the need for a hash table or special symbol vars
>>> or what that "arg" was, so I took it out, but it shouldn't make a
>>> difference.
>>
>> The hash table makes a significant different, try evaluating
>>
>>    (benchmark-run 100000 (funcall (byte-compile compiled) (current-buffer))) ;; (0.113651836 0 0.0)
>>
>
> You seem to be suggesting to byte-compile a compiled object which is
> odd.  Did you mean (byte-compile form)?  

Yes, that was a copy-o on my end.

>                                          More importantly, you're
> forcing the byte-compilation process to run every one of those 100000
> repetitions, which is not something we want to measure: the point of any
> code compilation is to do it once and then reuse the results of
> compilation many times.

Exactly, but if the byte-compilation would take place in buffer-match-p,
then the measurement is relevant.

> (and I'm still confused by the purpose of the hash table usage)

The rationale is the same as for regular expressions in the core.  They
are also compiled and stored, to avoid the need for them to be
interpreted over and over again.

This should all be explained in the bug report I pointed you to, and
where this discussion should continue.

>> The fresh symbols are used to keep the code clean and avoid possible
>> naming conflicts.
>
> Can you explain what naming conflict would arise or how the code I
> provided is somehow unclean?

This is currently not the case, but if the language extended in the
future, there is the possibility that naming conflicts could arise.  I
am just following the same principle used when writing macros that
avoids name capturing.

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

[Philip Kaludercic]

In general I'd like to remind everyone of the GNU Kind Communication
Guidelines, because the tone of the discussion in this issue report has
been unpleasant for a few days now.

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

> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>>> I've explained to Philip objective reasons why I think evaluated
>>> mini-languages are almost always inferior to a decent Lisp such as Elisp.
>>> You could perfectly reasonably deprecate these two variables.
>> Not where this discussion is going, is it?
>
> Not sure.  This started has a report of hidden buffer being incorrectly
> killed by project.el.  

The issue that was reported was that Eglot/jsonrpc raised an error that
broke `project-kill-buffer'.  This could have also all been solved by
wrapping a `with-demoted-errors' around `kill-buffer'.

>                        After much insistence, you've agreed to plug the
> hole in that library.  During tests I also discovered that project.el is
> killing other buffers nonsensically, like Gnus buffers, *ibuffer*, and
> many other global.  It was actually easier to find false-positives of
> your heuristic than true ones.  Again, after some insistence, you seem
> to have come around that these things represent bugs.
>
> Three people here have suggested an opt-in approach for the true
> positives.  Now your strategy seems to be "OK: let all these false
> positives remain nonsensically associated with a project in
> project-buffers but let's have global databases of exceptions for
> specific operations, using a (largely redundant) mini-language for
> buffer-matching".

For the record, I am still not convinced 100% either way.  Whether or
not whitelisting or blacklisting is the better approach will concretely
depend the list of misjudgments that the current condition makes.

> If that doesn't sound like a bad idea in the face of much better other
> ideas, I don't know what else to tell you.
>
>>>  > I'm fairly sure that the solution I offered would be easy enough
>>>  > implement, to actually protect the vulnerable buffer.
>>>  > I suppose we are not doing that, however.
>>> You sketched an untested code-less idea and I explained how flawed
>>> it was.
>>
>> Back atcha. Modulo "code-less".
>
> Not only did I provide code, I also verified that it works.  Anyone can
> see my messages to verify that.
>
> João

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

[João Távora]

Philip Kaludercic <philipk@posteo.net> writes:

>> I couldn't figure out where this argument arise or who should provides
>> it (the condition?).  It wasn't clear.  At any rate, if you understand
>> this you can probably re-add it and I'm sure it won't make any
>> difference to the time.
>
> See `display-buffer-assq-regexp' in window.el.

Sorry, I'm not following.  Anyway, as I said, this seems to be a detail:
feel free to add back arg wherever it is needed.

>>                                          More importantly, you're
>> forcing the byte-compilation process to run every one of those 100000
>> repetitions, which is not something we want to measure: the point of any
>> code compilation is to do it once and then reuse the results of
>> compilation many times.
>
> Exactly, but if the byte-compilation would take place in buffer-match-p,
> then the measurement is relevant.

But then you're throwing away the benefits of compilation.  But my
suggestion is for you to get rid of "buffer-match-p".  Rather, make a
'buffer-matcher' that does the compilation, and then place the return
value of that, which is a plain old (possibly very fast, if compiled)
function object in the display-buffer-alist variables and everywhere
where you can put functions.

That way you still get your mini-language, you get a much faster version
of it, and you don't force your mini-language to other people who prefer
just typing plain old Elisp.

>> (and I'm still confused by the purpose of the hash table usage)
>
> The rationale is the same as for regular expressions in the core.  They
> are also compiled and stored, to avoid the need for them to be
> interpreted over and over again.
>
> This should all be explained in the bug report I pointed you to, and
> where this discussion should continue.

Well, I think you're overcomplicating this.  If you insulate
display-buffer-alist from the mini-language (i.e. you keep it the way it
was), then you don't need this hash table, and you can still use the
mini-language.

(add-to-list 'display-buffer-alist
             `(,(philips-mini-language-buffer-matcher
                  '(and (mode . x) (or (not (and "123")))))
               . display-buffer-in-side-window))

Again: you keep your mini-language and you can suggest people use
philips-mini-language-buffer-matcher in many other library APIs in
Emacs, not just window.el, but without needing to ever touch those
libraries.

It's a much more modular design, less impositive, much smaller and much
faster.

> This is currently not the case, but if the language extended in the
> future, there is the possibility that naming conflicts could arise.  I
> am just following the same principle used when writing macros that
> avoids name capturing.

You don't need to cargo cult that principle blindly.  This kind of macro
hygiene you are talking about is for macros that take arbitrary lisp
forms, which is not the case of the mini-language.  If it ever is, add
the hygiene then and likely not in a top-level defvar.

João

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

[João Távora]

Philip Kaludercic <philipk@posteo.net> writes:

>> Not sure.  This started has a report of hidden buffer being incorrectly
>> killed by project.el.  
>
> The issue that was reported was that Eglot/jsonrpc raised an error that
> broke `project-kill-buffer'.  This could have also all been solved by
> wrapping a `with-demoted-errors' around `kill-buffer'.

No.  It couldn't.  The error is there to show you among other things
that the LSP connection isn't being shut down correctly, which is not
something to paper over.  And even if you did paper over the error, you
would break eglot-autoshutdown.  I've explained that at least 3 times
already in the beginning of this discussion.

>> Three people here have suggested an opt-in approach for the true
>> positives.  Now your strategy seems to be "OK: let all these false
>> positives remain nonsensically associated with a project in
>> project-buffers but let's have global databases of exceptions for
>> specific operations, using a (largely redundant) mini-language for
>> buffer-matching".
>
> For the record, I am still not convinced 100% either way.

I just said that becasue you at one point did suggested the
opt-in approach 

  I have to admit that I am more and more inclined to make the list a
  opt-in thing, where  we explicitly mark those major modes that are tied
  to a project.

But of course it's OK to change one's mind back and forth.

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:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>>> I couldn't figure out where this argument arise or who should provides
>>> it (the condition?).  It wasn't clear.  At any rate, if you understand
>>> this you can probably re-add it and I'm sure it won't make any
>>> difference to the time.
>>
>> See `display-buffer-assq-regexp' in window.el.
>
> Sorry, I'm not following.  Anyway, as I said, this seems to be a detail:
> feel free to add back arg wherever it is needed.

I just meant to give an example where and how the argument is used.

>>>                                          More importantly, you're
>>> forcing the byte-compilation process to run every one of those 100000
>>> repetitions, which is not something we want to measure: the point of any
>>> code compilation is to do it once and then reuse the results of
>>> compilation many times.
>>
>> Exactly, but if the byte-compilation would take place in buffer-match-p,
>> then the measurement is relevant.
>
> But then you're throwing away the benefits of compilation.  But my
> suggestion is for you to get rid of "buffer-match-p".  Rather, make a
> 'buffer-matcher' that does the compilation, and then place the return
> value of that, which is a plain old (possibly very fast, if compiled)
> function object in the display-buffer-alist variables and everywhere
> where you can put functions.

I see, but I don't agree.  My main objection here is that the conditions
aren't introspectable (e.g. when using ECI) any more and that it all
becomes more verbose.

> That way you still get your mini-language, you get a much faster version
> of it, and you don't force your mini-language to other people who prefer
> just typing plain old Elisp.

That is not the case to begin with, as the "mini-language" is just a
super-set of Emacs Lisp, seeing as any function is a legal word of the
language.  As I have said before, it just makes it easier to write
common operations like matching buffer names or major modes.

>>> (and I'm still confused by the purpose of the hash table usage)
>>
>> The rationale is the same as for regular expressions in the core.  They
>> are also compiled and stored, to avoid the need for them to be
>> interpreted over and over again.
>>
>> This should all be explained in the bug report I pointed you to, and
>> where this discussion should continue.
>
> Well, I think you're overcomplicating this.  If you insulate
> display-buffer-alist from the mini-language (i.e. you keep it the way it
> was), then you don't need this hash table, and you can still use the
> mini-language.
>
> (add-to-list 'display-buffer-alist
>              `(,(philips-mini-language-buffer-matcher
>                   '(and (mode . x) (or (not (and "123")))))
>                . display-buffer-in-side-window))
>
> Again: you keep your mini-language and you can suggest people use
> philips-mini-language-buffer-matcher in many other library APIs in
> Emacs, not just window.el, but without needing to ever touch those
> libraries.
>
> It's a much more modular design, less impositive, much smaller and much
> faster.

I have not much elese to say on the issue here, that hasn't already been
said.  If you think it is such a mistake, please report a bug or post on
emacs-devel and we can clarify the issue there.

>> This is currently not the case, but if the language extended in the
>> future, there is the possibility that naming conflicts could arise.  I
>> am just following the same principle used when writing macros that
>> avoids name capturing.
>
> You don't need to cargo cult that principle blindly.  This kind of macro
> hygiene you are talking about is for macros that take arbitrary lisp
> forms, which is not the case of the mini-language.  If it ever is, add
> the hygiene then and likely not in a top-level defvar.

I don't see any disadvantage from following best practices, even if it
is not immediately relevant.  There is no reason why `buffer' ought to
be a special symbol, so I would like to avoid the possibility of this
leading to issues that hack to be back-hacked at some point in the
future.

But this is just a matter of personal style.  I have seen you write
documentations strings like "[...] This docstring appeases checkdoc,
that's all.", while I insist on documenting every function I commit,
even if I am the only one who uses the package.  We just have different
principles.

> João

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:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>>> Not sure.  This started has a report of hidden buffer being incorrectly
>>> killed by project.el.  
>>
>> The issue that was reported was that Eglot/jsonrpc raised an error that
>> broke `project-kill-buffer'.  This could have also all been solved by
>> wrapping a `with-demoted-errors' around `kill-buffer'.
>
> No.  It couldn't.  The error is there to show you among other things
> that the LSP connection isn't being shut down correctly, which is not
> something to paper over.  And even if you did paper over the error, you
> would break eglot-autoshutdown.  I've explained that at least 3 times
> already in the beginning of this discussion.

That was not my concern, my concern was that project-kill-buffer broke.
I continue to not see a reason why project.el should be considered
broken because of this, and not jsonrpc/eglot.

To be fair, I am not as familiar with LSP as you are, so there might be
a technical reason why this is the case, but without something like
that, you appear to be privileging an arbitrary perspective, supposedly
because you are the maintainer of these packages.

This is probably best solved by reading code.  Can you point me to a few
functions/section/etc. that would help me clarify the situation.

>>> Three people here have suggested an opt-in approach for the true
>>> positives.  Now your strategy seems to be "OK: let all these false
>>> positives remain nonsensically associated with a project in
>>> project-buffers but let's have global databases of exceptions for
>>> specific operations, using a (largely redundant) mini-language for
>>> buffer-matching".
>>
>> For the record, I am still not convinced 100% either way.
>
> I just said that becasue you at one point did suggested the
> opt-in approach 
>
>   I have to admit that I am more and more inclined to make the list a
>   opt-in thing, where  we explicitly mark those major modes that are tied
>   to a project.

The key word here is "more and more inclined".

> But of course it's OK to change one's mind back and forth.

I haven't made up my mind, I am just trying to understand all
perspectives, and get a better overview over the issue.

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

[João Távora]

Philip Kaludercic <philipk@posteo.net> writes:

>> But then you're throwing away the benefits of compilation.  But my
>> suggestion is for you to get rid of "buffer-match-p".  Rather, make a
>> 'buffer-matcher' that does the compilation, and then place the return
>> value of that, which is a plain old (possibly very fast, if compiled)
>> function object in the display-buffer-alist variables and everywhere
>> where you can put functions.
>
> I see, but I don't agree.  My main objection here is that the conditions
> aren't introspectable (e.g. when using ECI) any more and that it all
> becomes more verbose.

If you want introspection, i.e. see the translated Elisp code when
inspecting the value of display-buffer-alist, then pass another argument
to buffer-matcher which makes it return an uncompiled function.

Furthermore, as debugging goes, the buffer-matcher idea is much better:
if the user does a mistake in his mini-language program, that mistake
can be caught when buffer-matcher is called, i.e. during her init.  Not
when display-buffer-alist is about to run the program.  And if your
mini-language ever evolves, you can also issue warnings at this point.
This is what compilation and compilers have always existed for.  Lisp
gives you this power in a few lines (something which takes a lot of hard
in other languages).

>> That way you still get your mini-language, you get a much faster version
>> of it, and you don't force your mini-language to other people who prefer
>> just typing plain old Elisp.
>
> That is not the case to begin with, as the "mini-language" is just a
> super-set of Emacs Lisp, seeing as any function is a legal word of the
> language.  As I have said before, it just makes it easier to write
> common operations like matching buffer names or major modes.

That's not what a super-set is.  A super-set language is C++ to C,
because C++ can compile (almost any) C program.  The mini-language is a
DSL.  In my opinion, one that brings very few advantages, but there's no
accounting for taste.  So the common way to do DSL's in Elisp is use the
approach I provided.  You make them more useful, and you don't force
people to use them.

> I have not much elese to say on the issue here, that hasn't already been
> said.  If you think it is such a mistake, please report a bug or post on
> emacs-devel and we can clarify the issue there.

OK.  It's not so much a mistake as lost opportunity to make a more
reusable, less britlle piece of software using less code, less
docstrings, less manual writing, etc.  Cuts down on the mental overload,
potentially teaches some people Elisp instead of mini-language, and
reduces CPU usage and laptop battery time.

>>> This is currently not the case, but if the language extended in the
>>> future, there is the possibility that naming conflicts could arise.  I
>>> am just following the same principle used when writing macros that
>>> avoids name capturing.
>>
>> You don't need to cargo cult that principle blindly.  This kind of macro
>> hygiene you are talking about is for macros that take arbitrary lisp
>> forms, which is not the case of the mini-language.  If it ever is, add
>> the hygiene then and likely not in a top-level defvar.
>
> I don't see any disadvantage from following best practices, even if it
> is not immediately relevant.

This is not a best practice, really it's not. MAKE-SYMBOL and GENSYM are
for macro that take forms hygiene: this is simply not that case.

> be a special symbol, so I would like to avoid the possibility of this
> leading to issues that hack to be back-hacked at some point in the
> future.
>
> But this is just a matter of personal style.  I have seen you write
> documentations strings like "[...] This docstring appeases checkdoc,
> that's all."

Ahaha, that was just a joke.  Keep documenting all your functions,
you're very right to do so.  Trust me, don't make defvar's for
(make-symbol).  Noone does this: it's just cruft.

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:

>>> That way you still get your mini-language, you get a much faster version
>>> of it, and you don't force your mini-language to other people who prefer
>>> just typing plain old Elisp.
>>
>> That is not the case to begin with, as the "mini-language" is just a
>> super-set of Emacs Lisp, seeing as any function is a legal word of the
>> language.  As I have said before, it just makes it easier to write
>> common operations like matching buffer names or major modes.
>
> That's not what a super-set is.  A super-set language is C++ to C,
> because C++ can compile (almost any) C program.  The mini-language is a
> DSL.  In my opinion, one that brings very few advantages, but there's no
> accounting for taste.  So the common way to do DSL's in Elisp is use the
> approach I provided.  You make them more useful, and you don't force
> people to use them.

Let B be the set of S-Expressions that `buffer-match-p' handles, and F
be the set of S-Expressions that satisfy `functionp'.  We see that

   B ∖ F ≠ ∅

as for any foo, (major-mode . foo) ∈ B, but (major-mode . foo) ∉ F.  We
see that

   F ∖ B = ∅

as every function (per `functionp') is a valid `buffer-match-p'
predicate.  Hence we conclude that B ⊋ F (strict superset).             ∎

>> be a special symbol, so I would like to avoid the possibility of this
>> leading to issues that hack to be back-hacked at some point in the
>> future.
>>
>> But this is just a matter of personal style.  I have seen you write
>> documentations strings like "[...] This docstring appeases checkdoc,
>> that's all."
>
> Ahaha, that was just a joke.  Keep documenting all your functions,
> you're very right to do so.  Trust me, don't make defvar's for
> (make-symbol).  Noone does this: it's just cruft.

Again, see the other bug report, the code I pasted in this thread was
just a sketch.

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

[João Távora]

Philip Kaludercic <philipk@posteo.net> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>>> Not sure.  This started has a report of hidden buffer being incorrectly
>>>> killed by project.el.  
>>>
>>> The issue that was reported was that Eglot/jsonrpc raised an error that
>>> broke `project-kill-buffer'.  This could have also all been solved by
>>> wrapping a `with-demoted-errors' around `kill-buffer'.
>>
>> No.  It couldn't.  The error is there to show you among other things
>> that the LSP connection isn't being shut down correctly, which is not
>> something to paper over.  And even if you did paper over the error, you
>> would break eglot-autoshutdown.  I've explained that at least 3 times
>> already in the beginning of this discussion.
>
> That was not my concern, my concern was that project-kill-buffer
> broke.  I continue to not see a reason why project.el should be
> considered broken because of this, and not jsonrpc/eglot.

It was always broken since the way it was created.  Eglot's precedes it,
I've also explained this.  You can't kill a internal hidden buffer just
because you can see it anymore than you unintern some "--" symbol in
obarray simply because you can see it and don't like its inelegant name,
or you think it's taking too much RAM.  I can't really explain this any
other way.

> This is probably best solved by reading code.  Can you point me to a few
> functions/section/etc. that would help me clarify the situation.
> I haven't made up my mind, I am just trying to understand all
> perspectives, and get a better overview over the issue.

OK, one last time.  First, the above principle of encapsulation has
nothing to do with LSP or Eglot or Jsonrpc.

Specifically in Eglot, as I already explained, eglot.el and jsonrpc.el
colaborate so that jsonrpc.el holds a network process to communicate
with the LSP server.  It uses a buffer for more convenient and efficient
parsing of messages.  That buffer is out of bounds for Eglot (and anyone
else).  Eglot doesn't know or care that a buffer is being used, it only
actuate on the handle using operations of jsonrpc.el's API.  According
to user options, Eglot provides controlled shutdown and reconnection
using these operations.

All of this has worked for a long time and predates project.el's buffer
collection/killing/switching logic.

If you or some Lisp package finds and destroys the hidden implementation
detail, all the above is broken.  Therefore packages like project.el
that do it are buggy in that regard.

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:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> João Távora <joaotavora@gmail.com> writes:
>>
>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>
>>>>> Not sure.  This started has a report of hidden buffer being incorrectly
>>>>> killed by project.el.  
>>>>
>>>> The issue that was reported was that Eglot/jsonrpc raised an error that
>>>> broke `project-kill-buffer'.  This could have also all been solved by
>>>> wrapping a `with-demoted-errors' around `kill-buffer'.
>>>
>>> No.  It couldn't.  The error is there to show you among other things
>>> that the LSP connection isn't being shut down correctly, which is not
>>> something to paper over.  And even if you did paper over the error, you
>>> would break eglot-autoshutdown.  I've explained that at least 3 times
>>> already in the beginning of this discussion.
>>
>> That was not my concern, my concern was that project-kill-buffer
>> broke.  I continue to not see a reason why project.el should be
>> considered broken because of this, and not jsonrpc/eglot.
>
> It was always broken since the way it was created.  Eglot's precedes it,
> I've also explained this.  

This is not an argument.

>                            You can't kill a internal hidden buffer just
> because you can see it anymore than you unintern some "--" symbol in
> obarray simply because you can see it and don't like its inelegant name,
> or you think it's taking too much RAM.  I can't really explain this any
> other way.

The equivalence is not given, more so because there is no package that
manages symbols the way that project.el manages buffers.  If this is
all, your argument, or rather statement, is simply not convincing, no
matter how often it is reiterated. *

>> This is probably best solved by reading code.  Can you point me to a few
>> functions/section/etc. that would help me clarify the situation.
>> I haven't made up my mind, I am just trying to understand all
>> perspectives, and get a better overview over the issue.
>
> OK, one last time.  First, the above principle of encapsulation has
> nothing to do with LSP or Eglot or Jsonrpc.

Again, it is not given that this applies to buffers.  Robust code should
be able to handle a missing resource or prevent the resource from being
revoked.  This is not possible with buffers, so this is a design fault
with Eglot or Jsonprc. *

> Specifically in Eglot, as I already explained, eglot.el and jsonrpc.el
> colaborate so that jsonrpc.el holds a network process to communicate
> with the LSP server.  It uses a buffer for more convenient and efficient
> parsing of messages.  That buffer is out of bounds for Eglot (and anyone
> else).  

Can I `switch-to-buffer' into it, can I `kill-buffer' it?  Yes?  So it
is not out of bounds, and a mistake was made in the design by assuming
this is the case. *

E.g. why does the buffer have a default directory?

>         Eglot doesn't know or care that a buffer is being used, it only
> actuate on the handle using operations of jsonrpc.el's API.  According
> to user options, Eglot provides controlled shutdown and reconnection
> using these operations.

OK, these appear as convince options that can be used, but don't prevent
me from killing any buffers. *

> All of this has worked for a long time and predates project.el's buffer
> collection/killing/switching logic.

Again, this is not an argument, just a statement of fact that doesn't
imply anything definitive.  From the same statement I can also infer
that jsonrpc/Eglot had an undetected bug that was exposed by project.el.
You are committing an informal logical fallacy by assuming some kind of
temporal order in the attribution of mistakes. *

> If you or some Lisp package finds and destroys the hidden implementation
> detail, all the above is broken.  Therefore packages like project.el
> that do it are buggy in that regard.

This is a "If it can be destroyed by the truth, it deserves to be
destroyed"-kind of thing.  Double-dash symbols are "private" by
convention, but that might change in the future.  Hidden buffers are
just not displayed, as said in the manual (found via the index):

    Buffers that are ephemeral and generally uninteresting to the user
    have names starting with a space, so that the ‘list-buffers’ and
    ‘buffer-menu’ commands don’t mention them (but if such a buffer visits a
    file, it *is* mentioned).  A name starting with space also initially
    disables recording undo information; see *note Undo::.

The keywords being "ephemeral and generally uninteresting", not "private
and off-limits" This is not the same kind of thing like with a symbol
that was generated using `make-symbol', where you are guaranteed nobody
can access it without being passed the symbol directly or via some dirty
hacks.  And just so it is clear, using `buffer-list' is not a dirty
hack. *

If you need private buffers, you either need to argue that the
convention be clarified in the reference manual or have these
implemented in such a way (e.g. by passing a flag to
`get-buffer-create') nobody can access a private buffer without being
passed the buffer object or via some dirty hacks.  And just so it is
clear, using `buffer-list' is not a dirty hack.

  * These are all "Strong Opinions, Weakly Held", I really don't care
    about it too much.  What I don't like about this discussion is the
    tone you take in postulating the mistake was made somewhere else and
    that is that.  This is arrogant, unkind and non-constructive
    behaviour.  If necessary I am ready to discuss this off-list and
    resolve any issues.  I am inherently non-confrontation and dislike
    the fact that I have to even clarify this.  My top priority is
    always to find common grounds.

    I am almost never frustrated by discussions on the Emacs development
    mailing list or in bug reports.  The tone is at best productive, at
    worst lethargic.  This thread is an exception, which is very sad to
    see.  I implore you once more to consider the GNU Kind Communication
    Guidelines and try to foster a productive and positive atmosphere.

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

[Juri Linkov]

>> OK, one last time.  First, the above principle of encapsulation has
>> nothing to do with LSP or Eglot or Jsonrpc.
>
> Again, it is not given that this applies to buffers.  Robust code should
> be able to handle a missing resource or prevent the resource from being
> revoked.  This is not possible with buffers, so this is a design fault
> with Eglot or Jsonprc. *

While the default value of project-kill-buffer-conditions really
looks too wide (especially covering fundamental-mode and special-mode),
OTOH I completely support the request to make Eglot more resilient
to unforeseeable situations.  Currently it's so brittle, so I get a lot
of such errors all the time:

Debugger entered--Lisp error: (wrong-type-argument arrayp nil)
  file-truename(nil)
  eglot--path-to-uri(nil)
  eglot--TextDocumentIdentifier()
  eglot--signal-textDocument/didClose()
  kill-buffer(#<buffer  *xref-temp*>)
  xref--convert-hits(...)
  xref-matches-in-files("word" ...)
  project--find-regexp-in-files("word" ...)
  apply(project--find-regexp-in-files ("word" ...))
  xref--show-xref-buffer(...)
  xref--show-xrefs(...)
  xref-show-xrefs(...)
  project-find-regexp("word")
  funcall-interactively(project-find-regexp "word")
  command-execute(project-find-regexp)

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

[João Távora]

Sorry Philip, but this is a very long email rehashing many things I've
addressed repeatedly.  I don't have anything to add.

I hope Dmitry installs the patch to project.el's
project-kill-buffer-conditions that he proposed so that this bug can be
closed.  Or some other patch like the ones I proposed.

Thanks for everything, I'm geting off the merry-go-round now.

João

Philip Kaludercic <philipk@posteo.net> writes:

[...snipped...]

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

[Juri Linkov]

> OTOH I completely support the request to make Eglot more resilient
> to unforeseeable situations.  Currently it's so brittle, so I get a lot
> of such errors all the time:
>
> Debugger entered--Lisp error: (wrong-type-argument arrayp nil)
>   file-truename(nil)
>   eglot--path-to-uri(nil)
>   eglot--TextDocumentIdentifier()
>   eglot--signal-textDocument/didClose()
>   kill-buffer(#<buffer  *xref-temp*>)
>   xref--convert-hits(...)
>   xref-matches-in-files("word" ...)
>   project--find-regexp-in-files("word" ...)
>   apply(project--find-regexp-in-files ("word" ...))
>   xref--show-xref-buffer(...)
>   xref--show-xrefs(...)
>   xref-show-xrefs(...)
>   project-find-regexp("word")
>   funcall-interactively(project-find-regexp "word")
>   command-execute(project-find-regexp)

Here's a patch that fixes this:

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index c5870618372..5b05f84c63c 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1792,7 +1792,9 @@ eglot--maybe-activate-editing-mode
   (unless eglot--managed-mode
     ;; Called when `revert-buffer-in-progress-p' is t but
     ;; `revert-buffer-preserve-modes' is nil.
-    (when (and buffer-file-name (eglot-current-server))
+    (when (and buffer-file-name
+               (not (string-match-p "\\` " (buffer-name)))
+               (eglot-current-server))
       (setq eglot--diagnostics nil)
       (eglot--managed-mode)
       (eglot--signal-textDocument/didOpen))))

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

[João Távora]

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

Juri, this doesn't seem right. Eglot shouldn't be turning itself on in
hidden buffers to start with: It's totally useless there.

So you have to explain an Emacs -Q recipe that demonstrates how Eglot
reached this nonsensical state. You haven't done that (or have you and i
have missed it?).

I tried project-find-regexp with Eglot-managed files with no problems, but
I have no idea what you're doing.

Also, this problem is totally off-topic here: this is about
project-kill-buffers. Please start as new issue of you haven't already.

João

On Thu, Nov 3, 2022, 17:39 Juri Linkov <juri@linkov.net> wrote:

> > OTOH I completely support the request to make Eglot more resilient
> > to unforeseeable situations.  Currently it's so brittle, so I get a lot
> > of such errors all the time:
> >
> > Debugger entered--Lisp error: (wrong-type-argument arrayp nil)
> >   file-truename(nil)
> >   eglot--path-to-uri(nil)
> >   eglot--TextDocumentIdentifier()
> >   eglot--signal-textDocument/didClose()
> >   kill-buffer(#<buffer  *xref-temp*>)
> >   xref--convert-hits(...)
> >   xref-matches-in-files("word" ...)
> >   project--find-regexp-in-files("word" ...)
> >   apply(project--find-regexp-in-files ("word" ...))
> >   xref--show-xref-buffer(...)
> >   xref--show-xrefs(...)
> >   xref-show-xrefs(...)
> >   project-find-regexp("word")
> >   funcall-interactively(project-find-regexp "word")
> >   command-execute(project-find-regexp)
>
> Here's a patch that fixes this:
>
> diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
> index c5870618372..5b05f84c63c 100644
> --- a/lisp/progmodes/eglot.el
> +++ b/lisp/progmodes/eglot.el
> @@ -1792,7 +1792,9 @@ eglot--maybe-activate-editing-mode
>    (unless eglot--managed-mode
>      ;; Called when `revert-buffer-in-progress-p' is t but
>      ;; `revert-buffer-preserve-modes' is nil.
> -    (when (and buffer-file-name (eglot-current-server))
> +    (when (and buffer-file-name
> +               (not (string-match-p "\\` " (buffer-name)))
> +               (eglot-current-server))
>        (setq eglot--diagnostics nil)
>        (eglot--managed-mode)
>        (eglot--signal-textDocument/didOpen))))
>

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

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

[Dmitry Gutov]

On 01.11.2022 13:36, João Távora wrote:
> On Mon, Oct 31, 2022 at 10:51 PM Dmitry Gutov <dgutov@yandex.ru 
> <mailto:dgutov@yandex.ru>> wrote:
> 
>  > I suggest you try it first.
> 
> It works in my test
> 
>  > Disaster, really?
> 
> The reason I came about the Gnus problem was when using it
> to reply to some emails here and trying out the C-x p k and finding out
> all my Gnus buffers were gone.

I have now pushed the proposed fix, as well as an additional change to 
except Gnus modes (not sure it will be enough -- there might be 
background buffers using some modes that don't derive -- but it seems to 
help in my brief testing).

That's not to mark a final decision, but to fix the immediate issues, 
and to remember them in the code.

The question now is how to better handle packages and features that are 
similar to Gnus in that regard. One alternative option would be to add a 
var through which such modes could opt-out (rather than opt-in). It 
seems more likely that we won't have to update 
project-kill-buffer-conditions often then.

A whitelisting approach seems cleaner/safer, though.

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

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Dmitry Gutov [2022-10-31 19:24 +0200] wrote:

> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index ac278edd40..1e7573c740 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -1223,7 +1223,9 @@ project-display-buffer-other-frame
>  (defcustom project-kill-buffer-conditions
>    '(buffer-file-name    ; All file-visiting buffers are included.
>      ;; Most of the temp buffers in the background:
> -    (major-mode . fundamental-mode)
> +    (and
> +     (major-mode . fundamental-mode)
> +     (not "\\` "))

AKA "\\`[^ ]" here and elsewhere, FWIW.

Thanks,

-- 
Basil

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

[Dmitry Gutov]

On 04.11.2022 13:21, Basil L. Contovounesios via Bug reports for GNU 
Emacs, the Swiss army knife of text editors wrote:
> AKA "\\`[^ ]" here and elsewhere, FWIW.

Right, thank you.