Fix rule end offset when spaces precede its own semicolon - #2135
Conversation
Signed-off-by: maximilliangrand <214999687+maximilliangrand@users.noreply.github.com>
|
The one failing check ( That fixture lives in With the two in sync the numbers line up exactly: So once a |
|
Of course, I will release |
|
Thanks! I will release it a little later together with #2136 |
The invariant
source.end.offsetof a non-Rootnode is the exclusive end of the node, so thatinput.css.slice(node.source.start.offset, node.source.end.offset)gives back exactly thatnode's own source text. That is what the JSDoc on
Source#endsays inlib/node.d.ts:69(via#1879) —
— what
lib/node.js:295repeats at therangeBy()call site ("source.end.offsetis exclusive, sowe don't need to add 1"), what
test/location.test.ts'scheckOffset()asserts for every nodetype, and what @idoros and @romainmenke reaffirmed in #2030:
A second invariant follows from it:
end.line/end.columnare inclusive (they point at the lastcharacter), so they must describe offset
end.offset - 1.A rule that owns a stray semicolon (
raws.ownSemicolon) breaks both whenever whitespace sitsbetween
}and;.Reproduction (published postcss 8.5.26)
Three things are wrong at once:
end.column: 1, end.line: 2is the;at offset 5, so the exclusiveend.offsetshould be6 — but it is 7. The two halves of the same
Positiondescribe different characters.\nthat belongs to the next rule'sraws.before— the two nodes' rangesnow touch/overlap instead of being disjoint (
b.source.start.offsetis8, and with more spacesthe range runs into the sibling:
'a{} ;b{color:red}'gives the first ruleend.offset: 10, i.e.'a{} ;b{c').Node#rangeBy()andNode#positionBy()forwardsource.end.offsetverbatim(
sourceOffset()prefers it when it is a number), sonode.error()/node.warn()ranges and anyconsumer that slices by offset inherit the bad value.
Root cause
lib/parser.js#L357-L358,added in #2012:
token[2]is the offset of the semicolon, butownSemicolonisthis.spacesplus thesemicolon — it also contains every space that came before it. So the length that is added starts
counting from the wrong base and overshoots by
ownSemicolon.length - 1. It happens to be correctonly for the exact
a{};shape, which is the one case #2012 was tested against.Every other non-
Rootend position in the parser uses the same base and adds exactly one —lib/parser.jslines 73, 88, 113, 183, 215 and 334, i.e.atrule()(three sites),comment(),decl()andend(). Line 358 is the only one that does not, and one is the correct increment theretoo: the tokenizer emits
;as a single-character token, sotoken[2] + 1is the byte after it.(
Root, set inendFile()at line 347, is the documented exception and is untouched.)The fix
end.line/end.columnwere already correct and are untouched, so nothing that consumesline/column (Stylelint diagnostics, source maps) changes — only the offset stops disagreeing
with them.
end.offsetbeforeinput.lengtha{};a{} ;a{} ;a{}\n;a{}\r\n;a{}\t\t\t;Out-of-bounds
end.offsetover that set: 5 before, 0 after (a{};was already correct — it isthe one shape where
ownSemicolon.lengthhappens to equal 1).How I found it
Property test rather than a hand-written case. The stringifier already reports which output bytes
belong to which node through the
builder(str, node)callback, andparse → stringifyisbyte-lossless, so those spans are the source spans. Diffing them against
node.sourceoverrandom CSS surfaces every place where the parser's own bookkeeping disagrees with itself. This was
the only class where the range ran past the node — the range covered bytes the node does not
own.
Tests
Three cases in
test/location.test.ts, next to the existingruletest, using the file'scheckOffset()helper:.a{};(the shape #2012 already handled, as a guard),.a{} ;, and.a{}\n;\n\n.b{}(which also asserts the following rule'sstartis no longer inside the previousrule's range).
On
origin/main'slib/parser.jswith these tests applied: 13 passed, 2 failed(
.a{} ;→"offset": 9vs expected7;.a{}\n;\n\n.b{}→ slice.a{}\n;\nvs.a{}\n;).With the patch: 15 passed, 0 failed.
Full suite with the one-line
postcss-parser-testsfixture change below applied:pnpm test→689 passed / 0 failed. Against the currently published
postcss-parser-tests@8.9.0it is688 / 689, the single failure being
parses semicolons.css(see "One companion change needed"below) — so CI on this PR is red until that fixture ships, and that failure is the fixture, not
the patch.
test:types✔,test:size16.37 kB / 16.5 kB,test:integration✔ (all real-world sites),coverage gate ✔ (with the fixture applied; without it
test:coverageexits 1 on the same singletest).
test:lintreports the same single pre-existing warning as untouchedmain(
test/visitor.test.ts:340 perfectionist/sort-objects), byte-identical with and without this change.Regression surface
Differential run of
origin/main'slib/against the patchedlib/over 74 real-worldstylesheets (Bootstrap 5, Bulma, Foundation, Tailwind, normalize.css, plus the
postcss-parser-testscorpus), 224,898 nodes:source.end.offsetand nevertouches
rawsor the stringifier, so the lossless round-trip is untouched by construction;semicolons.cssa{b:c} ;,rule 54,63 → 54,62— exactly the fixture in the companion PR below.Same differential over 60,009 generated stylesheets (30k grammar-generated with stray semicolons in
every whitespace shape, 30k hostile atom soup: unbalanced braces, unterminated comments and strings,
lone backslashes, BOMs,
\f,\r-only):CssSyntaxErrordiffersparse → toStringround-trip brokenstart.offsetmovedend.line/end.columnmovedend.offsetmovedrulenodes withraws.ownSemicolon(0 others)end.offset > input.css.lengthslice(start, end) !== node.toString()on those rulesOnly
Rulenodes withraws.ownSemicolonare affected, and only when whitespace precedes thesemicolon;
a{};is byte-for-byte unchanged. Parse throughput is unchanged (baseline5.16/5.20/5.25/5.28/5.19 ms vs patched 5.24/5.43/5.49/5.27/4.91 ms over Bootstrap 5, 30 iterations
per sample — within run-to-run noise). No regex, loop, or allocation is involved; the change
strictly lowers an offset that could previously exceed
input.css.length.(For BOM inputs the comparison is against
input.css, which is the BOM-stripped source the offsetsare relative to;
'a{} ;'was out of bounds before —end.offset8 on a 6-characterinput.css— and is 6 after.)One companion change needed
postcss-parser-tests'cases/semicolons.jsonrecords the pre-fix value for thea{b:c} ;casethat was added alongside #2012, so
parses semicolons.cssfails until it is updated. The fixtureis self-contradictory today —
"column": 8, "line": 8is the;at offset 61, while"offset": 63is the@of the next line:"source": { "end": { "column": 8, "line": 8, - "offset": 63 + "offset": 62 },Sent as postcss/postcss-parser-tests#32. This PR's CI stays red until that one lands and is
released — the only failing test is
parses semicolons.css, reading that fixture. Happy to rebasehere and bump the devDependency once it ships. That is the same sequence #2012 itself used
(postcss/postcss-parser-tests#28 first), and be364fd ("Fix end position in empty Custom
Properties") carried the resulting 8.5.0 → 8.5.1 bump in the same commit as the parser fix.
Worth noting that this is what #2012 set out to do in the first place — its stated goal was that
"slicing out the source range for a rule from the input CSS better matches
rule.toString()". Thatholds today only for
a{};; this patch makes it hold for everyownSemicolonshape.What I did not verify
end.line/end.columndo not move and only the offset changes, but I did not run their suites.end.offsetis too small — acustom property whose trailing whitespace stays in its value (
:root{ --a: b }) and a childlessat-rule keeping trailing
raws.betweenbefore}. Those have a different root cause(whitespace tokens carry no positions, so
findLastWithPosition()skips them) and, unlike thisone, fixing them would move
end.line/end.columnon very common CSS. I left them out of thisPR deliberately and can open a separate issue if that is useful.
pnpm run old(the-r modulepath theoldCI jobs use) on Node 26 — 689/689 with the fixture, 688/689 without — but not on the oldinterpreters themselves. The change introduces no new syntax (
x++on an existing number).postcss-parser-testsfixtures(postcss-scss, postcss-less, …) against the updated
semicolons.json. They would only beaffected if they reproduce the same
ownSemicolonarithmetic; I did not read their sources.