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