Repository navigation
Fix the position of the end of a file - #1352
Conversation
| const lineCol = lineColumn(source, { origin: 0 }).fromIndex(Math.min(index, source.length - 1)); | ||
|
|
||
| // Not lineColumn's fromIndex, which has no position for the end of the file. | ||
| const before = source.slice(0, index); |
There was a problem hiding this comment.
I feel like that's not a performant way of doing that.
There was a problem hiding this comment.
Good call. It's a scan with indexOf now, with no allocation, and it stops at the index.
For what it's worth, the old lineColumn(source) was slower still: it split the whole file into lines on every call, and report calls it twice per offense.
| Case | Before this PR (line-column) | First version (slice + split) | Now (indexOf scan) |
|---|---|---|---|
| 50 KB file, 20 offenses | 1.0 ms | 0.3 ms | 0.2 ms |
| 1 MB file, 1,000 offenses | 652 ms | 290 ms | 193 ms |
| 20 MB file, 1 offense | 12.2 ms | 5.7 ms | 3.7 ms |
How it was measured: Node 24.15 on an M3 Pro. Each file repeats one ~55-character line of Liquid, and the offenses are spread evenly through it. Each offense makes two calls, start and end, as report does. Times are one run, after a warm-up. The scan gives the same positions as the slice + split version, checked on edge cases and across the 50 KB file.
A line table built once per file would turn each lookup into a binary search, but it needs a cache. It seemed worth adding only if large files with thousands of offenses turn out to matter.
charlespwd
left a comment
charlespwd
left a comment
There was a problem hiding this comment.
perf impact of this one is probably not good. source.slice for getPosition seems wrong.
bf22331 to
7f38a55
Compare
getPosition used line-column's fromIndex, which only has positions for characters, so it clamped the index to the last character. An offense ending at the end of a file ended one character short, or on the line before when the file ends with a newline. The index was right, but the CLI and the editor use the line and character. It now finds the line by scanning for "\n" up to the index, as line-column splits on "\n" only. That's also cheaper: line-column split the whole source into lines on every call, twice per offense. A negative index, like the end of an unclosed block, still has line and character -1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7f38a55 to
d9c843c
Compare
Stacked on #1351. Review that one first; this PR's base moves down the stack as the PRs below it merge.
What are you adding in this PR?
An offense that ends at the end of a file ended one character short, or on the line before when the file ends with a newline.
getPositionused line-column'sfromIndex, which only has positions for characters, not for the end of the file. So it clamped the index to the last character.The
indexwas right, but the CLI and the editor uselineandcharacter. The language server's diagnostics come from them, so editor underlines were short too.getPositionnow finds the line by scanning for\nup to the index, as line-column splits on\nonly. A negative index, like the end of an unclosed block, still has line and character -1.getOffsetis unchanged.It's also cheaper. line-column split the whole file into lines on every call, and
reportcalls it twice per offense. Scanning allocates nothing and stops at the index:All existing tests pass unchanged; none covered the end of a file.
What's next? Any followup issues?
#1347 is stacked on this. Its specs record the corrected positions.
Tophatting
npx vitest run packages/theme-check-common/src/utils/position.spec.ts{{ content_for_header | split: ',' }}, alone in a file, reports ContentForHeaderModification over the whole file. Before, the offense ended at[0, 36], on the last}; now it ends at[0, 37].Before you deploy
changeset🤖 Generated with Claude Code