Skip to content

Fix rule end offset when spaces precede its own semicolon - #2135

Merged
ai merged 1 commit into
postcss:mainfrom
maximilliangrand:fix/own-semicolon-end-offset
Aug 14, 2026
Merged

Fix rule end offset when spaces precede its own semicolon#2135
ai merged 1 commit into
postcss:mainfrom
maximilliangrand:fix/own-semicolon-end-offset

Conversation

@maximilliangrand

Copy link
Copy Markdown
Contributor

The invariant

source.end.offset of a non-Root node is the exclusive end of the node, so that
input.css.slice(node.source.start.offset, node.source.end.offset) gives back exactly that
node's own source text. That is what the JSDoc on Source#end says in lib/node.d.ts:69 (via
#1879) —

However, end.offset of a non Root node is the exclusive position.

— what lib/node.js:295 repeats at the rangeBy() call site ("source.end.offset is exclusive, so
we don't need to add 1"), what test/location.test.ts's checkOffset() asserts for every node
type, and what @idoros and @romainmenke reaffirmed in #2030:

The start and end offset values look correct to me. They allow slicing the source string to
extract the exact AST node and align with other parsers (css-tree, CSSOM, TypeScript, Acorn, etc.)

A second invariant follows from it: end.line/end.column are inclusive (they point at the last
character), so they must describe offset end.offset - 1.

A rule that owns a stray semicolon (raws.ownSemicolon) breaks both whenever whitespace sits
between } and ;.

Reproduction (published postcss 8.5.26)

let postcss = require('postcss') // 8.5.26

let css = 'a {}\n;\n\nb { color: red }'
let a = postcss.parse(css).first

a.toString()                                        //=> 'a {}\n;'
a.source.end                                        //=> { column: 1, line: 2, offset: 7 }
css.slice(a.source.start.offset, a.source.end.offset) //=> 'a {}\n;\n'   <- one char too many

Three things are wrong at once:

  • end.column: 1, end.line: 2 is the ; at offset 5, so the exclusive end.offset should be
    6 — but it is 7. The two halves of the same Position describe different characters.
  • the slice swallows the \n that belongs to the next rule's raws.before — the two nodes' ranges
    now touch/overlap instead of being disjoint (b.source.start.offset is 8, and with more spaces
    the range runs into the sibling: 'a{} ;b{color:red}' gives the first rule
    end.offset: 10, i.e. 'a{} ;b{c').
  • the offset can leave the file entirely:
let css = 'a {}\n;'          // length 6
postcss.parse(css).first.source.end.offset //=> 7

Node#rangeBy() and Node#positionBy() forward source.end.offset verbatim
(sourceOffset() prefers it when it is a number), so node.error() / node.warn() ranges and any
consumer that slices by offset inherit the bad value.

Root cause

lib/parser.js#L357-L358,
added in #2012:

prev.source.end = this.getPosition(token[2])
prev.source.end.offset += prev.raws.ownSemicolon.length

token[2] is the offset of the semicolon, but ownSemicolon is this.spaces plus the
semicolon — 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 correct
only for the exact a{}; shape, which is the one case #2012 was tested against.

Every other non-Root end position in the parser uses the same base and adds exactly one —
lib/parser.js lines 73, 88, 113, 183, 215 and 334, i.e. atrule() (three sites), comment(),
decl() and end(). Line 358 is the only one that does not, and one is the correct increment there
too: the tokenizer emits ; as a single-character token, so token[2] + 1 is the byte after it.
(Root, set in endFile() at line 347, is the documented exception and is untouched.)

The fix

+        // `ownSemicolon` also holds the spaces before the semicolon, but
+        // the position above is the semicolon itself, so the node ends
+        // right after it.
         prev.source.end = this.getPosition(token[2])
-        prev.source.end.offset += prev.raws.ownSemicolon.length
+        prev.source.end.offset++

end.line/end.column were already correct and are untouched, so nothing that consumes
line/column (Stylelint diagnostics, source maps) changes — only the offset stops disagreeing
with them.

input end.offset before after input.length
a{}; 4 4 4
a{} ; 6 5 5
a{} ; 8 6 6
a{}\n; 6 5 5
a{}\r\n; 8 6 6
a{}\t\t\t; 10 7 7

Out-of-bounds end.offset over that set: 5 before, 0 after (a{}; was already correct — it is
the one shape where ownSemicolon.length happens 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, and parse → stringify is
byte-lossless, so those spans are the source spans. Diffing them against node.source over
random 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 existing rule test, using the file's
checkOffset() helper: .a{}; (the shape #2012 already handled, as a guard), .a{} ;, and
.a{}\n;\n\n.b{} (which also asserts the following rule's start is no longer inside the previous
rule's range).

On origin/main's lib/parser.js with these tests applied: 13 passed, 2 failed
(.a{} ;"offset": 9 vs expected 7; .a{}\n;\n\n.b{} → slice .a{}\n;\n vs .a{}\n;).
With the patch: 15 passed, 0 failed.

Full suite with the one-line postcss-parser-tests fixture change below applied: pnpm test
689 passed / 0 failed. Against the currently published postcss-parser-tests@8.9.0 it is
688 / 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:size 16.37 kB / 16.5 kB, test:integration ✔ (all real-world sites),
coverage gate ✔ (with the fixture applied; without it test:coverage exits 1 on the same single
test). test:lint reports the same single pre-existing warning as untouched main
(test/visitor.test.ts:340 perfectionist/sort-objects), byte-identical with and without this change.

Regression surface

Differential run of origin/main's lib/ against the patched lib/ over 74 real-world
stylesheets (Bootstrap 5, Bulma, Foundation, Tailwind, normalize.css, plus the
postcss-parser-tests corpus), 224,898 nodes:

  • stringified output differs on 0 files — this change writes only source.end.offset and never
    touches raws or the stringifier, so the lossless round-trip is untouched by construction;
  • 1 node's offsets changed in the whole corpus: semicolons.css a{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):

baseline patched
stringified output differs 0
thrown CssSyntaxError differs 0
parse → toString round-trip broken 0 0
start.offset moved 0
end.line / end.column moved 0
end.offset moved 41,304, all on rule nodes with raws.ownSemicolon (0 others)
end.offset > input.css.length 5,692 0
slice(start, end) !== node.toString() on those rules 39,693 0

Only Rule nodes with raws.ownSemicolon are affected, and only when whitespace precedes the
semicolon; a{}; is byte-for-byte unchanged. Parse throughput is unchanged (baseline
5.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 offsets
are relative to; 'a{} ;' was out of bounds before — end.offset 8 on a 6-character
input.css — and is 6 after.)

One companion change needed

postcss-parser-tests' cases/semicolons.json records the pre-fix value for the a{b:c} ; case
that was added alongside #2012, so parses semicolons.css fails until it is updated. The fixture
is self-contradictory today — "column": 8, "line": 8 is the ; at offset 61, while
"offset": 63 is 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 rebase
here 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()". That
holds today only for a{};; this patch makes it hold for every ownSemicolon shape.

What I did not verify

  • No downstream check against Stylelint / Stylelint VSCode. I believe the risk is nil here because
    end.line/end.column do not move and only the offset changes, but I did not run their suites.
  • The same property test also flags two unrelated cases where end.offset is too small — a
    custom property whose trailing whitespace stays in its value (:root{ --a: b }) and a childless
    at-rule keeping trailing raws.between before }. Those have a different root cause
    (whitespace tokens carry no positions, so findLastWithPosition() skips them) and, unlike this
    one, fixing them would move end.line/end.column on very common CSS. I left them out of this
    PR deliberately and can open a separate issue if that is useful.
  • No real Node 10/12/14 runtime available here. I ran pnpm run old (the -r module path the
    old CI jobs use) on Node 26 — 689/689 with the fixture, 688/689 without — but not on the old
    interpreters themselves. The change introduces no new syntax (x++ on an existing number).
  • I did not run any third-party parser that consumes the postcss-parser-tests fixtures
    (postcss-scss, postcss-less, …) against the updated semicolons.json. They would only be
    affected if they reproduce the same ownSemicolon arithmetic; I did not read their sources.

Signed-off-by: maximilliangrand <214999687+maximilliangrand@users.noreply.github.com>
@maximilliangrand

Copy link
Copy Markdown
Contributor Author

The one failing check (parses semicolons.css) is the expected cross-repo lag, not a defect in this PR.

That fixture lives in postcss-parser-tests, and its recorded end.offset for the own-semicolon rule encodes the very off-by-one this PR fixes. The companion correction is postcss/postcss-parser-tests#32 (merged). CI here still installs postcss-parser-tests from npm, which hasn't been re-released with that change yet, so the fixture it compares against is the old offset: 63.

With the two in sync the numbers line up exactly:

parse('a {}\n;').first.source.end.offset   // 8.5.26: 7 (past EOF)  ->  this PR: 6
semicolons.css nodes[3] end.offset          // npm fixture: 63       ->  merged #32 / this PR: 62

So once a postcss-parser-tests release carries #32 (or the dev-dependency is bumped to the merged commit), this check goes green with no further change here. Everything else in CI passes. Happy to bump the devDependency in this PR if you'd prefer it self-contained.

@ai

ai commented Aug 14, 2026

Copy link
Copy Markdown
Member

Of course, I will release postcss-parser-test update today (I need also update dev tools there, so it will take a while)

@ai
ai merged commit 55a4edf into postcss:main Aug 14, 2026
0 of 10 checks passed
@ai

ai commented Aug 14, 2026

Copy link
Copy Markdown
Member

Thanks!

I will release it a little later together with #2136

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