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

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

[Augusto Stoffel]

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

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

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:

>> 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#61866: 29.0.60; Eglot: Dir-local workspace config doesn't work as described

[Augusto Stoffel]

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#61866: 29.0.60; Eglot: Dir-local workspace config doesn't work as described

[João Távora]

On Wed, Mar 1, 2023 at 7:10 AM Augusto Stoffel <arstoffel@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