Record node end positions (EndLine/EndColumn)

[reuvenharrison]

Problem

Node reports where an element starts (Line/Column) but not where it ends. Given a node for an entire mapping there is no way to know which line the mapping stops on, so a consumer cannot extract a whole collection from its source text.

That is the gap this fills: given a node, take the exact source span it occupies.

Change

EndLine/EndColumn alongside Line/Column, populated from the parser event's end_mark — at construction for scalars and aliases, and for mappings and sequences from the last child's end so the span reaches the end of the actual content.

Both are 1-based like Line/Column, and both are zero when the source carried no position (a textless tree built in memory).

Two cases that need care

Block collections. The MAPPING-END/SEQUENCE-END token sits at the start of the line following the dedent, so using it overshoots the element — a mapping would appear to extend into whatever comes next. Taking the last child's end keeps the span tight. Empty collections have no last child and fall back to the token's mark.

Flow collections. These close with an explicit } or ], and there the END token's mark is just past the delimiter, which is exactly right. So flow uses the token and block uses the last child.

Both behaviours have tests.

Tests

end_position_test.go covers scalars, block and flow collections, nesting, block scalars, empty collections and aliases.

TestNodeRoundtrip clears the new fields before comparing. Its expected node literals predate EndLine/EndColumn and do not set them; end positions are covered by the new test rather than by duplicating positions across every literal there. Happy to change that if you would rather see them spelled out.

Notes

Additive: two new fields and no change to existing behaviour, so nothing that ignores them is affected.

This has been running in a downstream fork for some months, used to slice an OpenAPI operation or schema out of its source file for display. Offering it here because it is a general parser capability rather than anything specific to that use, and carrying it in a fork is the wrong place for it.

Draft while I get review on my side — happy to take feedback before then, and to split the test-file change out if that is easier to review.

[reuvenharrison]

Rebased onto current v3 — the branch was cut before the move off gopkg.in/check.v1, so it conflicted.

Two things changed in the rebase, both in tests:

• end_position_test.go is rewritten in standard testing style. The position assertions collapse into three small helpers (checkKind, checkStart, checkEnd) rather than an if-block per field, since almost every assertion here is "this node spans exactly here" — 116 individual checks would have made the file about twice as long without reading any better. Happy to expand them if you would rather see plain ifs throughout.
• TestNodeRoundtrip picks up the standard-testing signature from v3 and keeps the clearEndPos call, moved above the reflect.DeepEqual so it runs before the comparison rather than after.

I checked the converted assertions still bite rather than merely passing: introducing an off-by-one in the block-collection end produces 16 distinct failures, and the suite is green with it reverted.

[reuvenharrison]

Validated the port against the fork it came from, since "the tests pass" only shows the new tests agree with themselves.

I walked both node trees in lockstep — kind, value, Line, Column, EndLine, EndColumn, child count — over every YAML file I could find:

corpus	files	differences
kin-openapi's test corpus	8,347 (70 unparseable, skipped by both)	0
a downstream differ's test corpus	619	0
real specs: GitHub API (9.3 MB), Stripe, two generated	4	0
hand-written edge cases	8	0

The edge cases target the parts most likely to diverge: literal/folded/keep/strip block scalars, empty and nested flow collections, a file with no trailing newline, anchors and merge keys, multi-document streams, trailing and standalone comments, quoting and implicit types, and sequences of mappings.

Also worth noting the corpora are not neutral: this branch is the source of a downstream implementation that has been slicing OpenAPI operations out of source files for months, and that consumer's own tests pass against the positions being compared here. So the agreement is between this PR and code with real usage behind it, not between two fresh implementations.

Not claiming this proves correctness — a systematic error present in both would be invisible to a differential — but it rules out the port having dropped or altered anything in transit, which was the risk I could actually test.

[reuvenharrison]

One thing in here is not purely additive, and it deserves calling out rather than leaving for review to find.

Besides the two new fields, the change in scannerc.go modifies an existing value: a block scalar token's end_mark now stops just past the last content character instead of advancing past the trailing line break.

That is necessary — otherwise a description: | block reports an end on the following node's line, which defeats the purpose of the feature — but it means asking where else that mark can be observed. Tracing it:

• a scalar token's end_mark flows only into the scalar event's end_mark (parserc.go, the token.typ == yaml_SCALAR_TOKEN branch)
• the only reader of a scalar event's end_mark is the new EndLine/EndColumn code in decode.go
• collection events take end_mark from their own END token, not from a child's, so it does not propagate upward
• diagnostics use start_mark and problem_mark, not a scalar's end_mark

Empirically the existing suite agrees: it passes unchanged, including the encode and roundtrip tests, which would notice if emitted output shifted.

If you would rather not change the token at all, the alternative is to leave end_mark alone and carry the content-end position as a separate field on the token, used only when setting EndLine. Slightly more plumbing, strictly additive. Happy to switch if that is the preference — I went with modifying it because nothing reads it and a parallel field seemed like carrying two nearly-identical marks around.

[ccoVeille]

Hi @reuvenharrison

Thank you for working on this. It's a great addition. That said, I'm likely to reply the same thing as I did here:

• Add UnmarshalerWithContext on the v3 line #389 (comment)

TL;DR; the v3 is considered stale and will recieve no runtime code changes except security fixes.

Could you consider opening this in the main branch ?

[reuvenharrison]

Reopened against main as #391, and happy for this one to be closed.

Worth flagging that #391 is not a straight port, because main caught something this branch did not. Here I changed the block scalar token's EndMark directly, having traced that nothing read it, and said in a comment above that the alternative was to carry the content end as a separate field. On main that alternative turns out to be necessary: cmd/go-yaml pins the token span in its position dump, and changing the mark breaks it. So #391 adds a separate field and leaves every existing mark alone. It is strictly additive, which this branch is not.

I also checked the port against this branch rather than only against its own tests, walking both node trees in lockstep over kind, value, Line, Column, EndLine, EndColumn and child count: 4,240 files and 619 files from two corpora, zero differences.

[ccoVeille]

Please close this one.

We will review the other one.

Be patient, everyone is busy with real life in summer