Repository navigation
fix(workspace): validate the spans of diagnostics from the database - #1050
Merged
Merged
Conversation
Collaborator
Author
|
All these offset helpers should also be standardised in the project |
# Conflicts: # crates/pgls_workspace/src/workspace/server.tests.rs
# Conflicts: # crates/pgls_plpgsql_check/src/diagnostics.rs # crates/pgls_typecheck/src/diagnostics.rs
psteinroe
added a commit
that referenced
this pull request
Nov 2, 2026
…res reports (#1107) 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)), one past the last character for an error at the end of the input. Three places converted it to a byte offset on their own: typecheck (`byte_offset_of_position`, #1050), plpgsql_check (`byte_offset_from_one_based_char_offset`) and the statement execution code action (`rejected`). They now call `pgls_line_index::offset_of_char_position`, next to the other position conversions. There's no behaviour change: the three copies did the same thing. 0 and positions past the end of the text give no offset, and the position after the last character is the end of the text. plpgsql_check computed the offset as a `usize` and now gets a `TextSize`, like the other two, which only differs for texts over 4 GiB. Its `OneBasedCharOffset` newtype is gone with the function. AGENTS.md and ARCHITECTURE.md now say that positions are converted with `pgls_line_index`, not by hand. Tests: the typecheck test `error_positions_are_character_positions` moved to `pgls_line_index` as `character_positions_are_one_based`, with an empty text and position 1 added. The plpgsql_check tests for query and compile error positions with non-ASCII text run against the shared function unchanged. `cargo test -p pgls_line_index -p pgls_typecheck -p pgls_plpgsql_check -p pgls_workspace`, `cargo clippy --workspace --all-targets -- -D warnings`. Not covered: the other position helpers stay where they are for now. The LSP position conversions in the WASM bindings move onto the line index in #1070, and the remaining copies (fix-all edits in the language server and WASM, the CLI's line numbers, the regression fixtures' 1-based character columns) are follow-ups.
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.
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). 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:expectandline_number - 1underflowed for 0. A line number that isn't a positive number or isn't in the body now gives no span.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.line.to_lowercase()and the offset used inline. 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, andTHENis found the same way.get.Workspace: before a database span is moved into the file,
database_spanchecks 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_textandignores_line_numbers_that_are_not_in_the_body(plpgsql_check); both fail against the oldresolve_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.