Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure default-directory ends in slash #1281

Closed
wants to merge 1 commit into from
Closed

Ensure default-directory ends in slash #1281

wants to merge 1 commit into from

Conversation

wyuenho
Copy link

@wyuenho wyuenho commented Aug 30, 2023

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
Copy link
Owner

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
Copy link
Author

wyuenho commented Aug 30, 2023

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

  • pyright is installed globally outside of the project with pipx install pyright
  • the virtualenv and python paths were found using poetry install && poetry env info in the term

@joaotavora
Copy link
Owner

Thanks. That is enough to reproduce the problem.

@joaotavora
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

wyuenho commented Aug 31, 2023

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

@joaotavora
Copy link
Owner

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
Copy link
Author

wyuenho commented Aug 31, 2023

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
Copy link
Owner

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/).

@joaotavora joaotavora closed this Sep 1, 2023
@wyuenho wyuenho deleted the ensure-directory-ends-in-slash branch September 1, 2023 00:08
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Sep 1, 2023
Even though scopeUri is specified to be of documentUri type, some
servers (notably pyright) insist on passing the pathname of a
directory there.  In pyright's case this is frequently useless, as the
directory is the project directory.  Nevertheless we can be lenient to
those servers by detecting whether the value is a directory and doing
the right thing.

Note that we do not (yet) support per-file configuration storage.

* lisp/progmodes/eglot.el (eglot--workspace-configuration-plist):
Rework.

Co-authored-by: João Távora <joaotavora@gmail.com>
GitHub-reference: joaotavora/eglot#1281
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants