Skip to content

fix(lsp): debug assertions at range boundaries, keep client positions on char boundaries - #952

Open
psteinroe wants to merge 1 commit into
fix/lsp-arguments-uris-positionsfrom
chore/range-assertions
Open

psteinroe wants to merge 1 commit into
fix/lsp-arguments-uris-positionsfrom
chore/range-assertions

Conversation

@psteinroe

Copy link
Copy Markdown
Collaborator

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 adds debug_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

  • Statement → file (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 a COMMIT after it. RuleFix::apply checks 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 for fix_file.
  • File offsets → LSP positions (pgls_lsp): LineIndex::line_col asserts that the offset is inside the text and on a char boundary, and to_lsp::position asserts that the position maps back to the same offset. to_lsp::text_edits asserts that the diff covers the whole text.
  • Text edits: TextEdit::new_string asserts that its equal and deleted parts match the old text and cover all of it. Applying didChange asserts 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_cli and pgls_text_edit tests, nor in a throwaway workspace test over non-ASCII identifiers, quoted and dollar-quoted function bodies, and every code action position plus fix_file. Two input paths, though, would have broken the assertions with valid client input, and both panicked in release builds too:

// "select 'é';", UTF-8 positions: a column inside `é`
didChange range (0,9)-(0,10) -> panic: byte index 9 is not a char boundary
// any encoding: a range that ends before it starts
didChange range (0,5)-(0,2)  -> panic in TextRange::new (assertion failed: start.raw <= end.raw)
  • A client column inside a character, i.e. a UTF-8 column inside a multi-byte character or a UTF-16 column between the halves of a surrogate pair, now means the start of that character (LineIndex::offset_clamped). Before, it became an offset inside the character, and didChange panicked in replace_range. Other handlers could slice the text there too.
  • from_lsp::text_range returns an error for a range that ends before it starts, so didChange skips 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_error and document_changes_with_invalid_ranges.

Not covered

  • References and inlay hint offsets in 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.
  • Typecheck and plpgsql_check spans come from the database, so they need validation rather than assertions.
  • Diagnostic detail spans (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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant