Repository navigation
fix(lsp): keep a CRLF line break whole in the fix-all edit - #1108
Merged
Merged
Conversation
The language server's LineIndex converts offsets to line/column positions in UTF-8, UTF-16 and UTF-32. It lives in pgls_lsp, which pulls in tokio and tower-lsp, so the WASM bindings and pgls_suppressions couldn't use it and have their own line handling. Move it, with LineCol, WideLineCol, WideEncoding and WideChar, into a small crate that only depends on pgls_text_size and rustc-hash. No behaviour change.
The WASM bindings had their own position conversion, which treats the \r of a \r\n as part of the line break (#969), while the language server's index clamped a column past the end of a CRLF line to the \n and reported an offset between \r and \n as a column after the \r. Neither is a position the LSP allows. The shared LineIndex now follows the WASM rules: the end of a line is before its \r\n, an offset between \r and \n is the end of its line, and an offset past the end of the text is the end of the text. A column inside a character is the start of the character. The WASM bindings keep a LineIndex per document and convert positions with it.
Keep the index in line_index.rs, nearly unchanged, with the position types in lib.rs and the tests in tests.rs, so that git still follows the file from pgls_lsp when merging branches that change it.
…/wasm-line-index # Conflicts: # crates/pgls_line_index/src/lib.rs
# Conflicts: # crates/pgls_lsp/src/session.rs
edjubert
approved these changes
Nov 2, 2026
# Conflicts: # ARCHITECTURE.md # crates/pgls_line_index/src/lib.rs # crates/pgls_line_index/src/line_index.rs # crates/pgls_lsp/src/adapters/mod.rs
# Conflicts: # crates/pgls_line_index/src/lib.rs # crates/pgls_line_index/src/tests.rs
This file contains hidden or 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
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.
Stacked on #1070.
The fix-all code action replaces only the part of the file that its fixes changed. The language server and the WASM bindings each had their own
changed_rangefor that, and only the WASM one (#969) took in the whole\r\nwhen only one half of it changed. In the language server, such an edit ended between the\rand the\n, a position the LSP doesn't allow, so it lost the half that changed:changed_rangenow lives inpgls_line_index, next to the rule that a position can't point into a\r\n, and both use it. The WASM bindings keep their behaviour; the language server gets the widening. Each caller still turns the range into an LSP edit in its own position types (fix_all_edit).Tests:
fix_all_edits_keep_crlf_wholeinpgls_lspapplies the fix-all edit throughfrom_lspfor CRLF and LF changes and a non-ASCII line; it fails without this change.changed_range_keeps_the_common_start_and_end(moved frompgls_lsp) andchanged_range_keeps_crlf_wholetest the function inpgls_line_index; the WASM test from #969 runs against it unchanged.cargo test -p pgls_line_index -p pgls_lsp -p pgls_wasm,cargo clippy --workspace --all-targets -- -D warnings, andbun run build && bun testin the WASM package (49 pass).Not covered:
to_lsp::text_edits, which formatting uses, still doesn't widen a diff that changes one half of a\r\n(see #1070). #1107 also adds a module topgls_line_index; whichever merges second gets a small conflict in the module list oflib.rs, where both lines stay.