robustly handle invalid LSP ranges #6512
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pascalkuthe
force-pushed
the
robust_lsp_range
branch
from
4b5f548 to
0c4b15e
Compare
pascalkuthe
added
C-bug
pascalkuthe
force-pushed
the
robust_lsp_range
branch
from
0c4b15e to
a402e2a
Compare
pascalkuthe
added
the
S-waiting-on-review
Comment on lines
+132
to
+136
| // If it extends past the end, truncate it to the end. This is because the | ||
| // way the LSP describes the range including the last newline is by | ||
| // specifying a line number after what we would call the last line. | ||
| log::warn!("LSP position {pos:?} out of range assuming EOF"); | ||
| return Some(doc.len_chars()); |
There was a problem hiding this comment.
This is #6022, right? And this change brings us in line with VSCode's handling of this edge-case?
There was a problem hiding this comment.
exactly vscode behaves the same and the comment is from neovim so they also behave the same
the-mikedavis
previously approved these changes
Mar 31, 2023
pascalkuthe
force-pushed
the
robust_lsp_range
branch
from
a402e2a to
799075a
Compare
archseer
reviewed
Apr 3, 2023
| offset_encoding: OffsetEncoding, | ||
| ) -> Option<Range> { | ||
| // This is sort of an edgecase. It's not clear from the spec how to deal with | ||
| // ranges where end < start. They don't make much sense but vscode simply caps start to end | ||
| // and because it's not specified quite a few LS rely on this as a result (for example the TS server) |
archseer
approved these changes
Apr 3, 2023
Triton171
pushed a commit
to Triton171/helix
that referenced
this pull request
Jun 18, 2023
wes-adams
pushed a commit
to wes-adams/helix
that referenced
this pull request
Jul 4, 2023
smortime
pushed a commit
to smortime/helix
that referenced
this pull request
Jul 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix crashes with the TS language server reported on matrix
The Problem is that the way helix deals with invalid LSP ranges is different from other editors right now. Helix currently panics if the start of a LSP range is after the end of the range. The standard doesn't actually state that this is forbidden. VSCode simply sets
start=endin that case and there seem to be a few servers relying on this sadly. I matched that behavior but logged an error to make sure this is not missed when tracking down issues. Alternatively we could discard such ranges. We definitely shouldn't panic tough.Similarly, we now allow ranges with line numbers past the end (simply treated as EOF). VSCode behaves the same and so does neovim. I copied their comment here: