fix: preserve whitespace around .. in prqlc highlight - #6232
Conversation
prql-bot
left a comment
There was a problem hiding this comment.
Self-review, so COMMENT rather than an approval. The core of the fix holds up — I rebuilt the binary and confirmed the snapshot in highlight_range_whitespace is exactly what it emits, and traced the padding arithmetic across bound, left-unbound, right-unbound, both-unbound, and a .. carrying line-leading indentation. Nothing here is blocking.
One substantive suggestion: split the surplus evenly rather than dumping it on the left. That makes take 1 .. 5 round-trip exactly instead of collapsing to take 1 .. 5, which narrows the residue the description calls out to genuinely asymmetric spacing. The other two are comment accuracy — LineWrap spans are wider than their text too, and the Range arm left in highlight_token_kind is unreachable rather than reachable-with-degraded-output.
Verification
Built target/debug/prqlc at 894d2be and ran the test input directly:
$ printf 'from x\ntake 1..5\ntake 1 .. 5\ntake 1 ..5\ntake 1.. 5\ntake 1 .. 5\n' \
| prqlc --color=never experimental highlight
from x
take 1..5
take 1 .. 5
take 1 ..5
take 1.. 5
take 1 .. 5
Matches the inline snapshot. With the even-split variant applied, the last line becomes take 1 .. 5 and the preceding four are unchanged; take 1 .. 5 (asymmetric) renders as take 1 .. 5, preserving width and binding.
The span claim checks out against the lexer: the range parser in lexer/mod.rs is whitespace().or_not().then(just("..")).then(whitespace().or_not()) with the span taken over the whole thing, and whitespace() is text::inline_whitespace() — so it can't cross a newline, and a .. at the start of an indented line correctly carries that indent in its span.
Applies the review suggestions on #6232: split the surplus evenly when both sides are unbound, so symmetric spacing round-trips exactly; and correct the two comments about `LineWrap` spans and the unreachable `Range` arm.
prql-bot
left a comment
There was a problem hiding this comment.
Self-review, so COMMENT rather than an approval.
The three suggestions from the last round are applied, and I re-verified them against a binary rebuilt at 1b40e85 — all five spacings in highlight_range_whitespace round-trip byte-for-byte, and both comment corrections match what the lexer actually does. I also worked through the cases the snapshot doesn't cover: a .. carrying line-leading indentation, an odd surplus, a trailing take 1.. at end of line, and tabs around the ... Every one preserves both the span width and the binding, so nothing here is blocking.
Two gaps, both of which I've pushed to the branch rather than leaving open, since I'm the author here:
- No
CHANGELOG.mdentry. This changes user-facingprqlcoutput, and the contributing guide asks for a line "if a change is user-facing"; the siblingprqlcfixes #6221 and #6223 both have one. Added under Fixes. - The lossy case isn't pinned by a test. The description calls out that asymmetric spacing is re-centred, but nothing in the snapshot exercises it — so a later change to how the surplus splits would alter that behaviour with no test moving. Added
take 1 .. 5→take 1 .. 5as a sixth line, with a doc comment saying why it's there.
Verification
Built target/debug/prqlc at 1b40e85 and ran the inputs the snapshot doesn't include:
$ printf 'from x\ntake 1 .. 5\ntake 1\t..\t5\ntake 1..\n' \
| prqlc --color=never experimental highlight
from x
take 1 .. 5
take 1 .. 5
take 1..
- Odd surplus (
1 .. 5, three spaces to split) comes back as1 .. 5— re-split, but the width and the unbound-on-both-sides binding are intact, which is the documented residue. - Tabs are rendered as spaces. That's pre-existing and consistent:
highlightalready emits every inter-token gap as" ".repeat(diff), so the whole output is space-normalised. The binding is unaffected, since a tab and a space are bothtext::inline_whitespace()to the lexer. take 1..with nothing after it isbind_right: trueandwidth == 2, sosurplusis0and no stray padding is emitted.
A .. at the start of an indented continuation line also round-trips exactly:
$ printf 'from x\nderive y = (1\n .. 5)\n' | prqlc --color=never experimental highlight
from x
derive y = (1
.. 5)
That's the both-unbound branch with an odd surplus of 1, and div_ceil puts the extra space on the left, which happens to be where it came from.
usize::div_ceil is stable since 1.73, under the workspace rust-version = "1.81.0", and lexer/lr.rs already uses it.
I checked for an existing helper before accepting the new highlight_range: Display for TokenKind renders a Range as '{left}..{right}' with single-space minimum padding, but it's quote-wrapped for diagnostics and has no access to the span, so it isn't reusable here.
cargo test -p prqlc --bin prqlc highlight passes with the added test line; cargo fmt --all --check is clean and prettier leaves the changelog entry unchanged.
prqlc experimental highlightprinted a bare..for every range token, dropping the whitespace either side. That whitespace is what decides whether the range binds, so the highlighted output could be a different program than the input:take 1 .. 5is a compile error, but highlight renders it astake 1..5, which compiles toLIMIT 5.The lexer folds the surrounding whitespace into the range token's own span (
lexer/mod.rs:123-137) and records it asbind_left/bind_right, which the parser then matches on (parser/expr.rs:335-351,parser/types.rs:50-52). Range is the only kind whose span is wider than its own text and whose extra width is load-bearing, so the newhighlight_rangetakes the span width alongside the two flags and pads the..back out — restoring the binding, and keeping the token at its full span width so the rest of the line holds its column.Verified with
cargo test -p prqlc(605 tests, all passing),cargo clippy -p prqlc --all-targets --no-default-features --features=default,lsp -- -D warnings, andcargo fmt --all --check.cargo-insta/cargo-nextestaren't available in the sandbox (#6144), so this is plaincargo teston theprqlcpackage rather than the fulltask prqlc:pull-request.Regression test and before/after
highlight_range_whitespaceinprqlc/prqlc/src/cli/highlight.rscovers the bound case plus four unbound spacings. Against the pre-fix binary it fails with every line collapsing totake 1..5:One residue is left, narrowed to asymmetric spacing only: when both sides are unbound the span records the total width but not how it splits, so the surplus is split evenly. Symmetric spacing round-trips exactly (
take 1 .. 5comes back as itself), while genuinely asymmetric spacing is re-centred —take 1 .. 5renders astake 1 .. 5— pinned by the last line ofhighlight_range_whitespace, so a future change to the split can't alter it silently. The width and the binding are preserved in both cases; only an asymmetric split is lost, and it isn't recoverable from the token.highlight_token_kind'sRangearm is now unreachable:highlightinterceptsRangebefore dispatching there, and aLineWraponly ever carries comments —line_wrap()builds itsVec<TokenKind>fromcomment()alone (lexer/mod.rs:256-269). The arm is kept only to keep the match exhaustive.codecov/patchis red at 86.95% (20 of 23 diff lines). All three missed lines are the unreachableRangearm inhighlight_token_kind— no input can reach it, so a test written to touch it wouldn't be exercising anything real.codecov/projectis unchanged at 93.88%.