Repository navigation
fix(lsp): debug assertions at range boundaries, keep client positions on char boundaries - #952
Merged
Merged
Conversation
- pgls.executeStatement indexed its arguments without checking them, so a call without arguments panicked. Missing or malformed arguments are now an InvalidParams error. - Documents of other schemes than file: shared the workspace document of the file with the same path: the git: side of a diff, every cell of a notebook. Closing the diff closed the file's document. They are now keyed by their whole URI. - Positions past the end of a line or of the document failed the whole request. They are clamped, as the LSP specifies, and an inlay hint that can't be placed is left out instead of failing the response.
…tions on char boundaries
psteinroe
changed the base branch from
fix/lsp-arguments-uris-positions
to
main
October 26, 2026 09:12
This was referenced Oct 30, 2026
psteinroe
added a commit
that referenced
this pull request
Nov 1, 2026
#1044) Follow-up to #906. The fix guard in `Analyser::analyse` applies each fix to its statement and drops it if the statement no longer parses. A fix with an edit after the end of its statement couldn't be checked that way and was kept unchecked. `banUncommittedTransaction`'s fix is one: it adds `commit;` after the last statement. If the file doesn't end with a semicolon, that fix turns ```sql begin; create table users (id bigint) ``` into `create table users (id bigint)\ncommit;`, which doesn't parse. The spec harness, which checks that every fix produces valid SQL, panics on this file. ### What changed - `AnalyserParams::source`: the text of the file. The workspace and the spec harness pass it. - With it, a fix that also edits after its statement is applied to the text of the file from the start of the statement to the end of the last statement the fix edits (or the last edit, if that's later), and that text must parse with the fix applied. If the text doesn't parse without the fix either, e.g. because a statement in it uses syntax of a newer Postgres, the fix can't be checked and is kept, as before. Without the source, such fixes are kept, as before. - Fixes inside their statement are checked on the statement as before. ### Tests - `fixes_after_the_statement_are_checked_on_the_file`: a `COMMIT` added after a statement with and without its semicolon, after a last statement without a semicolon (`;\ncommit;` is valid, `\ncommit;` isn't), inside the next statement, and in text that doesn't parse. - New spec `banUncommittedTransaction/no_semicolon_at_the_end.sql`: the diagnostic has no fix now. Without the change, the harness fails with "the fix `Add COMMIT at the end` produces invalid SQL". - `cargo test -p pgls_analyser`, `cargo test -p pgls_workspace`, `cargo clippy --workspace --all-targets -- -D warnings`. ### Not covered - `banUncommittedTransaction` could add `;\ncommit;` when the last statement has no semicolon, so this file still gets a fix. That's a change of the rule; here the guard only drops the invalid fix. - #952 (open) adds debug assertions on the fix edits at the same place; whichever merges second needs a small merge.
psteinroe
added a commit
that referenced
this pull request
Nov 1, 2026
) Follow-up to #952's "Not covered". A lint rule reports spans relative to its statement, and `Analyser::analyse` moves the diagnostic's span into the file. The spans of its details (`LinterDiagnostic::detail(Some(span), ..)` and `label`) stayed relative to the statement, while the printer and the LSP read them as file offsets. No rule passes a detail span today, but the first one that does would show its related information at the wrong place. ### What changed `LinterDiagnostic::move_into_file(statement_range)` moves the span of the diagnostic and the spans of its details into the file; `analyse` calls it where it moved the span before. The docs of `label`/`detail` say that the span is relative to the statement, like the diagnostic's span. #952 is open and conflicting with main; it doesn't cover this item, so this PR is based on main. Its `debug_assert_in_statement` could check the detail spans too once both are in. ### Tests `diagnostics_move_into_the_file`: a diagnostic with a span, a detail with a span and one without, and a diagnostic without a span (it points at the statement). `cargo test -p pgls_analyser`, `cargo clippy --workspace --all-targets -- -D warnings`.
psteinroe
added a commit
that referenced
this pull request
Nov 2, 2026
…1050) Follow-up to #952's "Not covered": the spans of typecheck and plpgsql_check diagnostics are computed from positions the database reports, so they need validation rather than debug assertions. Several of them could panic or point at the wrong text. ### What changed **Typecheck** (`pgls_typecheck`): Postgres reports the position of an error as a 1-based *character* position ([`parser_errposition`](https://github.com/postgres/postgres/blob/REL_18_6/src/backend/parser/parse_node.c#L106)). We used it as a byte offset (`pos - 1`), so the span was off after a non-ASCII character, and position 0 underflowed. It's now converted to a byte offset of the text we sent (`byte_offset_of_position`); a position outside of the text gives no precise span. **plpgsql_check** (`pgls_plpgsql_check::resolve_span`), for issues of a statement: - The line number was parsed with `expect` and `line_number - 1` underflowed for 0. A line number that isn't a positive number or isn't in the body now gives no span. - Line offsets counted `line.len() + 1`, which is one byte short per `\r\n`, so on files with Windows line endings the span started too early, and slicing the body there could panic on a non-ASCII character. - The statement keyword was found in `line.to_lowercase()` and the offset used in `line`. Lowercasing can change the length (`İ` becomes 3 bytes), so the offset could be wrong or inside a character. It's now an ASCII case-insensitive search on the line itself, like the query search already does, and `THEN` is found the same way. - The remaining slices of the body use `get`. **Workspace**: before a database span is moved into the file, `database_span` checks that it lies inside the statement and on char boundaries of its text. Otherwise the diagnostic points at the statement, like one without a span. ### Tests - `error_positions_are_character_positions` (typecheck). - `maps_statements_on_lines_with_crlf_and_non_ascii_text` and `ignores_line_numbers_that_are_not_in_the_body` (plpgsql_check); both fail against the old `resolve_span`. - `database_spans_must_fit_the_statement` (workspace). - `cargo test -p pgls_typecheck -p pgls_plpgsql_check -p pgls_workspace`, `cargo clippy --workspace --all-targets -- -D warnings`.
psteinroe
added a commit
that referenced
this pull request
Nov 2, 2026
The language server, the WASM bindings, `pgls_suppressions` and `pgls_diagnostics` each map offsets to lines on their own. The language server's `LineIndex` is the only one that also converts UTF-16 and UTF-32 columns, but it lives in `pgls_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 holds `LineIndex` together with `LineCol`, `WideLineCol`, `WideEncoding` and `WideChar`, and only depends on `pgls_text_size` and `rustc-hash`. `PositionEncoding` and the `ls_types` adapters (`from_lsp`, `to_lsp`) stay in `pgls_lsp`, since they're about what the client negotiated. The fields of `LineIndex` and `WideChar` are 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 in `lib.rs` and the tests in `tests.rs`. The WASM bindings and `pgls_suppressions` move onto the crate in follow-up PRs, because their line handling differs from the language server's and moving them changes behaviour: - WASM treats the `\r` of a `\r\n` as part of the line break (#969): a column past the end of a CRLF line clamps before the `\r`, and an offset between `\r` and `\n` becomes the end of the line. The language server clamps to the `\n` and reports a column after the `\r`, which the LSP doesn't allow. - `pgls_suppressions` treats a lone `\r` as a line break, while the language server and WASM only break at `\n`. Tests: the index-only tests (`out_of_bounds_line`, the ignored `test_every_chars`) moved with the index. The `ls_types` conversion tests stay in `pgls_lsp`. A new `line_breaks_and_wide_characters` test 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`/`LineIndexBuf` stay as they are. They're a public, borrowed slice type in `SourceCode<T, L>`, used only by the CLI's code frame printer and the `file:line:col` header, which count columns in characters and treat a lone `\r` as 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.
# Conflicts: # crates/pgls_line_index/src/line_index.rs
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.
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 #928.
Most of the bugs in the last review were ranges that were silently wrong after they crossed from one coordinate system into another: UTF-8 vs UTF-16, non-ASCII identifier spans, offsets through
''unescaping, fix edits outside their statement. This addsdebug_assert!s on both sides of those boundaries, so tests and fuzzers in debug builds fail on them instead of producing a slightly wrong hover or fix. Release builds are unchanged.Where
pgls_analyser): before a rule's span and fix edits are moved into the file, they must lie inside the statement text and on its char boundaries. Fix edits may also start at or after the end of the statement, since some fixes add aCOMMITafter it.RuleFix::applychecks the other side: the edits are sorted, don't overlap, and lie on char boundaries of the text they're applied to. That's the text of the file forfix_file.pgls_lsp):LineIndex::line_colasserts that the offset is inside the text and on a char boundary, andto_lsp::positionasserts that the position maps back to the same offset.to_lsp::text_editsasserts that the diff covers the whole text.TextEdit::new_stringasserts that its equal and deleted parts match the old text and cover all of it. ApplyingdidChangeasserts that the range is on char boundaries of the document.Bugs found on the way
None of the assertions fired in the
pgls_workspace,pgls_lsp,pgls_analyser(including the Postgres regression corpus),pgls_pretty_print,pgls_cliandpgls_text_edittests, nor in a throwaway workspace test over non-ASCII identifiers, quoted and dollar-quoted function bodies, and every code action position plusfix_file. Two input paths, though, would have broken the assertions with valid client input, and both panicked in release builds too:LineIndex::offset_clamped). Before, it became an offset inside the character, anddidChangepanicked inreplace_range. Other handlers could slice the text there too.from_lsp::text_rangereturns an error for a range that ends before it starts, sodidChangeskips that change like any other range it can't convert, instead of panicking.Tests:
positions_inside_a_character_are_its_start,ranges_that_end_before_they_start_are_an_erroranddocument_changes_with_invalid_ranges.Not covered
pgls_workspace(hint.offset + stmt.range.start()and the hover/semantic token references) are being reworked by the walk cache (perf(workspace): cache the walk of a document for hover, inlay hints and semantic tokens #930), so they get no assertions here. The LSP side still checks every offset they turn into.LinterDiagnostic::detail(Some(span), ..)) aren't moved into the file. No rule passes a span today, but the first one that does will show its related information at the wrong place.