Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

[Theodor Thornhill]

João Távora <joaotavora@gmail.com> writes:

> On Wed, Apr 17, 2024 at 7:41 PM Theodor Thornhill <theo@thornhill.no> wrote:
> have been looking for an Eglot maintainer but everyone I contacted
>
>> If you want I can do this - so that you don't have to. I've been
>> considering it for some time after I responded to your call for
>> maintainers.
>
> OK, I've added you to the GitHub repo as a collaborator.  Let me know
> off-list what your intentions are (to phase it out, keep it and take
> it over, or whatever) and let's see how it goes.
>

Thanks, will do.

>> > The change seems well structured, well coded, and well described in the
>> > commit message, so I could understand it easily.  Do keep that up.
>> >
>> > But of course going from file-buffer-visiting to something else
>> > whose underlying implementation is faster but doesn't chase
>> > symlinks is probably going to have some kind of functional implication
>> > right?  I wonder if (or rather "I hope that") you guys considered it.
>>
>> I did - and afaict it has no such implications. It
>
> I've identified at least one implication.  A rather obvious one, given
> that you've effectively removed symlink-awareness from Eglot.
>
> When working in a project with symlinks and visiting such symlinks,
> the LSP server  is now informed (via LSP 'didOpen')  of the symlinked
> file as if it was its own file, whereas before your changes Eglot
> sent over the true name of the file.   You can see this easily
> from M-x eglot-events-buffer.
>
> What does this mean in practice?  Many things potentially, but
> at least this one: I've run this experiment:  In a new dir, create
> a lib.h file with this silly content:
>
>   int foo();
>
> Then create a main.cpp like this:
>
>   #include "lib.h"
>   int main() { return foo(); }
>
> Then create a mainlink.cpp symbolic link to main.cpp
>
> Then M-x eglot (launches clangd server to manage the new directory
> as a project).
>
> Let's say that during the Eglot session I visit both main.cpp
> and mainlink.cpp in different buffers (either because I don't visit
> them at the same time or because find-file-existing-other-name is
> nil).  Then I press M-? on lib.h's foo() to tell me who references it.
>
> Before you change, Eglot will -- correctly -- tell me there  is a single
> user of lib.h's foo() function in my project.
>
> After your change, it tells me there are two users.  This is wrong,
> there is only one.
>
> It could be that some servers with direct access to the file system
> can deduplicate the information and add back the symlink smarts.
>
> But clangd doesn't do this, and in general servers _can't_ do
> this because LSP models a virtual file system.

>From what I can see in other servers this is recognized as a server
problem, and not a client problem [1], [2], [3]. 

>
> And for symlinks to large enough files, I'd be surprised if this
> doesn't slow down the performance of the server because it has to
> analyse what it is told is a completely new file.
>
> So this seems like a pretty big flaw to me after just minimal
> surface scratching.  Please reinstate the previous code.
>

I can absolutely do this, but I'd rather we consider it a bit more
before I do. On my system there is a real, tangible performance increase
with this change, and the symlink one seems more like a server edge case
issue. Maybe we could check for file-symlink-p rather than runthe whole
file-truepath?


> We can resume discussion of your performance problems in the
> bug report.  I've seen the profile reports and file-truename is
> not thaat big of a problem.  But nevertheless I think I can
> help you think of other places to cache things.  Caching
> eglot--TextDocumentIdentifier per-buffer seems like a much
> easier way to solve your problems.

I disagree - it really makes a huge difference in felt latency.

Theo


[1]: https://github.com/clangd/clangd/issues/124
[2]: https://github.com/microsoft/python-language-server/issues/181
[3]: https://github.com/microsoft/vscode-python/issues/2613