bug#70724: 29.2.50; eglot-reconnect errors when the project is deleted

[Dmitry Gutov <dmitry@gutov.dev>]

Seems like we could use Eglot's maintainer's input.

On 18/05/2024 11:31, Eli Zaretskii wrote:
> Ping!  Can we make some progress here?
> 
>> Cc: app-emacs-dev@janestreet.com
>> Date: Sat, 4 May 2024 04:09:48 +0300
>> From: Dmitry Gutov <dmitry@gutov.dev>
>>
>> Hi Spencer,
>>
>> On 02/05/2024 22:37, Spencer Baugh wrote:
>>   >
>>   > In some project /home/foo/proj, with pretty much any LSP server:
>>   >
>>   > 1. In /home/foo/proj, M-x eglot, starting some LSP server
>>   >
>>   > 2. Delete the directory /home/foo/proj
>>   >
>>   > 3. The LSP server will crash/exit
>>   >
>>   > 4. The process sentinel for the server will run, running
>>   >     eglot--on-shutdown which by default will call eglot-reconnect
>>   >
>>   > 5. eglot-reconnect extracts the saved project instance out of the
>>   >     server, which has a root directory which no longer exists, and calls
>>   >     eglot--connect with it
>>   >
>>   > 6. eglot--connect calls project-name on a nonexistent project instance,
>>   >     which may fail with an error depending on the project implementation
>>   >     (I have a custom project implementation, but I think this can happen
>>   >     with project-vc too)
>>   >
>>   > 7. This causes the process sentinel to error.
>>   >
>>   > I think the right fix is probably for eglot--on-shutdown (or maybe
>>   > eglot-reconnect) to call (project-current nil (project-root pr)) to find
>>   > the new project instance.  If that returns nil, the project has
>>   > disappeared, and eglot should just not try to reconnect.  This also
>>   > would make eglot behave better if the project layout changes (e.g. if
>>   > there are nested projects).
>>
>> I think I like this solution (as long as the nil value returned by
>> project-current on this step is appropriately handled).
>>
>> Something like:
>>
>> diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
>> index 6896baf30ce..7b2461c3ce6 100644
>> --- a/lisp/progmodes/eglot.el
>> +++ b/lisp/progmodes/eglot.el
>> @@ -1426,11 +1426,15 @@ eglot-reconnect
>>      (interactive (list (eglot--current-server-or-lose) t))
>>      (when (jsonrpc-running-p server)
>>        (ignore-errors (eglot-shutdown server interactive nil
>> 'preserve-buffers)))
>> -  (eglot--connect (eglot--major-modes server)
>> -                  (eglot--project server)
>> -                  (eieio-object-class-name server)
>> -                  (eglot--saved-initargs server)
>> -                  (eglot--language-ids server))
>> +  (let* ((root (project-root (eglot--project server)))
>> +         (project (project-current nil root)))
>> +    (if (not project)
>> +        (eglot--error "Project in `%s' is gone!" root)
>> +      (eglot--connect (eglot--major-modes server)
>> +                      project
>> +                      (eieio-object-class-name server)
>> +                      (eglot--saved-initargs server)
>> +                      (eglot--language-ids server))))
>>      (eglot--message "Reconnected!"))
>>
>>    (defvar eglot--managed-mode) ; forward decl
>>
>>
>> Though it also raises a question about the caching strategy for VC-Aware
>> project backend. At the moment is associates a project with a directory
>> more or less indefinitely, and this is a case to watch out for.
>>
>>   > Alternatively, maybe eglot--on-shutdown shouldn't automatically
>>   > reconnect in the first place?  Maybe reconnection should happen
>>   > automatically only when some specific buffer tries to interact with the
>>   > LSP - then it can run project-current in the context of that specific
>>   > buffer, and see there's no project, and fail.  Plus, if the user kills
>>   > all the buffers in the project (possibly with project-kill-buffers)
>>   > before deleting it, this approach would entirely avoid the unnecessary
>>   > eglot reconnection attempt.
>>
>> This also sounds good, though it'd probably require more changes
>> overall. Additionally, perhaps I'd change the association from (server
>> -> project) to (server -> project-root), relying on the project
>> backends' internal caches to fetch the project value whenever it's
>> needed. That might be the most reliable approach. Perhaps the slowest in
>> theory, but hopefully not noticeably so.