Package: emacs;
Reported by: Augusto Stoffel <arstoffel <at> gmail.com>
Date: Tue, 28 Feb 2023 12:39:02 UTC
Severity: normal
Found in version 29.0.60
Fixed in version 29.1
Done: Augusto Stoffel <arstoffel <at> gmail.com>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 61866 in the body.
You can then email your comments to 61866 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
joaotavora <at> gmail.com, bug-gnu-emacs <at> gnu.org
:bug#61866
; Package emacs
.
(Tue, 28 Feb 2023 12:39:02 GMT) Full text and rfc822 format available.Augusto Stoffel <arstoffel <at> gmail.com>
:joaotavora <at> gmail.com, bug-gnu-emacs <at> gnu.org
.
(Tue, 28 Feb 2023 12:39:02 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Augusto Stoffel <arstoffel <at> gmail.com> To: bug-gnu-emacs <at> gnu.org Subject: 29.0.60; Eglot: Dir-local workspace config doesn't work as described Date: Tue, 28 Feb 2023 13:38:37 +0100
Quoting from the manual: > Here’s an example of defining the workspace-configuration settings [...] > > ((python-mode > . ((eglot-workspace-configuration > . (:pylsp (:plugins (:jedi_completion (:include_params t > :fuzzy t) > :pylint (:enabled :json-false))))))) > (go-mode > . ((eglot-workspace-configuration > . (:gopls (:usePlaceholders t)))))) The above doesn't work. > Alternatively, the same configuration could be defined as follows: > ((nil > . ((eglot-workspace-configuration > . (:pylsp (:plugins (:jedi_completion (:include_params t > :fuzzy t) > :pylint (:enabled :json-false))) > :gopls (:usePlaceholders t)))))) But this version does. To see why, patch eglot-signal-didChangeConfiguration as follows: (defun eglot-signal-didChangeConfiguration (server) "Send a `:workspace/didChangeConfiguration' signal to SERVER. When called interactively, use the currently active server" (interactive (list (eglot--current-server-or-lose))) (message "=> %s %s" (current-buffer) default-directory) (jsonrpc-notify ...) Then you'll see that upon first starting the server, one gets: => *temp* ~/project_dir/ Presumably, the temp buffer is in fundamental mode. Note also that if you later do `M-x eglot-signal-didChangeConfiguration RET', then the that buffer's local value of `eglot-workspace-configuration' is used, since: => actual_file.py ~/project_dir/subdir [ This behavior is confusing and reinforces my opinion that server information should be kept away from buffer-local variables. Related glitch: if you have two servers running, A and B, and then, from a buffer in project A you do M-x eglot-show-workspace-configuration RET and select server B in the prompt, you get server A's information. ] There's probably no better place than dir-locals to store "workspace" (aka project) configuration. But it would be better to then record this configuration in the server object after it's read for .dir-locals.el, and provide some UI to modify the configuration on the fly if desired.
bug-gnu-emacs <at> gnu.org
:bug#61866
; Package emacs
.
(Tue, 28 Feb 2023 19:19:02 GMT) Full text and rfc822 format available.Message #8 received at 61866 <at> debbugs.gnu.org (full text, mbox):
From: João Távora <joaotavora <at> gmail.com> To: Augusto Stoffel <arstoffel <at> gmail.com> Cc: 61866 <at> debbugs.gnu.org Subject: Re: bug#61866: 29.0.60; Eglot: Dir-local workspace config doesn't work as described Date: Tue, 28 Feb 2023 19:20:35 +0000
Augusto Stoffel <arstoffel <at> gmail.com> writes: > Quoting from the manual: > >> Here’s an example of defining the workspace-configuration settings [...] >> >> ((python-mode >> . ((eglot-workspace-configuration >> . (:pylsp (:plugins (:jedi_completion (:include_params t >> :fuzzy t) >> :pylint (:enabled :json-false))))))) >> (go-mode >> . ((eglot-workspace-configuration >> . (:gopls (:usePlaceholders t)))))) > > The above doesn't work. I've just tried it and it works fine. Here's what I did: I put that example in a .dir-locals.el nearby a thingy.py and thingy.go file in the same dir. I start two Eglots managing each file and inspect that the correct config is sent in each case. > => *temp* ~/project_dir/ Have you tried adding a trace of the 'major-mode' variable to that 'message' call? It should produce the correct major mode. > Presumably, the temp buffer is in fundamental mode. This is why we set major-mode and call hack-dir-local-variables-non-file-buffer. You could write a recipe that demonstrating that it fails, but read on as I have a patch to present to fix another separate thing you reported here. > Note also that if you later do `M-x > eglot-signal-didChangeConfiguration RET', then the that buffer's local > value of `eglot-workspace-configuration' is used, since: > > => actual_file.py ~/project_dir/subdir This is how it should work. A buffer in python-mode should report its directory-local values. > [ This behavior is confusing and reinforces my opinion that server > information should be kept away from buffer-local variables. I'm not sure I see the benefit. This would cache things and necessitate some invalidation when the user changes the .dir-locals.el file. > Related glitch: if you have two servers running, A and B, and then, > from a buffer in project A you do > M-x eglot-show-workspace-configuration RET > and select server B in the prompt, you get server A's information. ] I've reproduced this, and indeed this was quite broken. The patch at the end should fix it, but I'd like you to test it. > There's probably no better place than dir-locals to store "workspace" > (aka project) configuration. But it would be better to then record this > configuration in the server object after it's read for .dir-locals.el, .dir-locals.el is short for "dir-local variables". buffer-local variables are an integral part to that mechanism. If we add a new slot in Eglot's server object we're adding caching/duplication/entangling (however you prefer to call this). Sometimes that is needed, for example for performance reasons, but it comes with invalitation challenges. Please try this patch: diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el index ffc9511469f..89cdd8c14c7 100644 --- a/lisp/progmodes/eglot.el +++ b/lisp/progmodes/eglot.el @@ -1335,10 +1335,7 @@ eglot--connect (lambda () (setf (eglot--inhibit-autoreconnect server) (null eglot-autoreconnect))))))) - (let ((default-directory (project-root project)) - (major-mode (car managed-modes))) - (hack-dir-local-variables-non-file-buffer) - (run-hook-with-args 'eglot-connect-hook server)) + (run-hook-with-args 'eglot-connect-hook server) (eglot--message "Connected! Server `%s' now managing `%s' buffers \ in project `%s'." @@ -2444,9 +2441,7 @@ eglot-workspace-configuration (defun eglot-show-workspace-configuration (&optional server) "Dump `eglot-workspace-configuration' as JSON for debugging." - (interactive (list (and (eglot-current-server) - (eglot--read-server "Server configuration" - (eglot-current-server))))) + (interactive (list (eglot--read-server "Show workspace configuration for" t))) (let ((conf (eglot--workspace-configuration-plist server))) (with-current-buffer (get-buffer-create "*EGLOT workspace configuration*") (erase-buffer) @@ -2457,14 +2452,23 @@ eglot-show-workspace-configuration (json-pretty-print-buffer)) (pop-to-buffer (current-buffer))))) -(defun eglot--workspace-configuration (server) - (if (functionp eglot-workspace-configuration) - (funcall eglot-workspace-configuration server) - eglot-workspace-configuration)) - -(defun eglot--workspace-configuration-plist (server) - "Returns `eglot-workspace-configuration' suitable for serialization." - (let ((val (eglot--workspace-configuration server))) +(defun eglot--workspace-configuration-plist (server &optional path) + "Returns SERVER's workspace configuration as a plist. +If PATH consider that file's `file-name-directory' to get the +local value of the `eglot-workspace-configuration' variable, else +use the root of SERVER's `eglot--project'." + (let ((val (with-temp-buffer + (setq default-directory + (if path + (file-name-directory path) + (project-root (eglot--project server)))) + ;; FIXME; Should probably join values of all managed + ;; major mode of this server. + (setq major-mode (car (eglot--major-modes server))) + (hack-dir-local-variables-non-file-buffer)() + (if (functionp eglot-workspace-configuration) + (funcall eglot-workspace-configuration server) + eglot-workspace-configuration)))) (or (and (consp (car val)) (cl-loop for (section . v) in val collect (if (keywordp section) section @@ -2489,25 +2493,18 @@ eglot-handle-request (apply #'vector (mapcar (eglot--lambda ((ConfigurationItem) scopeUri section) - (with-temp-buffer - (let* ((uri-path (eglot--uri-to-path scopeUri)) - (default-directory - (if (and uri-path - (not (string-empty-p uri-path)) - (file-directory-p uri-path)) - (file-name-as-directory uri-path) - (project-root (eglot--project server))))) - (setq-local major-mode (car (eglot--major-modes server))) - (hack-dir-local-variables-non-file-buffer) - (cl-loop for (wsection o) - on (eglot--workspace-configuration-plist server) - by #'cddr - when (string= - (if (keywordp wsection) - (substring (symbol-name wsection) 1) - wsection) - section) - return o)))) + (cl-loop with scope-uri-path = + (and scopeUri (eglot--uri-to-path scopeUri)) + for (wsection o) + on (eglot--workspace-configuration-plist server + scope-uri-path) + by #'cddr + when (string= + (if (keywordp wsection) + (substring (symbol-name wsection) 1) + wsection) + section) + return o)) items))) (defun eglot--signal-textDocument/didChange ()
bug-gnu-emacs <at> gnu.org
:bug#61866
; Package emacs
.
(Tue, 28 Feb 2023 20:27:01 GMT) Full text and rfc822 format available.Message #11 received at 61866 <at> debbugs.gnu.org (full text, mbox):
From: Augusto Stoffel <arstoffel <at> gmail.com> To: João Távora <joaotavora <at> gmail.com> Cc: 61866 <at> debbugs.gnu.org Subject: Re: bug#61866: 29.0.60; Eglot: Dir-local workspace config doesn't work as described Date: Tue, 28 Feb 2023 21:26:21 +0100
On Tue, 28 Feb 2023 at 19:20, João Távora wrote: > I've just tried it and it works fine. Here's what I did: I put that > example in a .dir-locals.el nearby a thingy.py and thingy.go file in the > same dir. I start two Eglots managing each file and inspect that the > correct config is sent in each case. > >> => *temp* ~/project_dir/ > > Have you tried adding a trace of the 'major-mode' variable to that > 'message' call? It should produce the correct major mode. Duh, I don't know what I did wrong. This indeed works as you describe. >> [ This behavior is confusing and reinforces my opinion that server >> information should be kept away from buffer-local variables. > > I'm not sure I see the benefit. This would cache things and necessitate > some invalidation when the user changes the .dir-locals.el file. The benefit is that my suggestion would restrict the origin of server configuration to exactly 1 place: the dir-locals.el. It would never ever come from a buffer-local value of some random managed buffer. >> Related glitch: if you have two servers running, A and B, and then, >> from a buffer in project A you do >> M-x eglot-show-workspace-configuration RET >> and select server B in the prompt, you get server A's information. ] > > I've reproduced this, and indeed this was quite broken. The patch at > the end should fix it, but I'd like you to test it. > >> There's probably no better place than dir-locals to store "workspace" >> (aka project) configuration. But it would be better to then record this >> configuration in the server object after it's read for .dir-locals.el, > > .dir-locals.el is short for "dir-local variables". buffer-local > variables are an integral part to that mechanism. If we add a new slot > in Eglot's server object we're adding caching/duplication/entangling > (however you prefer to call this). Sometimes that is needed, for > example for performance reasons, but it comes with invalitation > challenges. So again, this would be _removing_ duplication. Yes, the managed buffers would have some garbage buffer-local value, but it would be always ignored. Okay, now that I read your patch, it achieves the same goal of ignoring the buffer-locals in a different way. Makes sense to me. > Please try this patch: > > diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el > index ffc9511469f..89cdd8c14c7 100644 > --- a/lisp/progmodes/eglot.el > +++ b/lisp/progmodes/eglot.el > @@ -1335,10 +1335,7 @@ eglot--connect > (lambda () > (setf (eglot--inhibit-autoreconnect server) > (null eglot-autoreconnect))))))) > - (let ((default-directory (project-root project)) > - (major-mode (car managed-modes))) > - (hack-dir-local-variables-non-file-buffer) > - (run-hook-with-args 'eglot-connect-hook server)) > + (run-hook-with-args 'eglot-connect-hook server) > (eglot--message > "Connected! Server `%s' now managing `%s' buffers \ > in project `%s'." > @@ -2444,9 +2441,7 @@ eglot-workspace-configuration > > (defun eglot-show-workspace-configuration (&optional server) > "Dump `eglot-workspace-configuration' as JSON for debugging." > - (interactive (list (and (eglot-current-server) > - (eglot--read-server "Server configuration" > - (eglot-current-server))))) > + (interactive (list (eglot--read-server "Show workspace configuration for" t))) > (let ((conf (eglot--workspace-configuration-plist server))) > (with-current-buffer (get-buffer-create "*EGLOT workspace configuration*") > (erase-buffer) > @@ -2457,14 +2452,23 @@ eglot-show-workspace-configuration > (json-pretty-print-buffer)) > (pop-to-buffer (current-buffer))))) > > -(defun eglot--workspace-configuration (server) > - (if (functionp eglot-workspace-configuration) > - (funcall eglot-workspace-configuration server) > - eglot-workspace-configuration)) > - > -(defun eglot--workspace-configuration-plist (server) > - "Returns `eglot-workspace-configuration' suitable for serialization." > - (let ((val (eglot--workspace-configuration server))) > +(defun eglot--workspace-configuration-plist (server &optional path) > + "Returns SERVER's workspace configuration as a plist. > +If PATH consider that file's `file-name-directory' to get the > +local value of the `eglot-workspace-configuration' variable, else > +use the root of SERVER's `eglot--project'." > + (let ((val (with-temp-buffer > + (setq default-directory > + (if path > + (file-name-directory path) > + (project-root (eglot--project server)))) > + ;; FIXME; Should probably join values of all managed > + ;; major mode of this server. > + (setq major-mode (car (eglot--major-modes server))) I don't like that merging idea. Think of a server like Digestif that covers 4 major modes and several more derived ones. In fact, IIUC, mode derivations imply that it's impossible to know beforehand all modes a server can manage (other than inspecting the whole obarray). It seems best to declare that the user must save the config in the nil section of dir-locals.el. The configuration helper of the other bug could enforce this. Or maybe put it under the fake `eglot' entry like this: ((go-mode . ((indent-tabs-mode . nil))) (eglot . ((eglot-workspace-configuration . (:gopls (:usePlaceholders t)))))) It should work if you adapt the last quoted line, since (provided-mode-derived-p 'eglot 'eglot) => t, but is probably too eccentric. > + (hack-dir-local-variables-non-file-buffer)() > + (if (functionp eglot-workspace-configuration) > + (funcall eglot-workspace-configuration server) > + eglot-workspace-configuration)))) > (or (and (consp (car val)) > (cl-loop for (section . v) in val > collect (if (keywordp section) section > @@ -2489,25 +2493,18 @@ eglot-handle-request > (apply #'vector > (mapcar > (eglot--lambda ((ConfigurationItem) scopeUri section) > - (with-temp-buffer > - (let* ((uri-path (eglot--uri-to-path scopeUri)) > - (default-directory > - (if (and uri-path > - (not (string-empty-p uri-path)) > - (file-directory-p uri-path)) > - (file-name-as-directory uri-path) > - (project-root (eglot--project server))))) > - (setq-local major-mode (car (eglot--major-modes server))) > - (hack-dir-local-variables-non-file-buffer) > - (cl-loop for (wsection o) > - on (eglot--workspace-configuration-plist server) > - by #'cddr > - when (string= > - (if (keywordp wsection) > - (substring (symbol-name wsection) 1) > - wsection) > - section) > - return o)))) > + (cl-loop with scope-uri-path = > + (and scopeUri (eglot--uri-to-path scopeUri)) > + for (wsection o) > + on (eglot--workspace-configuration-plist server > + scope-uri-path) > + by #'cddr > + when (string= > + (if (keywordp wsection) > + (substring (symbol-name wsection) 1) > + wsection) > + section) > + return o)) > items))) > > (defun eglot--signal-textDocument/didChange ()
bug-gnu-emacs <at> gnu.org
:bug#61866
; Package emacs
.
(Wed, 01 Mar 2023 01:43:01 GMT) Full text and rfc822 format available.Message #14 received at 61866 <at> debbugs.gnu.org (full text, mbox):
From: João Távora <joaotavora <at> gmail.com> To: Augusto Stoffel <arstoffel <at> gmail.com> Cc: 61866 <at> debbugs.gnu.org Subject: Re: bug#61866: 29.0.60; Eglot: Dir-local workspace config doesn't work as described Date: Wed, 01 Mar 2023 01:44:39 +0000
Augusto Stoffel <arstoffel <at> gmail.com> writes: >> I'm not sure I see the benefit. This would cache things and necessitate >> some invalidation when the user changes the .dir-locals.el file. > > The benefit is that my suggestion would restrict the origin of server > configuration to exactly 1 place: the dir-locals.el. It would never > ever come from a buffer-local value of some random managed buffer. That buffer is never "random", it is always a temporary buffer in the project's directory with the correct major-mode. >>> Related glitch: if you have two servers running, A and B, and then, >>> from a buffer in project A you do >>> M-x eglot-show-workspace-configuration RET >>> and select server B in the prompt, you get server A's information. ] >> >> I've reproduced this, and indeed this was quite broken. The patch at >> the end should fix it, but I'd like you to test it. >> >>> There's probably no better place than dir-locals to store "workspace" >>> (aka project) configuration. But it would be better to then record this >>> configuration in the server object after it's read for .dir-locals.el, >> >> .dir-locals.el is short for "dir-local variables". buffer-local >> variables are an integral part to that mechanism. If we add a new slot >> in Eglot's server object we're adding caching/duplication/entangling >> (however you prefer to call this). Sometimes that is needed, for >> example for performance reasons, but it comes with invalitation >> challenges. > > So again, this would be _removing_ duplication. Sorry but I don't follow. You can't take buffer-local variables out of the equation, so if we add a new slot and copy the value we're adding a copy to the process. If the user updates .dir-locals.el we have to add code to update that duplicated copy of information. In my version we don't have to do anything. >> + ;; FIXME; Should probably join values of all managed >> + ;; major mode of this server. >> + (setq major-mode (car (eglot--major-modes server))) > > I don't like that merging idea. Think of a server like Digestif that > covers 4 major modes and several more derived ones. In fact, IIUC, mode > derivations imply that it's impossible to know beforehand all modes a > server can manage (other than inspecting the whole obarray). > > It seems best to declare that the user must save the config in the nil > section of dir-locals.el. The configuration helper of the other bug > could enforce this. > > Or maybe put it under the fake `eglot' entry like this: > > ((go-mode > . ((indent-tabs-mode . nil))) > (eglot > . ((eglot-workspace-configuration > . (:gopls (:usePlaceholders t)))))) > > It should work if you adapt the last quoted line, since > (provided-mode-derived-p 'eglot 'eglot) => t, but is probably too > eccentric. Yes, a bit :-) I'm not sure I want to change this or break people's .dir-locals.el. I don't place too much importance on thought-experiment problems until I'm presented with evidence of a real problem. Like the eglot-show-workspace-configuration bug you showed me and that I just solved. I pushed the patch, after testing manually. I took out the FIXME, as it's not really relevant. João
bug-gnu-emacs <at> gnu.org
:bug#61866
; Package emacs
.
(Wed, 01 Mar 2023 07:11:01 GMT) Full text and rfc822 format available.Message #17 received at 61866 <at> debbugs.gnu.org (full text, mbox):
From: Augusto Stoffel <arstoffel <at> gmail.com> To: João Távora <joaotavora <at> gmail.com> Cc: 61866 <at> debbugs.gnu.org Subject: Re: bug#61866: 29.0.60; Eglot: Dir-local workspace config doesn't work as described Date: Wed, 01 Mar 2023 08:10:27 +0100
On Wed, 1 Mar 2023 at 01:44, João Távora wrote: > That buffer is never "random", it is always a temporary buffer in the > project's directory with the correct major-mode. Yes, you patch is great. By random I meant a real project buffer (random because it's just one of many files in a project). > I pushed the patch, after testing manually. I took out the FIXME, as > it's not really relevant. Fine by me, but that observation was important. If today I configure my server and put it under the `latex-mode' section (because I _think_ it's a LaTeX project), and tomorrow I open a new Emacs and then do C-x C-f myproject/bibliography.bib then my server will be in an unexpected state. So I'd would always suggest, if not enforce, that the server configuration goes in the nil section. Or, at the very least, not teach the problematic variant in the manual.
bug-gnu-emacs <at> gnu.org
:bug#61866
; Package emacs
.
(Wed, 01 Mar 2023 09:24:01 GMT) Full text and rfc822 format available.Message #20 received at 61866 <at> debbugs.gnu.org (full text, mbox):
From: João Távora <joaotavora <at> gmail.com> To: Augusto Stoffel <arstoffel <at> gmail.com> Cc: 61866 <at> debbugs.gnu.org Subject: Re: bug#61866: 29.0.60; Eglot: Dir-local workspace config doesn't work as described Date: Wed, 1 Mar 2023 09:22:54 +0000
On Wed, Mar 1, 2023 at 7:10 AM Augusto Stoffel <arstoffel <at> gmail.com> wrote: > So I'd would always suggest, if not enforce, that the server > configuration goes in the nil section. Or, at the very least, not > teach the problematic variant in the manual. If you're configuring a server that manages multiple different languages then you should use the 'nil' section, of course. I can add that to the manual. João
Augusto Stoffel <arstoffel <at> gmail.com>
to control <at> debbugs.gnu.org
.
(Thu, 09 Mar 2023 13:11:01 GMT) Full text and rfc822 format available.Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org
.
(Fri, 07 Apr 2023 11:24:07 GMT) Full text and rfc822 format available.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.