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.
View this message in rfc822 format
From: Augusto Stoffel <arstoffel <at> gmail.com> To: João Távora <joaotavora <at> gmail.com> Cc: 61866 <at> debbugs.gnu.org Subject: 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 ()
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.