improve handling of different URI schemes over LSP #9633
Conversation
the-mikedavis
added
A-language-server
|
Oh I always thought that method did that check automatically but apparently it does not according to the docs. Good catch |
helix-view/src/handlers/lsp.rs
Outdated
| Err(err) => { | ||
| let err = format!("{err}: {uri}"); | ||
| log::error!("{err}"); | ||
| self.set_error(err); |
There was a problem hiding this comment.
I'm not sure we need this error on the statusline - I think the error line in the log is enough
There was a problem hiding this comment.
agreed although we do statusline report for all the related errors in the file, and did for the FilePathError::UnableToConvert case before i refactored it out.
the-mikedavis
added
S-waiting-on-author
helix-term/src/commands/lsp.rs
Outdated
| fn location_to_file_location(location: &lsp::Location) -> Option<FileLocation> { | ||
| match uri_to_file_path(&location.uri) { | ||
| Ok(path) => { | ||
| let line = Some(( | ||
| location.range.start.line as usize, | ||
| location.range.end.line as usize, | ||
| )); | ||
| Some((path.into(), line)) | ||
| } | ||
| Err(FilePathError::UnsupportedScheme) => None, | ||
| Err(FilePathError::UnableToConvert) => unreachable!( | ||
| "file path should be able to be acquired from URI: {}", | ||
| location.uri | ||
| ), | ||
| } |
There was a problem hiding this comment.
I don't really think the panic is correct here this actually caused crashes in the past. The real fix is #7367 not sure what to do with this until then.
There was a problem hiding this comment.
i've edited this not to panic to air on the safe side, so both simply return None. note that if this crash is pervasive enough, #7367 isn't a complete fix since this function is also used for goto defintion and friends.
the-mikedavis
added
S-waiting-on-review
|
I can confirm that this patch, at this moment, does not solve the problem with helix not showing lsp detected errors under Windows, while #7367 solves the problem. |
|
There is a little overlap between how we solve the problems actually. We talked about this internally and we'd prefer to introduce a custom URI type and take care of invalid LSP URLs in I've pushed up #9652 re-using most of this work |
|
closing in favour of #9652. |
Objective
url::Url::to_file_pathdoes not check which scheme the URI adheres to, meaning currently when a language server sends us a URI containing an unrecognized scheme, we blindly treat it as if it were a file path, leading to strange results.This was causing issues for me using csharp-ls since when it resolves definitions over assembly boundaries, it will return a
csharp:/metadata/foo/bar/Baz.csURI, which a willing client can use to get readonly, decompiled definition info. In the current version, helix opens a blank buffer at/metadata/foo/bar/Baz.cs.Solution
This PR introduces a
uri_to_file_pathmethod which performs the expected behaviour, first checking the scheme, and changes all relevant uses ofUrl::to_file_pathto go via the new method instead and error without panicking when an unexpected scheme is found.Details
It appears the LSP spec says nothing about which schemes are valid, so I don't think csharp-ls is at fault here (though I wouldn't expect us to support the "csharp" scheme without plugins).
Note
Strangely, while csharp-ls uses its own scheme, omnisharp's URIs are of the form
file:///$metadata$/foo/bar/Baz.cs. It seems they adhere strongly to VSCode's lsp model which lets an extension act as middleware to intercept that URI to use other behaviour. Ergo, this PR does not fix the issue for omnisharp.