Repository navigation
refactor(line_index): one conversion of the character positions Postgres reports - #1107
Merged
Merged
Conversation
edjubert
approved these changes
Nov 2, 2026
psteinroe
added a commit
that referenced
this pull request
Nov 6, 2026
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_range` for that, and only the WASM one (#969) took in the whole `\r\n` when only one half of it changed. In the language server, such an edit ended between the `\r` and the `\n`, a position the LSP doesn't allow, so it lost the half that changed: ```sql -- the fixes turn "select 1;\r\nselect 2;" into "select 1;\nselect 2;" -- before: the edit is empty at (0, 9), the end of line 0, and the \r stays -- now: the edit replaces the \r\n at (0, 9)-(1, 0) with \n ``` `changed_range` now lives in `pgls_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_whole` in `pgls_lsp` applies the fix-all edit through `from_lsp` for CRLF and LF changes and a non-ASCII line; it fails without this change. `changed_range_keeps_the_common_start_and_end` (moved from `pgls_lsp`) and `changed_range_keeps_crlf_whole` test the function in `pgls_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`, and `bun run build && bun test` in 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 to `pgls_line_index`; whichever merges second gets a small conflict in the module list of `lib.rs`, where both lines stay.
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.
Postgres reports the position of an error as a 1-based character position (
parser_errposition), 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 callpgls_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
usizeand now gets aTextSize, like the other two, which only differs for texts over 4 GiB. ItsOneBasedCharOffsetnewtype 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_positionsmoved topgls_line_indexascharacter_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.