Skip to content

fix: preserve whitespace around .. in prqlc highlight - #6232

Merged
max-sixty merged 3 commits into
mainfrom
fix/highlight-range-whitespace
Aug 25, 2026
Merged

fix: preserve whitespace around .. in prqlc highlight#6232
max-sixty merged 3 commits into
mainfrom
fix/highlight-range-whitespace

Conversation

@prql-bot

@prql-bot prql-bot commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

prqlc experimental highlight printed 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 .. 5 is a compile error, but highlight renders it as take 1..5, which compiles to LIMIT 5.

The lexer folds the surrounding whitespace into the range token's own span (lexer/mod.rs:123-137) and records it as bind_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 new highlight_range takes 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, and cargo fmt --all --check. cargo-insta/cargo-nextest aren't available in the sandbox (#6144), so this is plain cargo test on the prqlc package rather than the full task prqlc:pull-request.

Regression test and before/after

highlight_range_whitespace in prqlc/prqlc/src/cli/highlight.rs covers the bound case plus four unbound spacings. Against the pre-fix binary it fails with every line collapsing to take 1..5:

    5     5 │ take 1..5
    6       │-take 1 .. 5
    7       │-take 1   ..5
    8       │-take 1..   5
    9       │-take 1  ..  5
          6 │+take 1..5
          7 │+take 1..5
          8 │+take 1..5
          9 │+take 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 .. 5 comes back as itself), while genuinely asymmetric spacing is re-centred — take 1 .. 5 renders as take 1 .. 5 — pinned by the last line of highlight_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's Range arm is now unreachable: highlight intercepts Range before dispatching there, and a LineWrap only ever carries comments — line_wrap() builds its Vec<TokenKind> from comment() alone (lexer/mod.rs:256-269). The arm is kept only to keep the match exhaustive.

codecov/patch is red at 86.95% (20 of 23 diff lines). All three missed lines are the unreachable Range arm in highlight_token_kind — no input can reach it, so a test written to touch it wouldn't be exercising anything real. codecov/project is unchanged at 93.88%.

@prql-bot prql-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread prqlc/prqlc/src/cli/highlight.rs Outdated
Comment thread prqlc/prqlc/src/cli/highlight.rs Outdated
Comment thread prqlc/prqlc/src/cli/highlight.rs Outdated
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 prql-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.md entry. This changes user-facing prqlc output, and the contributing guide asks for a line "if a change is user-facing"; the sibling prqlc fixes #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 .. 5take 1 .. 5 as 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 as 1 .. 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: highlight already 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 both text::inline_whitespace() to the lexer.
  • take 1.. with nothing after it is bind_right: true and width == 2, so surplus is 0 and 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.

@max-sixty
max-sixty merged commit 5b05204 into main Aug 25, 2026
38 of 39 checks passed
@max-sixty
max-sixty deleted the fix/highlight-range-whitespace branch August 25, 2026 16:29
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.

2 participants