Fix: Hook into eglot-ensure, instead of eglot--executable-find

[vedang]

eglot--executable-find was an internal function that has been removed in the latest version of Eglot (1.17). Due to this, emacs-pet no longer works with Eglot correctly, since it's advice is not used anymore. #50

In this commit, we fix the issue by making the following changes:

1. Extract the internals of pet-executable-find into an independent function pet-adjust-path-executable-find.

   • This function creates a buffer-local variants of exec-path and tramp-remote-path variables, and add venv paths found by pet. Eglot can now find the path to the right executables correctly.
   • We also remove calls to setenv PATH, since we don't need to modify a global system setting to make venv executables available to Eglot.

2. Remove all references to unused or non-existent eglot variables.

   • eglot--executable-find no longer exists.
   • eglot--uri-to-path is not used by pet.

3. Modify pet-executable-find to use our new function pet-adjust-paths-executable-find.

4. Advice eglot-ensure, which is an Eglot public API that is always called when starting an LSP server.

   • We are no longer depending on internal variables / functions for this, and so the Eglot code is now a bit more future-proof.

I've tested this change as follows:

1. Tested against Python projects using the following types of scaffolding:
   • poetry
   • Manual venv creation
2. Tested against Eglot versions 1.17 and 1.16 (Emacs versions 31 and 29)
3. I could not get make test to run correctly, because of native compilation / Emacs 31 issues.

[vedang]

I see that there are tests around eglot--executable-find. I will fix them and update this PR.

[wyuenho]

This is too complicated and advicing too far away from the call sites of executable-find. There's both eglot and eglot-ensure to activate eglot, and you've only advised eglot-ensure.

You need to advise eglot-alternatives and eglot--guess-contact, and in the advices, you just need to let bind file-name-handler-alist to a separate handler to resolve the exec path to the python executables.

[vedang]

Okay, makes sense.

Let me go through the eglot code a bit more carefully, and rewrite the commit based on your strategy. I'll do this as and when time permits this week and submit a patch.

[wyuenho]

No worries, take your time. Thanks for the report regardless.

[vedang]

Hi @wyuenho :

Advising eglot-alternatives is not useful, unfortunately. The main use of this function is when defining the variable eglot-server-programs. And since the variable is already defined by the time we get to it, advising it doesn't help.

If I look at the function that eglot-alternatives returns, it directly calls executable-find, which is too low-level a function for us to do anything about. Also, executable-find itself calls locate-file with predicate arg set to 1 and my understanding is that this ignores any file-name-handler-alist changes as well.

The option that remains, I think, is adjusting the path before the eglot-alternatives lambda function is called by eglot--guess-contact. This is what my original patch happened to be doing.

What do you think? I can clean up the approach in the original to only run before applying eglot--guess-contact -- no need to advice eglot-ensure. But I wanted to check with you if there is a better way to handle this.

[wyuenho]

Advising eglot-alternatives is not useful, unfortunately. The main use of this function is when defining the variable eglot-server-programs. And since the variable is already defined by the time we get to it, advising it doesn't help.

eglot-alternatives returns the absolute path for the pythons executable using executable-find in order to define eglot-server-programs, which is exactly what pet-executable-find does, it's just instead of advising eglot--executable-find directly, you are advising one level up, and instead of replacing the definition of executable-find in eglot-alternatives with an flet, you let bind file-name-handler-alist to an alist that maps the python executable name to a handler that delegates to pet-executable-find, before applying eglot-alternatives.

If I look at the function that eglot-alternatives returns, it directly calls executable-find, which is too low-level a function for us to do anything about. Also, executable-find itself calls locate-file with predicate arg set to 1 and my understanding is that this ignores any file-name-handler-alist changes as well.

This is incorrect for both counts. file-executable-p does not return 1, it returns t.

[vedang]

Advising eglot-alternatives is not useful, unfortunately. The main use of this function is when defining the variable eglot-server-programs. And since the variable is already defined by the time we get to it, advising it doesn't help.

eglot-alternatives returns the absolute path for the pythons executable using executable-find in order to define eglot-server-programs, which is exactly what pet-executable-find does, it's just instead of advising eglot--executable-find directly, you are advising one level up, and instead of replacing the definition of executable-find in eglot-alternatives with an flet, you let bind file-name-handler-alist to an alist that maps the python executable name to a handler that delegates to pet-executable-find, before applying eglot-alternatives.

eglot-alternatives returns a lambda function at compile time, and this lambda function returns the absolute path to the python executable. Here is the usage of eglot-alternatives, from https://github.com/emacs-mirror/emacs/blob/5db9471453af4a47d7665295ea007faf35c069b6/lisp/progmodes/eglot.el#L244

(defvar eglot-server-programs
  ;; FIXME: Maybe this info should be distributed into the major modes
  ;; themselves where they could set a buffer-local `eglot-server-program'
  ;; instead of keeping this database centralized.
  ;; FIXME: With `derived-mode-add-parents' in Emacs≥30, some of
  ;; those entries can be simplified, but we keep them for when
  ;; `eglot.el' is installed via GNU ELPA in an older Emacs.
  `(((rust-ts-mode rust-mode) . ("rust-analyzer"))
    ((cmake-mode cmake-ts-mode) . ("cmake-language-server"))
    (vimrc-mode . ("vim-language-server" "--stdio"))
    ((python-mode python-ts-mode)
     . ,(eglot-alternatives
         '("pylsp" "pyls" ("basedpyright-langserver" "--stdio")
           ("pyright-langserver" "--stdio")
           "jedi-language-server" "ruff-lsp")))
           ......
           ))

The call to eglot-alternatives has already expanded to a lambda function when eglot has loaded (before emacs-pet). So advising eglot-alternatives itself does not modify the behaviour of the lambda function.

If I look at the function that eglot-alternatives returns, it directly calls executable-find, which is too low-level a function for us to do anything about. Also, executable-find itself calls locate-file with predicate arg set to 1 and my understanding is that this ignores any file-name-handler-alist changes as well.

This is incorrect for both counts. file-executable-p does not return 1, it returns t.

The lambda function returned by eglot-alternatives calls executable-find here: https://github.com/emacs-mirror/emacs/blob/5db9471453af4a47d7665295ea007faf35c069b6/lisp/progmodes/eglot.el#L209 and this is the definition of executable-find (where is calls locate-file with predicate = 1): https://github.com/emacs-mirror/emacs/blob/5db9471453af4a47d7665295ea007faf35c069b6/lisp/files.el#L1246

(defun executable-find (command &optional remote)
  "Search for COMMAND in `exec-path' and return the absolute file name.
Return nil if COMMAND is not found anywhere in `exec-path'.  If
REMOTE is non-nil, search on the remote host indicated by
`default-directory' instead."
  (if (and remote (file-remote-p default-directory))
      (let ((res (locate-file
	          command
	          (mapcar
	           (lambda (x) (concat (file-remote-p default-directory) x))
	           (exec-path))
	          exec-suffixes 'file-executable-p)))
        (when (stringp res) (file-local-name res)))
    ;; Use 1 rather than file-executable-p to better match the
    ;; behavior of call-process.
    (let ((default-directory (file-name-quote default-directory 'top)))
      (locate-file command exec-path exec-suffixes 1))))

[wyuenho]

The call to eglot-alternatives has already expanded to a lambda function when eglot has loaded (before emacs-pet). So advising eglot-alternatives itself does not modify the behaviour of the lambda function.

You can advise a function even before the function to be adviced has been defined and loaded.

this is the definition of executable-find (where is calls locate-file with predicate = 1)

Ah thanks, I read the remote branch above and missed the local branch below. In that case, it's probably even easier, just replace the definition of executable-find in eglot-alternatives with an flet.

[vedang]

I think I am completely misunderstanding the situation here. Please clear it up for me.

• Imagine that I start Emacs and do some Rust programming using the rust-analyzer LSP.
• Eglot is now loaded, and eglot-server-programs is defined. eglot-alternatives has already been called and has returned a lambda function for each entry in eglot-server-programs, including the python-mode entry.
• I now load a .py file and emacs-pet kicks in. It advises eglot-alternatives. But this function is never called again, because there is already a lambda function defined for python-mode in eglot-server-programs.
• Our advice fails to do what we want.

What am I misunderstanding?

[wyuenho]

There are a couple of ways around this:

1. You can order the load order of eglot and pet with :after, to make sure eglot is loaded after pet.
2. Or you can unload and reload eglot with unload-feature and load-library.

WRT to the lambda in eglot-alternative, the problem is the alteratives are captured inside a closure and you can't access them right? Once you've dealt with either 1 or 2, in your eglot-alternative advice, you can wrap the lambda with an open closure, resolve the executable names with pet-executable-find to absolute paths, and set it back to the open closure slot right?

Alternatively, if the above sounds insanely complex to you, you can advice eglot--lookup-mode to refer to a eglot-server-programs in a let binding, or lastly, find a way to convert eglot-server-programs to a buffer local variable in a python-mode-hook, then you can slice and dice the alist however you want without interfering with the global value shared with other buffer in different major modes. Does this make sense?

[vedang]

Yes, this makes sense!

In my opinion, the right approach would be to advice eglot--lookup-mode and provide a let-binding for eglot-server-programs. This approach is independent of any load-order, and just works.

I have updated the commits with this approach, and tested locally that it works.

One open point is that I need to write tests, which is something I'll do over the weekend. Until then, please review the code change!

[wyuenho]

By "replacing the definition of executable-find, I don't mean copy and paste, there's cl-flet and cl-letf for these kind of things.

[vedang]

I've kept the structure similar to the existing eglot code on purpose, because it makes reading and understanding the code simpler in my opnion.

Even if we were to use cl-flet, we should still have to reproduce the entire lambda starting on line 917 because that's what the closure is. Only one line in the closure changes (replacing of executable-find with pet-executable-find, on line 932), but we have to copy the entire closure over anyway because the rest of the logic is still necessary.

[wyuenho]

If you advice eglot--lookup-mode, you can just look up the executables here right? Why do you still need to advice eglot-alternatives?

[vedang]

I have not adviced eglot-alternatives. I have just mimic'd the structure for readability purposes. I could have replaced line 966 with the closure starting on line 917, but this does not reduce complexity or improve readability in any way.

[vedang]

I've rebased my changes on top of the latest master. These are working fine with the latest version of eglot.