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

Warn if pyright is started from $HOME #2971

Closed
hejops opened this issue Jan 16, 2024 · 5 comments
Closed

Warn if pyright is started from $HOME #2971

hejops opened this issue Jan 16, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@hejops
Copy link

hejops commented Jan 16, 2024

Language server

pyright

Requested feature

The behaviour of lspconfig.util.root_pattern is "depth-first":

function M.root_pattern(...)
local patterns = vim.tbl_flatten { ... }
return function(startpath)
startpath = M.strip_archive_subpath(startpath)
for _, pattern in ipairs(patterns) do
local match = M.search_ancestors(startpath, function(path)
for _, p in ipairs(vim.fn.glob(M.path.join(M.path.escape_wildcards(path), pattern), true, true)) do
if M.path.exists(p) then
return path
end
end
end)

i.e., it checks the existence of a given root_file (e.g. pyproject.toml in the current working dir), ascends as far up if it can, then tries the next root_file in the current working dir.

As a result, the existence of a hypothetical ~/pyproject.toml (which, in real world situations, should probably not exist):

~/pyproject.toml
~/project/.git
~/project/file1
~/project/pyrightconfig.json
~/project/subproject/.git
~/project/subproject/file2

would override any other root_file. Since pyright has no problems with being initiated from $HOME, it will go on ahead, but take a very long time to init (up to 2 minutes on a convoluted macos system). A warning is produced in LspLog, but otherwise, the user has no idea that something might be off.

Short of reworking root_pattern to allow breadth-first search (which is, in itself, undesirable, since in the above example, setting the project root to be ~/project when working on file2 is fairly sane behaviour), the minimum change would be to raise a warning if pyright is being started from $HOME. In most foreseeable cases, users will not have any of the following files in $HOME.

local root_files = {
'pyproject.toml',
'setup.py',
'setup.cfg',
'requirements.txt',
'Pipfile',
'pyrightconfig.json',
'.git',
}

The only notable exception is $HOME/.git for dotfiles containing python files. Such situations are not unheard of, so I'm not sure if the warning would be useful.

NB: I saw that root_pattern has been slated for deprecation (since 2022-08 -- #2079), but I haven't looked at the corresponding vim.fs equivalent.

Other clients which have this feature

No response

@hejops hejops added the enhancement New feature or request label Jan 16, 2024
@ouuan
Copy link

ouuan commented Feb 15, 2024

It's #2885 that "cleaned" some configurations (#2904), but broke many others like #2975 and this one. It also broke my setup because I have a .eslintrc.js in $HOME and .eslintrc.cjs in my project.

I suggest that #2885 should be reimplemented in a backward-compatible way that does not change the behavior of the old configurations. e.g. use a new function or argument for the new behavior.

@justinmk
Copy link
Member

the existence of a hypothetical ~/pyproject.toml (which, in real world situations, should probably not exist):

And if it does exist, then the behavior is correct.

#2885 changed the behavior to be more deterministic and avoid lots of hand written code in the configs.

It also broke my setup because I have a .eslintrc.js in $HOME and .eslintrc.cjs in my project.

If you have an eslintrc in your HOME then presumably your HOME is a eslint project.

@justinmk justinmk closed this as not planned Won't fix, can't repro, duplicate, stale Feb 15, 2024
@ouuan
Copy link

ouuan commented Feb 15, 2024

The problem is that, almost all configurations were not using root_pattern(pattern1) or root_pattern(pattern2) or root_pattern(pattern3). They were using root_pattern(pattern1, pattern2, pattern3) or root_pattern(pattern1, pattern2) or root_pattern(pattern3, pattern4). So #2885 is not just a clean-up, but rather changes the behavior of most of them. Are you sure the new behavior is desired in all these servers? e.g. I'm sure that prioritizing eslintrc.js over eslintrc.cjs is incorrect.

@ouuan
Copy link

ouuan commented Feb 15, 2024

I think we should be able to specify that several patterns are of the same priority.

@ouuan
Copy link

ouuan commented Feb 15, 2024

If you have an eslintrc in your HOME then presumably your HOME is a eslint project.

  1. The actually correct behavior is to check for the root option in eslintrc, and stop at root: true. But that's complicated and it's acceptable to just approach the correct behavior in most situations.
  2. Let's just assume that what you say is correct. Then it's still irrelevant to prioritizing some eslintrc file formats. What if eslintrc.cjs is at $HOME and eslintrc.js is in my project?

#2885 changed the behavior to be more deterministic

Guessing which file format is prioritized is non-deterministic for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants