Repository navigation
refactor(lsp): move the line index into a pgls_line_index crate - #1063
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.
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.
# Conflicts: # crates/pgls_lsp/src/session.rs
psteinroe
added a commit
that referenced
this pull request
Nov 4, 2026
…1125) The CLI's code frames, its `file:line:col` header and the JSON, GitLab, JUnit and GitHub reporters get their line numbers from `pgls_diagnostics`' `SourceFile`, which ended a line at a lone `\r`. Everything else only ends lines at `\n`: the shared `pgls_line_index` (#1063), the language server, WASM, and the diff that `check` prints for formatting (`process_file/format.rs`). So for a file with a lone `\r`, the CLI's own diff and its diagnostics disagreed about the line numbers. `LineIndexBuf::from_source_text` now takes the line starts from `pgls_line_index::LineIndex`, which gets a `line_starts()` accessor for that, so the rule lives in one place. Columns stay what they were: 1-based characters from the start of the line. A lone `\r` is now a character of its line; the code frame already prints it as `␍`, and `char_width` now counts it as one column so the `^` marker under it lines up. This changes what the CLI prints for files with a lone `\r` (old Mac line endings, or a stray `\r`): their diagnostics move to the line an editor or the LSP shows, for example `1:11` instead of `2:1`. Files with `\n` or `\r\n` line endings print the same as before; no existing snapshot changed. Tests: - `pgls_diagnostics`: `a_lone_carriage_return_does_not_end_a_line` replaces the test that pinned the old behaviour, and `location_counts_lines_at_line_feeds_only` checks `SourceFile::location` with a lone `\r`, a `\r\n` and a 2-byte character. - `pgls_cli`: `check_lone_carriage_return_does_not_end_a_line_snapshot` runs `check` on such a file with the default, JSON, GitLab, JUnit and GitHub reporters. Without the fix, all of them report line 2. Not covered: `pgls_suppressions` still treats a lone `\r` as a line break.
psteinroe
added a commit
that referenced
this pull request
Nov 6, 2026
…LF line breaks (#1070) Stacked on #1063. The WASM bindings converted LSP positions with their own functions, which #969 fixed for CRLF files. Now they use the shared `pgls_line_index::LineIndex`, like the native language server, and the index takes over the rules #969 introduced. This changes behaviour in the native language server for positions next to a `\r\n`: ```sql select 1;\r\n select 2; -- (0, 100), a column past the end of line 0: -- before: offset 10, between the \r and the \n; now: offset 9, before the \r -- offset 10, between the \r and the \n: -- before: (0, 10), a column after the \r; now: (0, 9), the end of the line ``` The LSP says a position can't point between `\r` and `\n`, and that a column past the end of a line means the end of the line, which doesn't include its line break. What the index does now: - The end of a line is the start of its line break, so before the `\r` of a `\r\n`. It records the offsets of the `\n`s that follow a `\r`, so LF files pay nothing extra. - `line_col` turns an offset between `\r` and `\n` into the end of its line, and an offset past the end of the text into the end of the text. - `offset_clamped` turns a column inside a character into the start of that character. Before, the native language server returned an offset inside the character, and the WASM bindings moved to the start of the next character. The start matches what #952 does for the native language server, so `select '😀';` at UTF-16 column 9 (inside the surrogate pair) is offset 8 in both now. That's the only change for the WASM bindings. - A lone `\r` still doesn't end a line, in both. The WASM bindings keep a `LineIndex` per document, built on `didOpen` and `didChange`, like the native `Document`, instead of scanning the text on every conversion. `changed_range` still widens a fix-all edit that changes only one half of a `\r\n`, since that's about the edit, not the conversion. Tests: `line_breaks_and_wide_characters` in `pgls_line_index` now pins the new behaviour for `\r\n` (end of line, offset between `\r` and `\n`), a lone `\r`, an offset past the end, and columns inside 2-byte and 4-byte characters in UTF-8 and UTF-16. `crlf_belongs_to_the_line_break` checks the same through the native `from_lsp`/`to_lsp` and fails without this change. The WASM conversion tests from #969 run against the shared index unchanged, apart from the surrogate pair case above, plus a new lone `\r` test. `cargo test -p pgls_line_index -p pgls_lsp -p pgls_wasm`, `cargo clippy --workspace --all-targets -- -D warnings`, and the WASM package's `bun run build && bun test` (49 pass). Not covered: the native `to_lsp::text_edits` doesn't widen edits that change only one half of a `\r\n` the way the WASM fix-all does, so such an edit (e.g. `\r\n` to `\n`) becomes an empty range. Before, it ended at a column after the `\r`, which editors clamp to the same empty range. **Update:** merged main, which brought #952's debug assertions. `to_lsp::position` now asserts that a position maps back to the offset of its line and column, so an offset between the `\r` and the `\n` (mapped to the end of the line) passes. `LineIndex::line_col` keeps #952's assertion that the offset is inside the text, so the clamp of an offset past the end only applies in release builds, and the `line_col(100)` case is out of the `pgls_line_index` test.
psteinroe
added a commit
that referenced
this pull request
Nov 9, 2026
…1072) Stacked on #1063. The suppressions parser numbers lines with `str::lines`, which ends lines at `\n` (and drops the `\r` of a `\r\n`), but looks up their offsets and the lines of diagnostics in its own line index, which also ended a line at a lone `\r`. After a lone `\r`, the two disagree, and a line suppression points at the wrong text and doesn't apply to the line below it: ```sql select '\r'; -- pgls-ignore lint select 1; -- still reported ``` The parser now uses `pgls_line_index::LineIndex`, which ends lines at `\n` only, like `str::lines`, the language server and the WASM bindings. Its own `line_index.rs` is gone. `LineIndex` gets a `Default` (the index of an empty text), since `Suppressions` derives it. This changes behaviour only for files with a lone `\r`: lines after it are numbered one lower than before, which is what editors that end lines at `\n` show too. `\r\n` and `\n` files are numbered as before. Tests: `a_lone_carriage_return_does_not_end_a_line` checks that the suppression above applies and covers the comment's text; it fails without this change. `line_breaks_and_wide_characters` in `pgls_line_index` (#1063) pins the index's line numbers for `\r\n`, a lone `\r` and multi-byte characters. `cargo test -p pgls_suppressions -p pgls_line_index`, `cargo clippy --workspace --all-targets -- -D warnings`. Merging with #1008: that stack also edits `crates/pgls_suppressions/src/line_index.rs` (`line_for_offset` with `partition_point`). Whichever merges second gets a modify/delete conflict there; keep the deletion, since the shared index already looks lines up with `partition_point`. `parser.rs` merges cleanly, and the `Suppressions::default()` it returns early works with the new `Default`.
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.
The language server, the WASM bindings,
pgls_suppressionsandpgls_diagnosticseach map offsets to lines on their own. The language server'sLineIndexis the only one that also converts UTF-16 and UTF-32 columns, but it lives inpgls_lsp, which pulls in tokio and tower-lsp, so #969 fixed the WASM conversion in place instead of reusing it.This moves it into a new crate,
pgls_line_index, as a first step towards one implementation. The crate holdsLineIndextogether withLineCol,WideLineCol,WideEncodingandWideChar, and only depends onpgls_text_sizeandrustc-hash.PositionEncodingand thels_typesadapters (from_lsp,to_lsp) stay inpgls_lsp, since they're about what the client negotiated. The fields ofLineIndexandWideCharare now private; nothing outside the index used them.There's no behaviour change. The index keeps its own module,
line_index.rs, almost unchanged (imports and field visibility), so git follows the file when branches that change it (#952) merge main. The position types are inlib.rsand the tests intests.rs.The WASM bindings and
pgls_suppressionsmove onto the crate in follow-up PRs, because their line handling differs from the language server's and moving them changes behaviour:\rof a\r\nas part of the line break (fix(wasm): convert positions correctly in files with CRLF line endings #969): a column past the end of a CRLF line clamps before the\r, and an offset between\rand\nbecomes the end of the line. The language server clamps to the\nand reports a column after the\r, which the LSP doesn't allow.pgls_suppressionstreats a lone\ras a line break, while the language server and WASM only break at\n.Tests: the index-only tests (
out_of_bounds_line, the ignoredtest_every_chars) moved with the index. Thels_typesconversion tests stay inpgls_lsp. A newline_breaks_and_wide_characterstest pins the current behaviour for a\r\n, a lone\r, and 2- and 4-byte characters in UTF-16 and UTF-32.Not covered:
pgls_diagnostics'LineIndex/LineIndexBufstay as they are. They're a public, borrowed slice type inSourceCode<T, L>, used only by the CLI's code frame printer and thefile:line:colheader, which count columns in characters and treat a lone\ras a line break. Switching them to this index wouldn't share any column conversion, and it would change the line numbers the CLI prints for files with a lone\r, so I left them out.