Repository navigation
Conversation
…tions on char boundaries
This branch has not been deployed
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.