-
-
Notifications
You must be signed in to change notification settings - Fork 763
fix(language_server): handle invalid root URI gracefully #17376
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
base: main
Are you sure you want to change the base?
fix(language_server): handle invalid root URI gracefully #17376
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a panic that occurred when the language server received an invalid rootUri during initialization (e.g., file:// with no path). The fix gracefully handles URI conversion failures by making tool builders return Option and filtering out failed tool builds.
Key changes:
- Modified
ToolBuilder::build_boxedto returnOption<Box<dyn Tool>>instead ofBox<dyn Tool> - Updated
ServerLinterBuilder::buildandServerFormatterBuilder::buildto returnOptionand handle invalid URIs with the?operator - Added graceful fallback behavior in
WorkspaceWorker::is_responsible_for_uriandstart_workerto handle invalid root URIs
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
crates/oxc_language_server/src/tool.rs |
Changed build_boxed trait method signature to return Option<Box<dyn Tool>> with updated documentation |
crates/oxc_language_server/src/worker.rs |
Updated is_responsible_for_uri to return false for invalid URIs and modified start_worker to filter out failed tool builds using filter_map |
crates/oxc_language_server/src/linter/server_linter.rs |
Changed build and build_boxed to return Option, added error logging for invalid URIs, and removed outdated panic documentation |
crates/oxc_language_server/src/formatter/server_formatter.rs |
Changed build and build_boxed to return Option, added error logging for invalid URIs, and removed outdated panic documentation |
crates/oxc_language_server/src/tests.rs |
Updated FakeToolBuilder to return Some(Box::new(FakeTool)) and adjusted test code to work with the new Option return type |
crates/oxc_language_server/src/linter/tester.rs |
Added .expect("test requires valid root URI") to ensure tests fail clearly if the root URI is invalid |
crates/oxc_language_server/src/formatter/tester.rs |
Added .expect("test requires valid root URI") to ensure tests fail clearly if the root URI is invalid |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sysix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain a bit more why this is needed?
I would never expect an invalid file URI for workspace/root URI's:)
@camc314 I do not think we will support non-file URIs for a tool base.
If we are 100% sure, we should probably add a check in the initialize request and return an Err instead of panicking.
|
@Sysix I was debugging oxfmt not formatting on save in Zed, noticed an LSP crashed logand traced back to |
2c1204b to
7ffcc21
Compare
When LSP clients send an invalid rootUri (e.g., 'file://' with no path), to_file_path() returns None. Previously this caused a panic via unwrap(). Now build_boxed() returns Option<Box<dyn Tool>>, allowing worker to skip tools that fail to initialize. Server responds with capabilities instead of crashing.
7ffcc21 to
da003e0
Compare
…ris are not valid file paths (#17388) Follow up from #17376 (review) Tell the client, the server will not be started when we can create "cwd" for the tools.
camc314
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we don't know what case causes this, it seems reasonable to handle by retuning an err (over unwrapping)
|
Hmm I'm thinking the latest vscode failure isn't a flake |
String prefix matching incorrectly matched file:///path/test_workspace_second against root file:///path/test_workspace. Path::starts_with respects directory boundaries.
|
@camc314 fixed the vscode failur, reverted to path based comparison with error handling and added regression test in case it makes sense |
|
Because
After #17388 I am not sure, if this PR is needed anymore. |
Summary
rootUriis invalid (e.g.,file://with no path)to_file_path().unwrap()failed inserver_formatter.rsandserver_linter.rsToolBuilder::build_boxedto returnOption<Box<dyn Tool>>filter_map()to skip tools that fail to buildRepro (before fix)
After fix
Server returns capabilities JSON and continues running.