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

feat(lsp): use home directory as working directory #3649

Closed
wants to merge 1 commit into from

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Aug 13, 2024

Summary

Closes #2772

The original issue was triggered when a single file was opened by a single client. When this occurs, the client can't pass rootUri or workspaceFolders. When this happens, the working directory becomes env::current_dir(), which is the / in this particular case.

In this PR I refactored the code, and before falling back to env::current_dir(), we attempt to retrieve the home directory of the user, and use that one as working directory.

Test Plan

I manually tested the feature using a local project

@ematipico ematipico requested review from a team August 13, 2024 09:11
@github-actions github-actions bot added A-CLI Area: CLI A-Core Area: core A-LSP Area: language server protocol A-Changelog Area: changelog labels Aug 13, 2024
Copy link

codspeed-hq bot commented Aug 13, 2024

CodSpeed Performance Report

Merging #3649 will improve performances by 6.5%

Comparing feat/lsp-use-user-dir (c71554f) with main (10ef9d4)

Summary

⚡ 1 improvements
✅ 103 untouched benchmarks

Benchmarks breakdown

Benchmark main feat/lsp-use-user-dir Change
pure_9395922602181450299.css[cached] 4.1 ms 3.9 ms +6.5%

@Araxeus
Copy link

Araxeus commented Aug 13, 2024

Is it possible to set the working directory to the parent folder of the file?

The original issue was triggered when a single file was opened by a single client.

So if I open vscode with C://git/mysnippets/snippet.js the workdir should be C://git/mysnippets and not the home directory

(that way biome check "C://git/mysnippets/snippet.js" will have the same output as vscode)

@ematipico
Copy link
Member Author

It should, but unfortunately the LSP client doesn't provide such information, and the LSP server alone can't know which one is the working directory

@Araxeus
Copy link

Araxeus commented Aug 13, 2024

the LSP client doesn't provide such information

I don't know how it actually works - but can't the vscode extension pass the path itself?

this doesn't really feel like a real fix :/

@Araxeus
Copy link

Araxeus commented Aug 13, 2024

I think this might have potential to get fixed in biomejs/biome-vscode#200

the extension introduces an on-demand global LSP session dedicated to providing Biome features to Untitled files, and files opened in single-file mode.

@nhedger
Copy link
Member

nhedger commented Aug 13, 2024

I think this might have potential to get fixed in biomejs/biome-vscode#200

the extension introduces an on-demand global LSP session dedicated to providing Biome features to Untitled files, and files opened in single-file mode.

Indeed. The extension will create a dedicated LSP session where the parent folder of the file opened in single-file mode will be considered the root. This however may not necessarily be true for other LSP clients

@ematipico
Copy link
Member Author

the LSP client doesn't provide such information

I don't know how it actually works - but can't the vscode extension pass the path itself?

this doesn't really feel like a real fix :/

Ok, that's fair. I removed it.

However we should agree that the bug you filed and the "global/user configuration" are different :)

@nhedger
Copy link
Member

nhedger commented Aug 13, 2024

@ematipico, does env::current_dir() return the process's CWD? If so, I think it should be returned first instead of the home dir. When I start a new LSP session, I spawn the binary using the file's parent folder as the CWD. I cannot manually specify the root URI from the LSP client.

@ematipico
Copy link
Member Author

@ematipico, does env::current_dir() return the process's CWD?

Yeah, env::current_dir() calls unix getcwd.

think it should be returned first instead of the home dir.

But that's the actual bug 😅 When VSCode is opened with a single file using the CLI code file.js, rootUri and workspaceFolders are both None, and calling to env::current_dir() return the root system folder /, which we don't want unless it's the last resort.

Ironically, zed behaves differently, and when I call zed file.js, it is able to pass the correct rootUri and workspaceFolders, which is the parent folder that contains file.js

@nhedger
Copy link
Member

nhedger commented Aug 13, 2024

Ah sorry I wasn't super clear. In the current version of the extension we don't set a custom cwd which may explain why we get /. In v3 I'm explicitly passing the root of a project folder as the cwd.

I'll do some tests later to confirm my doubts

@ematipico
Copy link
Member Author

Oh, awesome! I am happy to hold off this PR then :)

@Araxeus
Copy link

Araxeus commented Aug 13, 2024

So this PR will just alter what happens in an edge case where the lsp client doesn't pass rootUri or workspaceFolders ?

If so then I'm not sure if this is needed, isn't it the lsp's client job to provide the directory? and if it doesn't then it probably indicates a bug in the lsp client implementation?

I don't think a user would expect the home directory to be used as CWD

IMO it should be an error for the lsp client not to pass a directory to work with, unless I'm missing something

@nhedger in v3 if I open vscode with nothing open, then create a new untitled file - will biome work? and if it will then what will be the cwd passed to the lsp server?

@ematipico
Copy link
Member Author

and if it doesn't then it probably indicates a bug in the lsp client implementation?

I don't think there's a spec for this case, so I am unsure if it's a VSCode bug, but it's possible. rootUri is marked as optional, as well as workspaceFolders.

I'll update the PR to use env::current_dir first, and then the home dir as fallback.

@Araxeus
Copy link

Araxeus commented Aug 13, 2024

rootUri is marked as optional, as well as workspaceFolders.

Feels weird, I wonder why that is… handling unsaved untitled files?

Maybe it shouldn't be optional - which would make lsp client implementation have to resolve that issue themselves

I'll update the PR to use env::current_dir first, and then the home dir as fallback.

isn't env::current_dir always defined? or do you mean fallback if it's equal to / exactly?

@nhedger
Copy link
Member

nhedger commented Aug 13, 2024

So this PR will just alter what happens in an edge case where the lsp client doesn't pass rootUri or workspaceFolders ?

If so then I'm not sure if this is needed, isn't it the lsp's client job to provide the directory? and if it doesn't then it probably indicates a bug in the lsp client implementation?

I don't think a user would expect the home directory to be used as CWD

IMO it should be an error for the lsp client not to pass a directory to work with, unless I'm missing something

@nhedger in v3 if I open vscode with nothing open, then create a new untitled file - will biome work? and if it will then what will be the cwd passed to the lsp server?

Yes. The extension will create a global LSP session, but no working directory will be set explicitly. I'm assuming it would default to / in that case.

@Araxeus
Copy link

Araxeus commented Aug 13, 2024

In this case, I propose that the lsp server and cli app should both default to the user directory if no config was found

lsp server would do it if the lsp client decides it (untitled unsaved files, custom workspace option, etc)

cli would do it if no config was found (in any parent dir) or if some kind of cli option was set (--global/-g?)

and then Biome could clearly state this in the documentation and effectively allow global configuration

another step would be to allow changing the global config directory - which is now proposed to be the user dir

@ematipico
Copy link
Member Author

Feels weird, I wonder why that is… handling unsaved untitled files?

Maybe it shouldn't be optional - which would make lsp client implementation have to resolve that issue themselves

It's part of the LSP spec: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#initialize

@ematipico ematipico closed this Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-CLI Area: CLI A-Core Area: core A-LSP Area: language server protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants