bug#61866: 29.0.60; Eglot: Dir-local workspace config doesn't work as described

[Augusto Stoffel]

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