joaotavora / eglot

A client for Language Server Protocol servers
GNU General Public License v3.0
2.25k stars 200 forks source link

Ensure default-directory ends in slash #1281

Closed wyuenho closed 1 year ago

wyuenho commented 1 year ago

Pyright needs to be configured after initialization, as such, the server will make a workspace/configuration request after establishing a connection. A typical request body contains a scopeUri that is a directory but does not end in a /.

The scopeUri is passed to eglot--uri-to-path, which does not canonicalize the path, i.e. makes sure if the path points to a directory, a / is always appended. The output of such is then given to eglot--workspace-configuration-plist which assumes the path is always a file, so file-name-directory simply returns the parent of the project directory instead of the project directory, which in turns sets the default-directory incorrectly for looking up eglot-workspace-configuration.

This PR fixes this bug.

joaotavora commented 1 year ago

Thanks, but need to see an MRE for this situation, consisting of a minimal python project with a dir-locals or similar setting for eglot-workspace-configuration and the first few interactions recorded in the Eglot events buffer. If you want you can provide before and after versions of your patch.

wyuenho commented 1 year ago

eglot-events-buffer output before.txt after.txt

You can see the directory local variables were not picked up before the change.

Directory structure

.
├── .dir-locals.el
├── poetry.lock
├── pyproject.toml
├── README.md
└── src
   └── example
      ├── __init__.py
      └── __main__.py

.dir-locals.el

;;; Directory Local Variables            -*- no-byte-compile: t -*-
;;; For more information see (info "(emacs) Directory Variables")

((nil . ((eglot-workspace-configuration . (:python
                                           (:pythonPath
                                            "~/Library/Caches/pypoetry/virtualenvs/example-Afjj3yjW-py3.11/bin/python"
                                            :venvPath
                                            "~/Library/Caches/pypoetry/virtualenvs/example-Afjj3yjW-py3.11/"))))))

pyproject.toml

[tool.poetry]
name = "example"
version = "0.1.0"
description = ""
authors = ["Your Name <you@example.com>"]
readme = "README.md"

[tool.poetry.dependencies]
python = "^3.11"

[build-system]
requires = ["poetry-core"]
build-backend = "poetry.core.masonry.api"

[tool.pyright]
strict = ["src"]

src

All empty files

Additional info

joaotavora commented 1 year ago

Thanks. That is enough to reproduce the problem.

joaotavora commented 1 year ago

A typical request body contains a scopeUri that is a directory but does not end in a /.

Reading https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration, I don't know if it is correct from the server to send a directory as scopeUri. It isspecified as a DocumentUri object, which in principle means it refers to a document, which is not a directory. Is it possible the server is in the wrong here?

Normally, if a server just wants to get the options for the containing workspace (which is what it appears to be doing in your example) the server will just omit scopeUri. Why isn't it doing that?

joaotavora commented 1 year ago

Can you say if this simpler patch fixes the issue for you?

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 65daa0941d5..42af71b8811 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -2609,7 +2609,8 @@ eglot--workspace-configuration-plist
   (let ((val (with-temp-buffer
                (setq default-directory
                      (if path
-                         (file-name-directory path)
+                         (if (file-directory-p path) path
+                           (file-name-directory path))
                        (project-root (eglot--project server))))
                ;; Set the major mode to be the first of the managed
                ;; modes.  This is the one the user started eglot in.
wyuenho commented 1 year ago

That also works. I was not aware the scopeUri should always be a DocumentUri.

joaotavora commented 1 year ago

I was not aware the scopeUri should always be a DocumentUri.

The standard is underspecified in many aspects and this is one of them. So servers (as clients) misunderstand them or, unfortunately too often, code to whatever VSCode expects.

It is conceivable that a server might want to get "workspace properties" for a specific file, but I haven't seen that happen yet (and Eglot doesn't support it -- though it could).

What I think the server should be doing here is not supplying scopeUri at all.

Even so, I think the patch I proposed is relatively safe: iff the scopeUri references a directory, then we treat it as a directory.

wyuenho commented 1 year ago

I've updated the PR to make sure the path always end in a slash even when it's a directory, because default-directory seems to always end in a slash.

joaotavora commented 1 year ago

I pushed your patch to Emacs master: https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=fad48a20e665e6b5b51c417e9c04946517a2aa2f.

Should be in the upcoming Eglot 1.16 or you can find it earlier in GNU-devel ELPA (https://elpa.gnu.org/devel/).