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

[João Távora]

Augusto Stoffel <arstoffel@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 ()