Record node end positions (EndLine/EndColumn) - #391
reuvenharrison wants to merge 2 commits into
Conversation
Node.Line and Column say where a node starts and nothing says where it stops, so a consumer wanting a node's extent has to re-derive it from the positions of everything around it. These two fields make the span explicit. A collection ends at its last child rather than at its END token, which after a block dedent already sits on the following line and would overshoot into the next node. A flow collection closes with an explicit delimiter, so its END mark is right, and an empty collection has no child and uses it too. Block scalars need care. The scalar token's EndMark advances past the trailing line break, so using it directly puts a `description: |` block's end on the following node's line. Rather than change that mark, which cmd/go-yaml pins in its position dump, the content end is carried alongside it as a separate field and read only when setting EndLine. The change is additive: no existing mark moves.
|
I have reservations about this changing Token Event and Node. That's a big ask. Can we not accomplish this with helper methods? What is the driving use case(s) for this? |
|
Fair questions — in order: The driving use case: oasdiff reports breaking changes in OpenAPI specs with exact source spans: a finding points at the lines of the node it concerns, and the review UI extracts and highlights exactly that YAML text. Start positions alone cannot bound a node, so today we maintain a fork of go-yaml whose only reason to exist is these two fields; this PR is the attempt to retire it. The same need appears in any linter, language server, or error reporter that wants "the text of this node": Line/Column say where it begins, and nothing says where it stops. On the size of the ask: only Node is public API here. The yaml package aliases libyaml.Node, so the commitment is exactly two exported fields on Node. Token and Event are internal/libyaml types — those changes are the plumbing that carries the mark from scanner to composer, invisible to importers (the only consumer outside internal is this repo's own cmd/go-yaml, whose pinned output the PR deliberately does not change). Nothing existing moves: no current mark changes meaning, and no existing test changes. On helper methods: if the concern is committing to a representation, I'm happy to switch to unexported fields with an accessor (say Happy to reshape to whichever form you prefer — the substance I need is only that the parser records where a node ends. |
Opened against
mainper @ccoVeille's request on #387; that PR is the same change on the v3 layout and can be closed in favour of this.Node.LineandColumnsay where a node starts, and nothing says where it stops. A consumer wanting a node's extent has to re-derive it from the positions of everything around it, which is possible but is the parser's own knowledge being reconstructed downstream. These two fields make the span explicit:The two decisions worth reviewing
A collection ends at its last child, not at its END token. After a block dedent the END token already sits on the following line, so using it would overshoot into the next node. Flow collections close with an explicit
}/], so their END mark is right and is used; an empty collection has no child and uses it too.Block scalars are carried separately rather than by changing an existing mark. A block scalar token's
EndMarkadvances past the trailing line break, so using it directly puts adescription: |block's end on the following node's line. On the v3 branch I changed that mark, having traced that nothing read it. That is not true here:cmd/go-yamlpins the token span in its position dump (1:6-4:1), and the change broke it. So the content end is carried alongside as a separate field on the token and event, read only when settingEndLine.This version is strictly additive. No existing mark moves, and no existing test changes.
Validation
The port was checked against the v3 implementation differentially rather than only against its own tests: both node trees walked in lockstep comparing kind, value,
Line,Column,EndLine,EndColumnand child count.The v3 branch itself was validated the same way against the fork it originated from, over ~8,900 files including the GitHub (9.3 MB) and Stripe specs, also with zero differences.
The tests cover literal, folded, keep and strip block scalars, flow and block collections, empty collections, anchors and merge keys, multi-document streams, comments, and a file with no trailing newline. I checked they bite rather than merely pass: an off-by-one in the collection end produces three distinct failures, and the suite is green with it reverted.