Ignore invalid file URI scheme in parse_file_path et al.#430
Ignore invalid file URI scheme in parse_file_path et al.#430nrc merged 1 commit intorust-lang:masterfrom
Conversation
src/actions/mod.rs
Outdated
| // we don't want to crash the RLS in case a client opens a file under different URI scheme | ||
| // like with git:/ or perforce:/ (Probably even http:/? We currently don't support remote schemes). | ||
| macro_rules! ignore_non_file_uri { | ||
| ($expr: expr; $log_name: expr; $uri: expr) => { |
There was a problem hiding this comment.
Can you use commas rather than semi-colons here?
There was a problem hiding this comment.
I actually can. Previously when I was working on macros I encountered some ambiguities when separating exprs with commas, but it's fine here. Updated
| // TODO: Support non-`file` URI schemes in VFS. We're currently ignoring them because | ||
| // we don't want to crash the RLS in case a client opens a file under different URI scheme | ||
| // like with git:/ or perforce:/ (Probably even http:/? We currently don't support remote schemes). | ||
| macro_rules! ignore_non_file_uri { |
There was a problem hiding this comment.
I would probably move the call to parse_file_path into this macro. However, what to do about spans? Maybe call this macro inside the span parsing code and propagate an error later? If that is too much work we could have a macro that parses a uri string to a path or does the error handling like this and another than just does this.
There was a problem hiding this comment.
Maybe call this macro inside the span parsing code and propagate an error later?
I already propagate the error here. Do you mean something different?
If that is too much work we could have a macro that parses a uri string to a path or does the error handling like this and another than just does this.
Do you mean something like separate parse_file_path_ignore! and convert_pos_to_span_ignore! kind of macros that do what the original functions do (without the unwrap) but still expand to include return; at call site if a resulting error is UrlFileParseError::InvalidScheme?
I think it's okay as a short-term fix, because it's somewhat flexible and doesn't compilate existing parsing/conversion functions, but I'm fine with whatever and can change it, just tell me which change would you prefer 🙂
There was a problem hiding this comment.
Actually convert_pos_to_span requires doc only to retrieve a file Path and uses only that. Maybe we could change the function signature to accept Path instead, revert return type so it returns Span as before (so no error handling would be needed there) and then move the parse_file_path into the ignore_non_file_uri macro as suggested?
4bc3d97 to
a9276bc
Compare
|
Pushed new version. Now there are 2 macros, Moved parsing file name out of |
|
Looks good, thanks! |
(Somewhat) fixes #421 (and related).
This is a QoL (short-term) fix implemented as per @nrc's #421 (comment):
This basically introduces
UrlFileParseErrorimplementingErrorand it's now returned in aResult<T, UrlFileParseError>forparse_file_pathandconvert_pos_to_span, which uses it.On an error that's related to invalid URI scheme, it just returns from the handler function.
$log_nameand$urivariables in macro are used only for the underlyingtrace!macro call and I can remove them if it makes things unnecessarily verbose (repeating function name as literal, separating uri binding for clarity etc.).I know that's not a complete fix, but at least it doesn't crash from what I've seen and properly works if a client edits the local file in
Source Controloption in VS Code (btw the non-filebuffer is read-only in VS Code).Should I make a separate test case for non-
fileURIs?