From 10e954d6d202526fd45afcee68c7dff7404ae6fe Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Fri, 31 Jul 2026 10:34:13 +0200 Subject: [PATCH 1/7] fix(yaml-lexer): give block container delimiters real source positions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A consumer laying YL's tokens out by position — colouring YAML in a TUI — had to sort each line's spans by column and discard tokens reporting line 0. Both workarounds were needed for YL and neither for the JSON lexer. Cause: block collections have no "{" / "[" / "}" / "]" characters, so goccy has no token to point a container delimiter at. Confirmed against the AST: block: MappingNode IsFlowStyle=false Start=":"@L1C5 End= SequenceNode IsFlowStyle=false Start="-"@L2C3 End= flow: SequenceNode IsFlowStyle=true Start="["@L1C7 End="]"@L1C12 Start is the first entry's SEPARATOR and End is nothing. Used literally, that put the opening delimiter after the key it precedes, info: title: Petstore L2 C8 delimiter /info <- the colon of `title`, a line below L2 C3 key /info/title <- column goes 8 -> 3 so the stream contradicted its own emission order, and left every closing delimiter at line 0 column 0 — not a position at all, since lines are 1-based. This affected every block container at every depth, including the ROOT mapping, which is why it showed up even in documents whose inner collections are flow. The delimiters now take the SPAN of what they enclose: the opener reports the first token inside, the closer the last. The span is read back off the emitted stream rather than computed from the AST, so it composes with everything the walk does to the children — merge-key resolution, alias expansion, tags, nested containers — none of which the node's own tokens know about. Flow collections keep their real delimiter positions untouched. Two consequences worth stating: - Offset moves with Line/Column (same source position), so closing delimiters now carry a real offset instead of 0. - Block delimiters share their neighbour's position rather than sitting strictly before/after it: there is no character of their own to point at. Consumers must order by non-decreasing position, not strictly increasing. Also fixes an implicit null reporting no position at all: an empty, comment-only or header-only document, and a "key:" with nothing after it. It now inherits the preceding token's position, or the start of input when it IS the whole stream. Found by running the invariants over the vendored test suite, not by hand — six documents the worked examples missed. Positions had NO test coverage before this, which is how it shipped. Now pinned: exact spans for block and flow, no zero positions, non-decreasing order over 199 suite documents, and Offset inside the input. The conformance baseline is unchanged (204/85/63) — that comparison covers token kind and value, not position, so it could never have caught this. Aliases still report the ANCHOR DEFINITION site and so can still go backwards. That is by design (see walkAlias) and now has a test saying so, so it stays a decision rather than becoming an accident. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- json/lexers/yaml-lexer/position_test.go | 263 ++++++++++++++++++++++++ json/lexers/yaml-lexer/walk.go | 77 ++++++- 2 files changed, 337 insertions(+), 3 deletions(-) create mode 100644 json/lexers/yaml-lexer/position_test.go diff --git a/json/lexers/yaml-lexer/position_test.go b/json/lexers/yaml-lexer/position_test.go new file mode 100644 index 0000000..78d93ae --- /dev/null +++ b/json/lexers/yaml-lexer/position_test.go @@ -0,0 +1,263 @@ +package lexer_test + +import ( + "fmt" + "strings" + "testing" + + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" + + "github.com/go-openapi/core/json/lexers/token" + yamllexer "github.com/go-openapi/core/json/lexers/yaml-lexer" +) + +// Source positions (Line/Column/Offset). +// +// Block collections have no "{" / "[" / "}" / "]" characters, so goccy has no token to point a +// container delimiter at: it hands us the first entry's separator (the ":" of the first pair, the +// "-" of the first item) as the node's Start, and nothing at all as its End. Used literally that +// put the opening delimiter AFTER the key it precedes and the closing one at line 0 column 0 — +// so a consumer laying tokens out by position had to sort them back and discard the closers. +// +// walk.go:patchBlockSpan gives them the span of what they enclose instead. These tests pin that, +// and pin that FLOW collections keep their real delimiter positions untouched. + +// posTok is one token with the position YL reported for it. +type posTok struct { + kind token.Kind + val string + line int + col int + off uint64 +} + +func (p posTok) String() string { + return fmt.Sprintf("L%dC%d %v(%q)", p.line, p.col, p.kind, p.val) +} + +func lexPositions(t *testing.T, src string, opts ...yamllexer.Option) []posTok { + t.Helper() + + l := yamllexer.NewWithBytes([]byte(src), opts...) + var out []posTok + for tok := range l.Tokens() { + if tok.Kind() == token.EOF { + break + } + out = append(out, posTok{ + kind: tok.Kind(), val: string(tok.Value()), + line: l.Line(), col: l.Column(), off: l.Offset(), + }) + } + require.NoError(t, l.Err(), "input must lex: %q", src) + + return out +} + +// TestBlockContainerSpan pins where a block collection's delimiters land: the opening one on the +// first token it encloses, the closing one on the last. +func TestBlockContainerSpan(t *testing.T) { + t.Run("nested block mappings", func(t *testing.T) { + got := lexPositions(t, "info:\n title: Petstore\n version: 1\n") + + assert.Equal(t, []string{ + `L1C1 delimiter("")`, // root mapping opens on `info`, not on its colon + `L1C1 key("info")`, + `L2C3 delimiter("")`, // /info opens on `title`, not on the colon a line below + `L2C3 key("title")`, + `L2C10 string("Petstore")`, + `L3C3 key("version")`, + `L3C12 number("1")`, + `L3C12 delimiter("")`, // /info closes on its last token + `L3C12 delimiter("")`, // root closes there too + }, render(got)) + }) + + t.Run("block sequence", func(t *testing.T) { + got := lexPositions(t, "schemes:\n - http\n - https\n") + + assert.Equal(t, []string{ + `L1C1 delimiter("")`, + `L1C1 key("schemes")`, + `L2C5 delimiter("")`, // opens on `http`, not on the "-" before it + `L2C5 string("http")`, + `L3C5 string("https")`, + `L3C5 delimiter("")`, + `L3C5 delimiter("")`, + }, render(got)) + }) +} + +// TestFlowContainerPositionsUnchanged pins the other side of the fix: a flow collection HAS real +// delimiter characters, and those positions must survive untouched. +func TestFlowContainerPositionsUnchanged(t *testing.T) { + got := lexPositions(t, "tags: [a, b]\n") + + assert.Equal(t, []string{ + `L1C1 delimiter("")`, + `L1C1 key("tags")`, + `L1C7 delimiter("")`, // the real "[" + `L1C8 string("a")`, + `L1C11 string("b")`, + `L1C12 delimiter("")`, // the real "]" + `L1C12 delimiter("")`, + }, render(got)) + + t.Run("flow mapping", func(t *testing.T) { + got := lexPositions(t, "m: {a: 1}\n") + assert.Equal(t, `L1C4 delimiter("")`, render(got)[2], "the real \"{\"") + assert.Equal(t, `L1C9 delimiter("")`, render(got)[5], "the real \"}\"") + }) +} + +// TestNoTokenHasAZeroPosition pins that every emitted token carries a usable position. Line and +// column are 1-based, so 0 is not a position: a consumer that sees one can only discard the token. +func TestNoTokenHasAZeroPosition(t *testing.T) { + for _, src := range []string{ + "info:\n title: Petstore\n", + "schemes:\n - http\n - https\n", + "tags: [a, b]\n", + "m: {a: 1}\n", + "a:\n b:\n c:\n - 1\n - x: 2\n", + "- 1\n- 2\n", + "root: null\n", + "empty: {}\n", + "emptyseq: []\n", + } { + t.Run(src, func(t *testing.T) { + for _, p := range lexPositions(t, src) { + assert.Positivef(t, p.line, "%s: line must be 1-based", p) + assert.Positivef(t, p.col, "%s: column must be 1-based", p) + } + }) + } +} + +// TestPositionsAreNonDecreasing is the property the TUI actually needs: laying tokens out by +// position must not require sorting them first. +// +// Non-decreasing rather than strictly increasing: in block style a container delimiter has no +// character of its own, so it shares its neighbour's position. +func TestPositionsAreNonDecreasing(t *testing.T) { + for _, src := range []string{ + "info:\n title: Petstore\n version: 1\n", + "schemes:\n - http\n - https\n", + "tags: [a, b]\n", + "openapi: 3.0.0\ninfo:\n title: t\npaths:\n /p:\n get:\n responses:\n '200':\n description: ok\n", + "- a\n- b: 1\n c: 2\n- - nested\n - seq\n", + } { + t.Run(src, func(t *testing.T) { + assertNonDecreasing(t, lexPositions(t, src)) + }) + } +} + +// TestAliasPositionsStillPointAtTheAnchor documents the one case that stays out of order, so the +// behaviour is a recorded decision rather than an accident. +// +// Alias-expanded tokens deliberately report the ANCHOR DEFINITION site (see walkAlias), which is +// earlier in the document than the alias. A consumer highlighting an alias-bearing document +// therefore still cannot assume monotonicity. +func TestAliasPositionsStillPointAtTheAnchor(t *testing.T) { + got := lexPositions(t, "defs: &d\n p: 1\nuse: *d\n") + + var backwards bool + prev := posTok{} + for _, p := range got { + if p.line < prev.line || (p.line == prev.line && p.col < prev.col) { + backwards = true + } + prev = p + } + + assert.True(t, backwards, + "expansion still reports the anchor site: if this ever stops being true, the walkAlias "+ + "doc and the TUI's ordering assumptions both need revisiting") +} + +// TestOffsetTracksPosition pins that Offset moved with Line/Column — it comes from the same +// source position, so a container delimiter used to report offset 0 as well. +func TestOffsetTracksPosition(t *testing.T) { + src := "info:\n title: Petstore\n" + got := lexPositions(t, src) + + for _, p := range got { + assert.LessOrEqualf(t, p.off, uint64(len(src)), "%s: offset must be inside the input", p) + } + // the closing delimiters sit on the last token, not at offset 0 + last := got[len(got)-1] + assert.Positivef(t, last.off, "%s: a closing delimiter must carry a real offset", last) +} + +func render(toks []posTok) []string { + out := make([]string, 0, len(toks)) + for _, p := range toks { + out = append(out, p.String()) + } + + return out +} + +func assertNonDecreasing(t *testing.T, toks []posTok) { + t.Helper() + + prev := posTok{line: 1, col: 1} + for i, p := range toks { + ok := p.line > prev.line || (p.line == prev.line && p.col >= prev.col) + assert.Truef(t, ok, "token %d goes backwards: %s after %s\nstream: %s", + i, p, prev, strings.Join(render(toks), " ")) + prev = p + } +} + +// TestPositionsOverTheWholeSuite runs the position invariants over every document the vendored +// YAML Test Suite says is valid — 400-odd real-world shapes rather than the handful above. +// +// Alias-bearing documents are skipped for the ordering check only: expansion reports the anchor +// site by design (see TestAliasPositionsStillPointAtTheAnchor). Everything else must carry a +// 1-based position and never go backwards. +func TestPositionsOverTheWholeSuite(t *testing.T) { + var checked, skipped int + + for _, c := range loadSuite(t) { + if c.fail { + continue + } + + l := yamllexer.NewWithBytes([]byte(c.yaml), + yamllexer.WithMaxContainerStack(512), yamllexer.WithMaxTokens(1<<20)) + + var toks []posTok + for tok := range l.Tokens() { + if tok.Kind() == token.EOF { + break + } + toks = append(toks, posTok{ + kind: tok.Kind(), val: string(tok.Value()), + line: l.Line(), col: l.Column(), off: l.Offset(), + }) + } + if l.Err() != nil || len(toks) == 0 { + continue // rejected or empty: nothing to say about its positions + } + + t.Run(c.label(), func(t *testing.T) { + for _, p := range toks { + require.Positivef(t, p.line, "%s: line must be 1-based\nsrc=%q", p, c.yaml) + require.Positivef(t, p.col, "%s: column must be 1-based\nsrc=%q", p, c.yaml) + } + + // an alias re-walks the anchored node, so its tokens report the definition site + if strings.ContainsAny(c.yaml, "*&") { + skipped++ + + return + } + checked++ + assertNonDecreasing(t, toks) + }) + } + + t.Logf("ordering checked on %d documents, %d skipped for aliases", checked, skipped) +} diff --git a/json/lexers/yaml-lexer/walk.go b/json/lexers/yaml-lexer/walk.go index 6f433d0..e4e1b8d 100644 --- a/json/lexers/yaml-lexer/walk.go +++ b/json/lexers/yaml-lexer/walk.go @@ -100,11 +100,13 @@ func (l *YL) walkValue(node ast.Node, lvl int) { case *ast.MappingNode: l.walkMapping(n, lvl) case *ast.MappingValueNode: - // a single-entry mapping that goccy did not wrap in a MappingNode + // a single-entry mapping that goccy did not wrap in a MappingNode; only block style + // produces one, and its token is the pair's ":" (see patchBlockSpan) l.walkMappingEntries( []*ast.MappingValueNode{n}, posOf(n.GetToken()), posOf(n.GetToken()), + false, lvl, ) case *ast.SequenceNode: @@ -129,7 +131,7 @@ func (l *YL) walkValue(node ast.Node, lvl int) { // walkMapping emits an object: { key : value , … }. func (l *YL) walkMapping(n *ast.MappingNode, lvl int) { - l.walkMappingEntries(n.Values, posOf(n.Start), posOf(n.End), lvl) + l.walkMappingEntries(n.Values, posOf(n.Start), posOf(n.End), n.IsFlowStyle, lvl) } // walkMappingEntries emits the object delimiters around a list of key/value entries. @@ -137,6 +139,7 @@ func (l *YL) walkMapping(n *ast.MappingNode, lvl int) { func (l *YL) walkMappingEntries( values []*ast.MappingValueNode, start, end *yamltoken.Position, + flow bool, lvl int, ) { inner := lvl + 1 @@ -144,6 +147,7 @@ func (l *YL) walkMappingEntries( return } + openIdx := len(l.toks) l.emitDelim(token.OpeningBracket, start, inner) if hasMergeKey(values) { // D5: resolve "<<" merge keys with RFC precedence (explicit keys win, earlier merges @@ -167,6 +171,10 @@ func (l *YL) walkMappingEntries( // The closing delimiter reports the ENCLOSING level (the level it returns to), matching // the JSON lexer L, which pops the container before emitting the closer. l.emitDelim(token.ClosingBracket, end, lvl) + + if !flow { + l.patchBlockSpan(openIdx) + } } // entryKV is a resolved mapping member (after merge-key expansion). @@ -361,6 +369,7 @@ func (l *YL) walkSequence(n *ast.SequenceNode, lvl int) { return } + openIdx := len(l.toks) l.emitDelim(token.OpeningSquareBracket, posOf(n.Start), inner) for _, v := range n.Values { if l.err != nil { @@ -370,6 +379,10 @@ func (l *YL) walkSequence(n *ast.SequenceNode, lvl int) { } // The closing delimiter reports the ENCLOSING level, matching L (see walkMappingEntries). l.emitDelim(token.ClosingSquareBracket, posOf(n.End), lvl) + + if !n.IsFlowStyle { + l.patchBlockSpan(openIdx) + } } // walkAlias resolves an alias to its anchored value and emits its tokens inline, guarding @@ -512,14 +525,72 @@ func (l *YL) put(tok token.T, pos *yamltoken.Position, lvl int) { } e := emit{tok: tok, lvl: lvl} - if pos != nil { + switch { + case pos != nil: e.off = uint64(pos.Offset) //nolint:gosec // no overflow, no negative values e.line = pos.Line e.col = pos.Column + + case len(l.toks) > 0: + // No source position: an implicit value the document does not spell out (a "key:" with + // nothing after it). It belongs where the construct that implies it left off, so it + // inherits the preceding token's position rather than reporting none. + prev := l.toks[len(l.toks)-1] + e.off, e.line, e.col = prev.off, prev.line, prev.col + + default: + // Nothing precedes it either: an empty, comment-only or header-only document, whose + // single implicit null value is the whole token stream. The start of the input is the + // only position it can honestly claim. + e.off, e.line, e.col = 0, 1, 1 } l.toks = append(l.toks, e) } +// patchBlockSpan gives a BLOCK collection's delimiters real positions, openIdx being the index +// at which its opening delimiter was appended. +// +// Block style has no "{" / "[" / "}" / "]" characters, so goccy has no token to point the +// delimiters at. It sets the node's Start to the first entry's SEPARATOR — the ":" of the first +// pair, the "-" of the first item — and its End to nil. Taken literally that is worse than +// imprecise: +// +// - the opening delimiter lands AFTER the key it precedes ("info:" reports the ":" of the +// nested "title:" a line below), so the token stream contradicts its own order and a +// consumer laying tokens out by position has to sort them back; +// - the closing delimiter gets no position at all and surfaces as line 0, column 0, which is +// not a position — lines are 1-based — so a consumer can only discard it. +// +// Instead the delimiters take the SPAN of what they enclose: the opening one reports the first +// token inside the container, the closing one the last. Both are real, in-range positions, and +// an alias-free document then reads monotonically. +// +// They are EQUAL to their neighbour's position rather than strictly before/after it, because in +// block style there is no character of their own to point at: a consumer must order by +// non-decreasing position, not strictly increasing. (An alias-BEARING document can still go +// backwards, by design — expanded tokens report the anchor definition site, see walkAlias.) +// +// The span is read back off the emitted stream rather than computed from the AST so that it +// composes with everything the walk may have done to the children — merge-key resolution, alias +// expansion, tags, nested containers — none of which the node's own tokens know about. +// +// It is a no-op for a flow collection (the caller checks), for an empty one (nothing to span, +// so goccy's own tokens stand), and when a circuit breaker cut the walk short and the indices +// no longer line up. +func (l *YL) patchBlockSpan(openIdx int) { + closeIdx := len(l.toks) - 1 + if l.err != nil || openIdx < 0 || closeIdx <= openIdx || closeIdx >= len(l.toks) { + return + } + if closeIdx == openIdx+1 { + return // empty container: no children to take a span from + } + + first, last := l.toks[openIdx+1], l.toks[closeIdx-1] + l.toks[openIdx].off, l.toks[openIdx].line, l.toks[openIdx].col = first.off, first.line, first.col + l.toks[closeIdx].off, l.toks[closeIdx].line, l.toks[closeIdx].col = last.off, last.line, last.col +} + // overContainerStack reports whether opening a container at depth would exceed the // WithMaxContainerStack circuit breaker, setting the error if so. func (l *YL) overContainerStack(depth int) bool { From c4161f6f4bf5d92339daec7008de82c856144254 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Fri, 31 Jul 2026 11:23:39 +0200 Subject: [PATCH 2/7] docs(yaml-lexer): correct group 5 of the conformance inventory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prose said "two DK95 tab cases"; the third case is actually VJP3/1, and DK95/7 is a multi-document case (which conformanceXFail already had right — only the write-up was wrong). Also names what 4MUZ/2 and VJP3/1 have in common, since it turns out to be one defect rather than two: inside a FLOW mapping a line break between the key and its ":" is legal YAML 1.2, but goccy applies the block-context rule that they share a line. Verified directly against parser.ParseBytes. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- json/lexers/yaml-lexer/CONFORMANCE.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/json/lexers/yaml-lexer/CONFORMANCE.md b/json/lexers/yaml-lexer/CONFORMANCE.md index 5dd1ce0..3f1de8d 100644 --- a/json/lexers/yaml-lexer/CONFORMANCE.md +++ b/json/lexers/yaml-lexer/CONFORMANCE.md @@ -145,8 +145,12 @@ document is accepted and the values are simply not the ones the suite expects. ### 5. Valid documents the parser rejects — 3 cases -`goccy` fails to parse these at all (`4MUZ/2` flow mapping with the colon on the next -line; two `DK95` tab cases). The fix is upstream, or a pre-pass of our own. +`goccy` fails to parse these at all, so the fix is upstream (or a pre-pass of our own): + +- `4MUZ/2` (`{foo\n: bar}`) and `VJP3/1` (a flow mapping spread over five lines) are the + same defect twice — inside a **flow** mapping a line break between the key and its `:` + is legal, but the parser applies the block-context rule that they share a line; +- `DK95/4` (`foo: 1\n\t\nbar: 2`) is a line holding only a tab between two entries. ## Divergences by suite tag From dd6658d6ce7a1ad2c823823350783177554a9744 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Fri, 31 Jul 2026 12:13:00 +0200 Subject: [PATCH 3/7] docs(yaml-lexer): record why separators stay elided, and that multi-document is a boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two corrections to the record, both prompted by re-proposing things that were already settled. Separators: DESIGN.md already said YL never emits ":" / "," and that this is not an option. What it did not say is WHY, or that the tokens are available — so the question keeps coming back as if it were blocked on goccy. It is not: goccy exposes the ":" of every pair (MappingValueNode.Start) and the "-" of every block item (SequenceEntryNode.Start). The reason is that a separator mode is error-prone for no gain — consumers index by the value tokens, YAML's separators do not map onto JSON's, and a block "-" would need either a new delimiter kind in the shared token package (a JSON-side change for a YAML-only need) or a Comma token whose String() reports the wrong character. Recorded so the next reader does not rediscover the availability and mistake it for an argument. Multi-document: the 14 cases were counted among "54 divergences", which reads as 54 things needing attention. YL's model is structurally single-document, so those are an acknowledged boundary; they stay in the xfail table only so their verdict cannot drift unnoticed. The number worth acting on is 40. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- json/lexers/yaml-lexer/CONFORMANCE.md | 14 +++++++++----- json/lexers/yaml-lexer/DESIGN.md | 11 +++++++++++ 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/json/lexers/yaml-lexer/CONFORMANCE.md b/json/lexers/yaml-lexer/CONFORMANCE.md index 3f1de8d..dfa404c 100644 --- a/json/lexers/yaml-lexer/CONFORMANCE.md +++ b/json/lexers/yaml-lexer/CONFORMANCE.md @@ -8,6 +8,7 @@ accept + token stream matches the expected JSON 204 / 272 (75%) invalid document rejected 85 / 94 (90%) recorded only (no single-root JSON equivalent) 63 known divergences (conformanceXFail) 54 + of which out of scope by design 14 ``` ## What is being measured @@ -19,7 +20,8 @@ streams are identical. That framing matters for reading the numbers: a case `YL` fails is not necessarily a YAML bug. It may be a construct JSON cannot express, in which case being unable to lex -it is correct behavior. The 54 divergences below are split accordingly. +it is correct behavior. The 54 xfail entries below are split accordingly — 14 of them are the design boundary +rather than work. Three buckets, mirroring the JSON suite's `y_`/`n_`/`i_` split: @@ -100,11 +102,13 @@ largest single group. Genuinely out of scope within this group: explicit complex keys (`? [a, b] : c`), which JSON cannot express at all. -### 2. Multiple documents — 14 cases +### 2. Multiple documents — 14 cases (out of scope by design) A JSON token stream has one root, so a `%YAML`/`---` multi-document stream is rejected. -Out of scope until the ND-JSON work on the roadmap lands, at which point these become -an NDJSON mode rather than an error. +`YL`'s model is structurally single-document; this is an acknowledged boundary, not a +defect, and these 14 are listed in the xfail table only so the suite stays green and their +verdict cannot drift unnoticed. Counting divergences we might actually act on, the number +is **40, not 54**. (Note these are cases where the *expectation* is still single-root — a directive plus one document. Streams whose expectation is genuinely several JSON values fall into the @@ -173,7 +177,7 @@ In rough order of value per unit of effort: 2. **Over-permissiveness** (group 3) — 9 cases, and the only group where we accept documents we should reject. Worth confirming against goccy first. 3. **Scalar resolution** (group 4) — 5 cases, each needing individual reading. -4. **Multi-document** (group 2) — 14 cases, blocked on the ND-JSON decision. +4. ~~**Multi-document** (group 2)~~ — not on the list: out of scope by design (see above). 5. **Upstream parse failures** (group 5) — 3 cases, not ours to fix directly. ## Maintaining this diff --git a/json/lexers/yaml-lexer/DESIGN.md b/json/lexers/yaml-lexer/DESIGN.md index cedd206..2f1a290 100644 --- a/json/lexers/yaml-lexer/DESIGN.md +++ b/json/lexers/yaml-lexer/DESIGN.md @@ -66,6 +66,17 @@ a non-issue. them. Object = `{ key value key value }`, array = `[ value value ]`. (Unlike `L`, this is not an option — a verbatim/separator mode would only matter to a round-tripping consumer, which `YL` explicitly is not.) + + This is a decision, not a limitation, and it has been revisited: goccy exposes every + separator we would need — `MappingValueNode.Start` is the `:` of each pair, and + `SequenceEntryNode.Start` the `-` of each block item — so emitting them would be easy. + It is declined because a separator mode is error-prone for no gain: consumers index by + the *value* tokens (JSON pointer, position, colour by kind), and YAML's separators do not + map onto JSON's anyway. A block `-` has no JSON counterpart, so it would need either a + new delimiter kind in the shared `token` package — a JSON-side change for a YAML-only + need — or a `Comma` token whose `String()` reports the wrong character. Neither is worth + it. (For the record, goccy also does not expose the `,` between the pairs of a *flow* + mapping at all, so even a separator mode could not be complete.) - **`IndentLevel` matches `L`'s convention.** Openers and interior tokens sit at the container's level; a **closing** `}` / `]` reports the *enclosing* level (the level it returns to), because `L` pops the container before emitting the closer. `YL` mirrors that From d2772d59a4ff7fb8476992cab5bea5cfe68d6e31 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Fri, 31 Jul 2026 13:05:48 +0200 Subject: [PATCH 4/7] feat(yaml-lexer): resolve mapping keys that denote a scalar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A JSON object key is a string, so YL only accepts a key denoting a scalar. But YAML lets a key carry node properties and indirection that JSON has no place for and that do not stop it being a string: ? explicit key : v MappingKeyNode !!str key : v TagNode &anchor key : v AnchorNode *alias : v AliasNode and combinations of those ("? !!str &a foo"). All of them were rejected as "complex keys", and they are the single largest group of conformance divergences: 23 of the YAML Test Suite's documents turn on it, and the suite's own JSON equivalent shows the resolved string as the key in every one. resolveKey now peels each wrapper until a scalar is reached. An anchor written on a KEY is registered as it is peeled, so a later "*a" resolves to it, and alias resolution shares the existing cycle guard so a key aliasing its own definition terminates instead of recursing. resolveMapping's merge-key precedence goes through the same resolver, so a wrapped key inside a merge-bearing mapping dedupes correctly too. A key that really denotes a sequence or mapping stays rejected. Worth recording: written out directly ("? [a, b] : v") goccy refuses the document before we see it, so ErrComplexKey is now reachable only through indirection — an alias naming a collection. The tests pin both paths. accept + match 204/272 -> 226/272 comparable slice 79% -> 85% which is above every implementation in the middle band of matrix.yaml.info's JSON comparison (PyYAML 75%, psych 76%, go-yaml 76%, ruamel 78%). 22 of the 23 flipped. The 23rd, RR7F, turned out not to be ours at all: it is a defect in the FIXTURE. Its event stream and canonical dump both order the keys a, d; only its json field writes d first. The suite compares loaded data as unordered maps, so nothing there ever checked its JSON text against its own tree. We keep key order — a JSON token stream is ordered, and so is our model — so we match the event stream. Recorded as such rather than worked around, and worth reporting to yaml-test-suite. The recorded-behaviour golden caught two out-of-scope documents flipping reject -> accept as a side effect (6M2F, DFF7 — both valid YAML with explicit/alias keys and no JSON equivalent). Both are improvements; blessed deliberately rather than silently. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- json/lexers/yaml-lexer/CONFORMANCE.md | 104 +++++++------ .../yaml-lexer/conformance_xfail_test.go | 54 ++----- json/lexers/yaml-lexer/resolved_key_test.go | 141 ++++++++++++++++++ .../testdata/conformance_recorded.golden | 4 +- json/lexers/yaml-lexer/walk.go | 130 +++++++++++++--- 5 files changed, 325 insertions(+), 108 deletions(-) create mode 100644 json/lexers/yaml-lexer/resolved_key_test.go diff --git a/json/lexers/yaml-lexer/CONFORMANCE.md b/json/lexers/yaml-lexer/CONFORMANCE.md index dfa404c..5dd7771 100644 --- a/json/lexers/yaml-lexer/CONFORMANCE.md +++ b/json/lexers/yaml-lexer/CONFORMANCE.md @@ -4,13 +4,17 @@ Baseline measured against the vendored [YAML Test Suite](testdata/yaml-test-suit (revision `da267a5c`, 351 files → **406 cases**), by `TestConformanceYAML`. ``` -accept + token stream matches the expected JSON 204 / 272 (75%) +accept + token stream matches the expected JSON 226 / 272 (83%) invalid document rejected 85 / 94 (90%) recorded only (no single-root JSON equivalent) 63 -known divergences (conformanceXFail) 54 +known divergences (conformanceXFail) 32 of which out of scope by design 14 ``` +Resolved-scalar mapping keys landed (22 cases): a key carrying an explicit-key marker, +a tag, an anchor or an alias now resolves to the string it denotes. See §"Resolved keys" +below for what remains. + ## What is being measured `YL` reads YAML **as JSON**, so the suite's `json` field — the JSON a document is @@ -20,7 +24,7 @@ streams are identical. That framing matters for reading the numbers: a case `YL` fails is not necessarily a YAML bug. It may be a construct JSON cannot express, in which case being unable to lex -it is correct behavior. The 54 xfail entries below are split accordingly — 14 of them are the design boundary +it is correct behavior. The 32 xfail entries below are split accordingly — 14 of them are the design boundary rather than work. Three buckets, mirroring the JSON suite's `y_`/`n_`/`i_` split: @@ -44,8 +48,8 @@ over the same suite — "is the loaded data equal to the `json` field" — which question we ask. Its denominator is 401 (308 valid + 93 invalid). Our comparable slice is 366 cases (272 with a single-root JSON expectation + 94 invalid), -scoring **289 → 79%**. Counting the 40 cases with no JSON equivalent as misses instead — -a stricter reading — gives **289/406 → 71%**. So we sit somewhere in **71–79%** depending +scoring **311 → 85%**. Counting the 40 cases with no JSON equivalent as misses instead — +a stricter reading — gives **311/406 → 77%**. So we sit somewhere in **77–85%** depending on how the out-of-scope cases are counted. | implementation | JSON comparison | @@ -54,9 +58,9 @@ on how the out-of-scope cases are counted. | JS yaml | 92% | | Perl YAML::PP | 92% | | Haskell HsYAML | 91% | +| **`YL` (this lexer)** | **77–85%** | | Python ruamel.yaml | 78% | | Perl YAML::PP+libyaml | 78% | -| **`YL` (this lexer)** | **71–79%** | | JS js-yaml | 76% | | Ruby psych | 76% | | Go go-yaml | 76% | @@ -77,32 +81,35 @@ Read that carefully rather than as a ranking — three things make it not apples over-permissive accepts and the 3 parse failures — are goccy's behavior, not ours, and we cannot beat it without pre-validating ourselves. -The useful takeaway: **we are in the same band as the mainstream loaders** (PyYAML, psych, -go-yaml, ruamel, js-yaml) and clearly behind the four leaders. And the single biggest gap is -ours to close: fixing resolved-scalar keys (group 1 below, 23 cases) would put the -comparable slice at roughly **85%**, above every implementation in the middle band. +The useful takeaway: **we are now above the mainstream loaders** (PyYAML 75%, psych 76%, +go-yaml 76%, ruamel 78%) and below the four leaders. Resolved-scalar keys were the single +biggest gap and are now closed, which is what moved 79% → 85%. -## The 54 divergences +## The 32 remaining entries -### 1. Non-scalar mapping keys — 23 cases +### Resolved keys — done (22 cases) -`YL` requires an object key to be a scalar, because JSON keys are strings. Most of -these, however, are keys that *resolve* to a string: +A JSON object key is a string, so `YL` only accepts a key denoting a scalar. YAML lets a key +carry node properties and indirection that JSON has no place for but that do not stop it being +a string: ```yaml -top3: &node3 - *alias1 : scalar3 # the suite expects {"scalar1": "scalar3"} +? explicit key : v # MappingKeyNode +!!str key : v # TagNode +&anchor key : v # AnchorNode +*alias : v # AliasNode -- resolves through the anchor table ``` -An alias, an anchored scalar (`&a key : v`) or a tagged one (`!!str key : v`) is -rejected today, though the resolved key is an ordinary string and the suite's own JSON -shows it as such. **This is a feature gap, not a scope boundary**, and it is the -largest single group. +and combinations (`? !!str &a foo`). These were rejected as "complex". `walk.go:resolveKey` +now peels each wrapper until a scalar is reached, which is exactly what the suite's own JSON +equivalent shows for these documents. An anchor written on a *key* is registered too, so a +later `*a` resolves to it. -Genuinely out of scope within this group: explicit complex keys (`? [a, b] : c`), -which JSON cannot express at all. +What stays rejected: a key that really denotes a sequence or mapping. Note that written out +directly (`? [a, b] : v`) goccy refuses the document before we see it — our own `ErrComplexKey` +is reached only through indirection, an alias naming a collection. -### 2. Multiple documents — 14 cases (out of scope by design) +### 1. Multiple documents — 14 cases (out of scope by design) A JSON token stream has one root, so a `%YAML`/`---` multi-document stream is rejected. `YL`'s model is structurally single-document; this is an acknowledged boundary, not a @@ -114,7 +121,7 @@ is **40, not 54**. one document. Streams whose expectation is genuinely several JSON values fall into the recorded bucket instead.) -### 3. Invalid documents we accept — 9 cases +### 2. Invalid documents we accept — 9 cases The most valuable group: we are **more permissive than YAML allows**. @@ -134,20 +141,28 @@ All are inherited from the underlying `goccy/go-yaml` parser rather than introdu our walk — flow collections, comment placement and tabs. Each needs confirming against goccy upstream before deciding whether to report it there or pre-validate ourselves. -### 4. Accepted, but the token stream differs — 5 cases +### 3. Accepted, but the token stream differs — 6 cases ``` -565N Construct Binary (!!binary tag) +565N Construct Binary (!!binary tag) L24T/1 Trailing line of spaces -LE5A Spec Example 7.24. Flow Nodes -S4JQ Spec Example 6.28. Non-Specific Tags -UGM3 Spec Example 2.27. Invoice +LE5A Spec Example 7.24. Flow Nodes +S4JQ Spec Example 6.28. Non-Specific Tags +UGM3 Spec Example 2.27. Invoice +RR7F Mixed Block Mapping (see below -- not ours) ``` -Scalar-resolution divergences. The subtlest group, because nothing errors — the -document is accepted and the values are simply not the ones the suite expects. +The first five are scalar-resolution divergences, and the subtlest group: nothing errors, +the document is accepted and the values are simply not the ones the suite expects. + +`RR7F` is a **defect in the fixture**. Its YAML is `a: 4.2 / ? d / : 23`; its own event +stream (`+MAP =VAL :a =VAL :4.2 =VAL :d =VAL :23`) and its canonical `dump` both order the +keys `a, d`, but its `json` field writes `d` first. The suite compares loaded data as +unordered maps, so nothing there ever checked its JSON text against its own tree. We keep +key order — a JSON token stream is ordered, and so is our model — so we match the event +stream and diverge from the JSON text. Worth reporting upstream to yaml-test-suite. -### 5. Valid documents the parser rejects — 3 cases +### 4. Valid documents the parser rejects — 3 cases `goccy` fails to parse these at all, so the fix is upstream (or a pre-pass of our own): @@ -161,24 +176,25 @@ document is accepted and the values are simply not the ones the suite expects. Which YAML *features* are involved (a case carries several tags, so these overlap): ``` -spec 22 mapping 21 tag 16 explicit-key 12 flow 10 sequence 10 -directive 8 alias 7 comment 7 unknown-tag 7 whitespace 7 error 6 -header 6 indent 6 anchor 5 1.3-err 4 double 4 … +spec 14 tag 11 directive 8 flow 7 error 6 header 6 sequence 6 +indent 5 unknown-tag 5 whitespace 5 double 4 alias 3 comment 3 … ``` +`mapping`, `explicit-key` and `anchor` have dropped out of the top of this list entirely — +that is the resolved-keys work showing up. + ## Reading this as a work list In rough order of value per unit of effort: -1. **Resolved-scalar keys** (group 1) — 23 cases, one coherent feature: resolve an - alias/anchor/tag on the key side and use the resulting scalar. Takes the comparable - slice from 79% to **85%**, above every implementation in the matrix's middle band. - Adding groups 3 and 4 on top would reach **89%**, within reach of the leaders. -2. **Over-permissiveness** (group 3) — 9 cases, and the only group where we accept - documents we should reject. Worth confirming against goccy first. -3. **Scalar resolution** (group 4) — 5 cases, each needing individual reading. -4. ~~**Multi-document** (group 2)~~ — not on the list: out of scope by design (see above). -5. **Upstream parse failures** (group 5) — 3 cases, not ours to fix directly. +1. ~~**Resolved-scalar keys**~~ — **done**, 22 cases, 79% → 85%. +2. **Over-permissiveness** (group 2) — 9 cases, and the only group where we accept + documents we should reject. All goccy's behaviour; confirm upstream before working + around them (see `PROPOSALS-go-openapi.md` in the goccy checkout). +3. **Scalar resolution** (group 3) — 5 cases, each needing individual reading. Closing + these and group 2 would reach roughly **89%**, within reach of the leaders. +4. ~~**Multi-document** (group 1)~~ — not on the list: out of scope by design (see above). +5. **Upstream parse failures** (group 4) — 3 cases, not ours to fix directly. ## Maintaining this diff --git a/json/lexers/yaml-lexer/conformance_xfail_test.go b/json/lexers/yaml-lexer/conformance_xfail_test.go index 36a2029..f85c99e 100644 --- a/json/lexers/yaml-lexer/conformance_xfail_test.go +++ b/json/lexers/yaml-lexer/conformance_xfail_test.go @@ -7,45 +7,15 @@ package lexer_test // against NEW regressions. An entry that starts passing is reported as an error ("remove from xfail"), so the list // cannot rot into an excuse. // -// Baseline: 204/272 documents with a JSON equivalent are accepted AND lex to that JSON; 85/94 invalid documents are -// rejected. The 54 entries below group into five causes, in descending order of how much work they represent. +// Baseline: 226/272 documents with a JSON equivalent are accepted AND lex to that JSON; 85/94 invalid documents are +// rejected. The 32 entries below group into four causes; 14 of them (multiple documents) are a design boundary +// rather than work. // // See CONFORMANCE.md for the full analysis. func conformanceXFail() map[string]string { return map[string]string{ // --------------------------------------------------------------------------------------------------- - // 1. Non-scalar mapping keys (23). YL requires an object key to be a scalar, since JSON keys are - // strings. But most of these are keys that RESOLVE to a string -- an alias (`*a : v`), an anchored or - // tagged scalar (`&a k : v`, `!!str k : v`) -- and the suite's own JSON expectation shows the resolved - // string as the key. Supporting them is a real feature gap, not a scope boundary. The genuinely - // out-of-scope ones are complex keys (`? [a, b] : c`), which JSON cannot express at all. - // --------------------------------------------------------------------------------------------------- - "26DV": "non-scalar mapping key", - "2SXE": "non-scalar mapping key", - "2XXW": "non-scalar mapping key", - "5WE3": "non-scalar mapping key", - "74H7": "non-scalar mapping key", - "7BMT": "non-scalar mapping key", - "7FWL": "non-scalar mapping key", - "7W2P": "non-scalar mapping key", - "A2M4": "non-scalar mapping key", - "CN3R": "non-scalar mapping key", - "CT4Q": "non-scalar mapping key", - "E76Z": "non-scalar mapping key", - "GH63": "non-scalar mapping key", - "HMQ5": "non-scalar mapping key", - "JTV5": "non-scalar mapping key", - "L94M": "non-scalar mapping key", - "RR7F": "non-scalar mapping key", - "S9E8": "non-scalar mapping key", - "U3XV": "non-scalar mapping key", - "WZ62": "non-scalar mapping key", - "X8DW": "non-scalar mapping key", - "ZH7C": "non-scalar mapping key", - "ZWK4": "non-scalar mapping key", - - // --------------------------------------------------------------------------------------------------- - // 2. Multiple documents (14). A JSON token stream has one root, so YL rejects a multi-document stream. + // 1. Multiple documents (14). A JSON token stream has one root, so YL rejects a multi-document stream. // The suite's json field for these holds several JSON values in sequence. Out of scope until the // ND-JSON work lands (see the lexers README roadmap), at which point they become an NDJSON mode. // --------------------------------------------------------------------------------------------------- @@ -65,7 +35,7 @@ func conformanceXFail() map[string]string { "ZYU8": "multiple documents", // --------------------------------------------------------------------------------------------------- - // 3. Invalid documents we ACCEPT (9). Real conformance bugs, and the most valuable group: we are more + // 2. Invalid documents we ACCEPT (9). Real conformance bugs, and the most valuable group: we are more // permissive than YAML allows, mostly around flow collections, comments and tabs. Inherited from the // underlying goccy parser rather than introduced by our walk, so each needs to be confirmed against // goccy upstream before we decide whether to pre-validate ourselves. @@ -81,10 +51,16 @@ func conformanceXFail() map[string]string { "YJV2": "we accept an invalid document", // --------------------------------------------------------------------------------------------------- - // 4. Accepted, but the token stream differs from the expected JSON (5). Semantic divergences in how a - // scalar is resolved -- tags (!!binary, !!str), trailing whitespace, flow nodes. Each needs reading - // individually; they are the subtlest of the five groups because nothing errors. + // 3. Accepted, but the token stream differs from the expected JSON (6). Semantic divergences in how a + // scalar is resolved -- tags (!!binary, !!str), trailing whitespace, flow nodes. + // + // RR7F is not one of them: it is a defect in the FIXTURE. Its yaml is "a: 4.2 / ? d / : 23", and its own + // event stream and canonical dump both order the keys a, d -- but its json field writes d first. The + // suite compares loaded data as unordered maps, so nothing there ever checked its json text against its + // own tree. We keep key order (a JSON token stream is ordered, and so is our model), so we match the + // event stream and diverge from the json text. Reported upstream rather than worked around. // --------------------------------------------------------------------------------------------------- + "RR7F": "fixture defect: its json field contradicts its own event stream on key order", "565N": "token stream differs from the expected JSON", "L24T/1": "token stream differs from the expected JSON", "LE5A": "token stream differs from the expected JSON", @@ -92,7 +68,7 @@ func conformanceXFail() map[string]string { "UGM3": "token stream differs from the expected JSON", // --------------------------------------------------------------------------------------------------- - // 5. Valid documents rejected by the underlying parser (3). goccy fails to parse these at all, so the + // 4. Valid documents rejected by the underlying parser (3). goccy fails to parse these at all, so the // fix is upstream (or a pre-pass of our own). // --------------------------------------------------------------------------------------------------- "4MUZ/2": "goccy fails to parse a valid document", diff --git a/json/lexers/yaml-lexer/resolved_key_test.go b/json/lexers/yaml-lexer/resolved_key_test.go new file mode 100644 index 0000000..17bcf8c --- /dev/null +++ b/json/lexers/yaml-lexer/resolved_key_test.go @@ -0,0 +1,141 @@ +package lexer_test + +import ( + "testing" + + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" + + "github.com/go-openapi/core/json/lexers/token" + yamllexer "github.com/go-openapi/core/json/lexers/yaml-lexer" +) + +// Mapping keys that RESOLVE to a scalar. +// +// A JSON object key is a string, so YL only accepts a key that denotes a scalar. YAML lets a key +// carry node properties and indirection that JSON has no place for but that do not stop it being +// a string — an explicit key ("? k"), a tag ("!!str k"), an anchor ("&a k"), an alias ("*a"), and +// combinations. Those were rejected as "complex" until walk.go:resolveKey peeled them; 22 of the +// YAML Test Suite's documents turn on it. +// +// A key that really is a sequence or a mapping stays rejected: that one JSON cannot express. + +func keysOf(t *testing.T, src string) []string { + t.Helper() + + l := yamllexer.NewWithBytes([]byte(src)) + var keys []string + for tok := range l.Tokens() { + if tok.Kind() == token.Key { + keys = append(keys, string(tok.Value())) + } + } + require.NoErrorf(t, l.Err(), "must lex: %q", src) + + return keys +} + +func TestResolvedScalarKeys(t *testing.T) { + cases := []struct { + name string + src string + want []string + }{ + {name: "plain", src: "k: v\n", want: []string{"k"}}, + {name: "explicit key", src: "? k\n: v\n", want: []string{"k"}}, + {name: "explicit key, flow value", src: "? k\n: [1, 2]\n", want: []string{"k"}}, + {name: "tagged key", src: "!!str k: v\n", want: []string{"k"}}, + {name: "tagged integer key", src: "!!int 3: v\n", want: []string{"3"}}, + {name: "anchored key", src: "&a k: v\n", want: []string{"k"}}, + {name: "alias key", src: "d: &a name\nm:\n *a : v\n", want: []string{"d", "m", "name"}}, + {name: "explicit + tag", src: "? !!str k\n: v\n", want: []string{"k"}}, + {name: "explicit + anchor", src: "? &a k\n: v\n", want: []string{"k"}}, + {name: "tag + anchor", src: "!!str &a k: v\n", want: []string{"k"}}, + { + name: "anchor defined on a key is reusable", + src: "&a k: v\nm:\n *a : w\n", + want: []string{"k", "m", "k"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, keysOf(t, tc.src)) + }) + } +} + +// TestComplexKeysStillRejected pins the boundary: a key that denotes a collection is outside the +// JSON data model however many wrappers are peeled off it. +// +// Which error you get depends on how the key is written, and that is worth recording. Written +// out directly, goccy refuses the document before we see it ("found an invalid key for this +// map"), so ErrComplexKey is unreachable for those. It is reached through INDIRECTION — an alias +// naming a collection — where goccy is happy and only our JSON-shaped model objects. +func TestComplexKeysStillRejected(t *testing.T) { + t.Run("rejected upstream, before our walk", func(t *testing.T) { + for _, tc := range []struct { + name string + src string + }{ + {name: "sequence key", src: "? [a, b]\n: v\n"}, + {name: "mapping key", src: "? {a: 1}\n: v\n"}, + {name: "anchored sequence key", src: "? &a [x]\n: v\n"}, + } { + t.Run(tc.name, func(t *testing.T) { + l := yamllexer.NewWithBytes([]byte(tc.src)) + for range l.Tokens() { //nolint:revive // draining is the point + } + require.Error(t, l.Err(), "a collection key must be rejected") + assert.NotErrorIs(t, l.Err(), yamllexer.ErrComplexKey, + "goccy rejects these at parse time, so our own guard never runs") + }) + } + }) + + t.Run("rejected by our model", func(t *testing.T) { + // goccy parses this happily: the key is an alias, and only resolving it reveals a sequence + l := yamllexer.NewWithBytes([]byte("d: &a [x]\nm:\n *a : v\n")) + for range l.Tokens() { //nolint:revive // draining is the point + } + require.Error(t, l.Err()) + assert.ErrorIs(t, l.Err(), yamllexer.ErrComplexKey) + }) +} + +// TestResolvedKeyPosition pins where a resolved key reports itself: on the scalar it denotes, so +// the position always holds the text the token carries. +func TestResolvedKeyPosition(t *testing.T) { + // "&a k" -- the key token says "k", so it must point at k (column 4), not at the anchor + // one mapping: open, key, value, close + got := lexPositions(t, "&a k: v\n") + require.Len(t, got, 4) + assert.Equal(t, `L1C4 key("k")`, got[1].String(), + "the token says \"k\", so it must point at k -- not at the anchor that precedes it") + + t.Run("an alias key points at the anchor definition", func(t *testing.T) { + // consistent with alias-expanded VALUES (see walkAlias): the position holds the text + got := lexPositions(t, "d: &a name\nm:\n *a : v\n") + var key posTok + for _, p := range got { + if p.kind == token.Key && p.val == "name" { + key = p + } + } + assert.Equal( + t, + 1, + key.line, + "reports the anchor site (line 1), not the alias site (line 3)", + ) + }) +} + +// TestResolvedKeyCycleIsRejected pins that a key aliasing its own definition terminates rather +// than recursing: the guard is shared with the alias-cycle machinery. +func TestResolvedKeyCycleIsRejected(t *testing.T) { + l := yamllexer.NewWithBytes([]byte("&a *a : v\n")) + for range l.Tokens() { //nolint:revive // draining is the point + } + assert.Error(t, l.Err(), "a self-referential key must not recurse") +} diff --git a/json/lexers/yaml-lexer/testdata/conformance_recorded.golden b/json/lexers/yaml-lexer/testdata/conformance_recorded.golden index 09fa185..36244b7 100644 --- a/json/lexers/yaml-lexer/testdata/conformance_recorded.golden +++ b/json/lexers/yaml-lexer/testdata/conformance_recorded.golden @@ -6,7 +6,7 @@ 4MUZ/1 accept Flow mapping colon on line after key 5TYM reject Spec Example 6.21. Local Tag Prefix 6BFJ reject Mapping, key and flow sequence item anchors -6M2F reject Aliases in Explicit Block Mapping +6M2F accept Aliases in Explicit Block Mapping 6PBE reject Zero-indented sequences in explicit mapping keys 6WLZ reject Spec Example 6.18. Primary Tag Handle [1.3] 6XDY accept Two document start markers @@ -21,7 +21,7 @@ 9WXW reject Spec Example 6.18. Primary Tag Handle AVM7 accept Empty Stream CFD4 reject Empty implicit key in single pair flow sequences -DFF7 reject Spec Example 7.16. Flow Mapping Entries +DFF7 accept Spec Example 7.16. Flow Mapping Entries FH7J reject Tags on Empty Scalars FRK4 reject Spec Example 7.3. Completely Empty Flow Nodes HWV9 accept Document-end marker diff --git a/json/lexers/yaml-lexer/walk.go b/json/lexers/yaml-lexer/walk.go index e4e1b8d..1920c50 100644 --- a/json/lexers/yaml-lexer/walk.go +++ b/json/lexers/yaml-lexer/walk.go @@ -204,7 +204,7 @@ func (l *YL) resolveMapping(values []*ast.MappingValueNode) []entryKV { if _, isMerge := mv.Key.(*ast.MergeKeyNode); isMerge { continue } - if ks, ok := keyString(mv.Key); ok { + if ks, _, ok := l.scalarKeyResolved(mv.Key); ok { explicit[ks] = true } } @@ -239,7 +239,7 @@ func (l *YL) resolveMapping(values []*ast.MappingValueNode) []entryKV { continue } - ks, ok := keyString(mv.Key) + ks, _, ok := l.scalarKeyResolved(mv.Key) if !ok { l.err = ErrComplexKey @@ -303,20 +303,106 @@ func (l *YL) mergeEntries(src ast.Node) []*ast.MappingValueNode { // keyString returns the string form of a scalar mapping key (matching emitKey's token value), // used for merge de-duplication. ok is false for a non-scalar (complex) key. -func keyString(key ast.MapKeyNode) (string, bool) { +// maxKeyUnwrap bounds the resolveKey recursion. A key can legitimately stack node properties +// ("? !!str &a foo"), but only a handful deep; the bound is what stops an alias chain that +// resolves back into itself from recursing forever. +const maxKeyUnwrap = 16 + +// scalarKey reduces a mapping key to the scalar it denotes, returning its text and the token to +// report a position at. +// +// A JSON object key is a string, so YL only accepts a key that resolves to a scalar. "Resolves" +// is doing real work, because YAML lets a key carry node properties and indirection that the +// JSON data model has no place for but that do not stop it being a string: +// +// ? explicit key : v an explicit key (MappingKeyNode) wrapping any node +// !!str key : v a tagged key (TagNode) +// &anchor key : v an anchored key (AnchorNode) +// *alias : v an alias to a scalar defined elsewhere (AliasNode) +// +// and combinations of those ("? !!str foo"). Each wrapper is peeled until a scalar is reached; +// what comes out is an ordinary string key, which is exactly what the YAML Test Suite's own JSON +// equivalent shows for these documents. +// +// A key that resolves to a sequence or a mapping is genuinely outside the JSON data model and +// still fails with ErrComplexKey. +// +// Aliases are resolved against the anchor table, so this is a method: an alias key can only be +// resolved once the anchor it names has been walked, which the single forward pass guarantees +// (YAML requires an anchor to be defined before it is used). +func (l *YL) resolveKey(key ast.Node, depth int) (ast.Node, bool) { + if depth > maxKeyUnwrap { + return nil, false + } + switch k := key.(type) { + case *ast.MappingKeyNode: // "? key" + return l.resolveKey(k.Value, depth+1) + + case *ast.TagNode: // "!!str key" + return l.resolveKey(k.Value, depth+1) + + case *ast.AnchorNode: // "&a key" -- also registers the anchor, so a later "*a" resolves + if name := anchorName(k.Name); name != "" { + l.anchors[name] = k.Value + } + + return l.resolveKey(k.Value, depth+1) + + case *ast.AliasNode: // "*a" + name := anchorName(k.Value) + target, ok := l.anchors[name] + if !ok { + return nil, false + } + if l.expanding[name] { + return nil, false // a key aliasing its own definition + } + l.expanding[name] = true + node, resolved := l.resolveKey(target, depth+1) + delete(l.expanding, name) + + return node, resolved + + default: + return key, true + } +} + +// scalarKey is [YL.resolveKey] followed by the scalar test: it returns the key's text and the +// token whose position the Key token should report, or ok=false when the key is not a scalar. +// +// The position is the resolved scalar's own token, so the reported location always holds the +// text the token carries. For an alias that is the anchor DEFINITION site, consistent with how +// alias-expanded values are positioned (see walkAlias). +func (l *YL) scalarKeyResolved(key ast.MapKeyNode) (string, *yamltoken.Token, bool) { + node, ok := l.resolveKey(key, 0) + if !ok { + return "", nil, false + } + + return scalarKey(node) +} + +// scalarKey reads a scalar node's text and token. It does no unwrapping: callers that may see a +// wrapped key go through [YL.scalarKeyResolved]. +func scalarKey(node ast.Node) (string, *yamltoken.Token, bool) { + switch k := node.(type) { case *ast.StringNode: - return k.Value, true + return k.Value, k.Token, true case *ast.IntegerNode: - return k.Token.Value, true + return k.Token.Value, k.Token, true case *ast.FloatNode: - return k.Token.Value, true + return k.Token.Value, k.Token, true case *ast.BoolNode: - return k.Token.Value, true + return k.Token.Value, k.Token, true case *ast.NullNode: - return k.Token.Value, true + return k.Token.Value, k.Token, true + case *ast.LiteralNode: + // a block scalar used as an explicit key ("? |" ...): its text is the folded content + return k.Value.Value, k.Start, true default: - return "", false + return "", nil, false } } @@ -416,23 +502,21 @@ func (l *YL) emitKey(key ast.MapKeyNode, lvl int) { return } - switch k := key.(type) { - case *ast.StringNode: - l.putValue(token.MakeWithValue(token.Key, []byte(k.Value)), posOf(k.Token), lvl) - case *ast.IntegerNode: - l.putValue(token.MakeWithValue(token.Key, []byte(k.Token.Value)), posOf(k.Token), lvl) - case *ast.FloatNode: - l.putValue(token.MakeWithValue(token.Key, []byte(k.Token.Value)), posOf(k.Token), lvl) - case *ast.BoolNode: - l.putValue(token.MakeWithValue(token.Key, []byte(k.Token.Value)), posOf(k.Token), lvl) - case *ast.NullNode: - l.putValue(token.MakeWithValue(token.Key, []byte(k.Token.Value)), posOf(k.Token), lvl) - case *ast.MergeKeyNode: - // D5 (deferred): the "<<" merge key is a later increment. + if _, isMerge := key.(*ast.MergeKeyNode); isMerge { + // a "<<" reaching here was not resolved away by resolveMapping l.err = codes.ErrInvalidToken - default: + + return + } + + text, tok, ok := l.scalarKeyResolved(key) + if !ok { l.err = ErrComplexKey + + return } + + l.putValue(token.MakeWithValue(token.Key, []byte(text)), posOf(tok), lvl) } // walkInteger emits an integer, unconverted when its spelling is already a JSON number and From 3338d576684ff5f643b2a21acd5e67fab2b51188 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Fri, 31 Jul 2026 14:48:36 +0200 Subject: [PATCH 5/7] docs(yaml-lexer): reference the upstream issue for the RR7F fixture defect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit yaml/yaml-test-suite#179. That repository appears largely unmaintained, so the xfail entry is recorded as permanent rather than pending — nothing is waiting on it. If the fixture is ever corrected the entry reports itself as an unexpected pass, which is the signal to remove it. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- json/lexers/yaml-lexer/CONFORMANCE.md | 7 ++++++- json/lexers/yaml-lexer/conformance_xfail_test.go | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/json/lexers/yaml-lexer/CONFORMANCE.md b/json/lexers/yaml-lexer/CONFORMANCE.md index 5dd7771..76aa1ac 100644 --- a/json/lexers/yaml-lexer/CONFORMANCE.md +++ b/json/lexers/yaml-lexer/CONFORMANCE.md @@ -160,7 +160,12 @@ stream (`+MAP =VAL :a =VAL :4.2 =VAL :d =VAL :23`) and its canonical `dump` both keys `a, d`, but its `json` field writes `d` first. The suite compares loaded data as unordered maps, so nothing there ever checked its JSON text against its own tree. We keep key order — a JSON token stream is ordered, and so is our model — so we match the event -stream and diverge from the JSON text. Worth reporting upstream to yaml-test-suite. +stream and diverge from the JSON text. + +Reported as [yaml/yaml-test-suite#179](https://github.com/yaml/yaml-test-suite/issues/179). +That repository appears largely unmaintained, so this entry is **permanent, not pending** — +nothing is waiting on it. If the fixture is ever corrected the xfail will report itself as +an unexpected pass, which is the signal to remove it. ### 4. Valid documents the parser rejects — 3 cases diff --git a/json/lexers/yaml-lexer/conformance_xfail_test.go b/json/lexers/yaml-lexer/conformance_xfail_test.go index f85c99e..9a25850 100644 --- a/json/lexers/yaml-lexer/conformance_xfail_test.go +++ b/json/lexers/yaml-lexer/conformance_xfail_test.go @@ -60,7 +60,7 @@ func conformanceXFail() map[string]string { // own tree. We keep key order (a JSON token stream is ordered, and so is our model), so we match the // event stream and diverge from the json text. Reported upstream rather than worked around. // --------------------------------------------------------------------------------------------------- - "RR7F": "fixture defect: its json field contradicts its own event stream on key order", + "RR7F": "fixture defect (yaml-test-suite#179): its json contradicts its own event stream", "565N": "token stream differs from the expected JSON", "L24T/1": "token stream differs from the expected JSON", "LE5A": "token stream differs from the expected JSON", From ce1380122cf331db3ae72b97cb16fceded35adf4 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Fri, 31 Jul 2026 17:30:31 +0200 Subject: [PATCH 6/7] fix(yaml-lexer): strip a leading UTF-8 BOM before parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI fuzz case 509dec0845c6303e: "\t0" lexed as the STRING "0" under YL and the NUMBER 0 under L, tripping the JSON-subset differential. YAML 1.2 allows a document to be prefixed by a byte order mark and it is not content, but goccy does not strip it. The consequence is not a dirty value, it is a different parse — the mark becomes the first character of the first token: "{}" -> the SCALAR "{}", not an empty mapping "[1]" -> the SCALAR "[1]", not a sequence "a: 1" -> the key "a" So it has to go before the parser sees it; trimming it off the tokens afterwards would not recover the structure. YL now strips it in build(). The divergence is new but only half of it was: L started consuming a leading BOM in the UTF-8 round, which is what made the differential fire. YL mangling a BOM-prefixed document predates that and was simply never compared against anything. Positions are put back on the caller's coordinates, since the parser now sees an input three bytes shorter than the caller's: offsets gain the mark's width, and line 1 columns gain one character (a BOM is one character, and it is all on line one). Pinned by comparing every token of a BOM'd document against the same document without one. Tests: the parse is unchanged by a leading BOM across eight document shapes; the stream matches L's for the documents both accept (the fuzzer's invariant, pinned directly); U+FEFF anywhere else is ordinary content, including the second of two leading marks. The four shapes are seeded into FuzzYL. 2M fuzz executions clean. Conformance is unchanged at 226/85/63 — the suite's BOM fixtures are structure tests that goccy already rejected or accepted the same way. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- json/lexers/yaml-lexer/bom_test.go | 118 ++++++++++++++++++ json/lexers/yaml-lexer/fuzz_test.go | 8 ++ json/lexers/yaml-lexer/lexer.go | 5 + json/lexers/yaml-lexer/position_test.go | 41 ++++++ .../testdata/fuzz/FuzzYL/509dec0845c6303e | 2 + json/lexers/yaml-lexer/walk.go | 36 +++++- 6 files changed, 209 insertions(+), 1 deletion(-) create mode 100644 json/lexers/yaml-lexer/bom_test.go create mode 100644 json/lexers/yaml-lexer/testdata/fuzz/FuzzYL/509dec0845c6303e diff --git a/json/lexers/yaml-lexer/bom_test.go b/json/lexers/yaml-lexer/bom_test.go new file mode 100644 index 0000000..20f48aa --- /dev/null +++ b/json/lexers/yaml-lexer/bom_test.go @@ -0,0 +1,118 @@ +package lexer_test + +import ( + "testing" + + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" + + jsonlexer "github.com/go-openapi/core/json/lexers/default-lexer" + "github.com/go-openapi/core/json/lexers/token" + yamllexer "github.com/go-openapi/core/json/lexers/yaml-lexer" +) + +// A leading UTF-8 byte order mark. +// +// YAML 1.2 allows a document to be prefixed by a BOM, and it is not content. goccy does not +// strip it, and the consequence is not a dirty value but a different parse: the mark becomes the +// first character of the first token, so "{}" comes back as the SCALAR "{}" instead of +// an empty mapping. YL therefore strips it before parsing (walk.go:stripBOM). +// +// Found by the fuzzer as a JSON-subset divergence — L consumes a leading BOM, so the two lexers +// disagreed on documents that are valid to both. + +const bomPrefix = "\uFEFF" + +func TestLeadingBOMDoesNotChangeTheParse(t *testing.T) { + cases := []struct { + name string + body string + }{ + {name: "number", body: "0"}, + {name: "number after a tab", body: "\t0"}, + {name: "empty flow mapping", body: "{}"}, + {name: "flow sequence", body: "[1]"}, + {name: "flow mapping", body: "{a: 1}"}, + {name: "block mapping", body: "a: 1\n"}, + {name: "block sequence", body: "- 1\n- 2\n"}, + {name: "quoted scalar", body: `"x"`}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + want := lexKinds(t, tc.body) + got := lexKinds(t, bomPrefix+tc.body) + + assert.Equal( + t, + want, + got, + "a leading BOM is a document prefix, not content: it must not change the token stream", + ) + }) + } +} + +// TestLeadingBOMMatchesTheJSONLexer is the invariant the fuzzer checks, pinned directly: for a +// document both lexers accept, they must produce the same stream — BOM or no BOM. +func TestLeadingBOMMatchesTheJSONLexer(t *testing.T) { + for _, src := range []string{ + bomPrefix + "\t0", + bomPrefix + "0", + bomPrefix + "{}", + bomPrefix + "[1]", + bomPrefix + `{"a": 1}`, + bomPrefix + `"x"`, + } { + t.Run(src, func(t *testing.T) { + jl := jsonlexer.NewWithBytes([]byte(src)) + var want []string + for tok := range jl.Tokens() { + if tok.Kind() != token.EOF { + want = append(want, tok.Kind().String()+"("+string(tok.Value())+")") + } + } + require.NoError( + t, + jl.Err(), + "the JSON lexer must accept this for the comparison to mean anything", + ) + + assert.Equal(t, want, lexKinds(t, src)) + }) + } +} + +// TestBOMOnlyAtTheStart pins the boundary: U+FEFF anywhere else is an ordinary character. +func TestBOMOnlyAtTheStart(t *testing.T) { + t.Run("inside a scalar", func(t *testing.T) { + assert.Equal( + t, + []string{"delimiter()", "key(a)", "string(" + bomPrefix + "x)", "delimiter()"}, + lexKinds(t, "a: "+bomPrefix+"x\n"), + ) + }) + + t.Run("doubled: the second is content", func(t *testing.T) { + got := lexKinds(t, bomPrefix+bomPrefix+"0") + assert.Equal(t, []string{"string(" + bomPrefix + "0)"}, got, + "only ONE leading mark is a prefix; the second belongs to the scalar") + }) +} + +// lexKinds renders YL's stream as "kind(value)" strings. +func lexKinds(t *testing.T, src string) []string { + t.Helper() + + l := yamllexer.NewWithBytes([]byte(src)) + var out []string + for tok := range l.Tokens() { + if tok.Kind() == token.EOF { + continue + } + out = append(out, tok.Kind().String()+"("+string(tok.Value())+")") + } + require.NoErrorf(t, l.Err(), "must lex: %q", src) + + return out +} diff --git a/json/lexers/yaml-lexer/fuzz_test.go b/json/lexers/yaml-lexer/fuzz_test.go index 3f159ec..9876ac1 100644 --- a/json/lexers/yaml-lexer/fuzz_test.go +++ b/json/lexers/yaml-lexer/fuzz_test.go @@ -182,6 +182,14 @@ func FuzzYL(f *testing.F) { "e: &b\n a: 1\n <<: *b\n c: 3\n", "e: &b\n <<: [*b]\n", "a: &x\n - *x\n", + // a leading UTF-8 BOM is a document prefix, not content. goccy does not strip it, so it + // used to become the first character of the first token and change the parse outright: + // "{}" came back as a SCALAR. Caught by the JSON-subset differential, since L does + // consume a leading BOM. + "\uFEFF\t0", + "\uFEFF{}", + "\uFEFF[1]", + "\uFEFFa: 1\n", } for _, s := range seeds { f.Add([]byte(s)) diff --git a/json/lexers/yaml-lexer/lexer.go b/json/lexers/yaml-lexer/lexer.go index abd43a8..76ba647 100644 --- a/json/lexers/yaml-lexer/lexer.go +++ b/json/lexers/yaml-lexer/lexer.go @@ -37,6 +37,10 @@ type YL struct { built bool // whether toks has been built for the current input err error // sticky lexer error state + // bomBytes is 3 when the input opened with a UTF-8 byte order mark, which is stripped + // before parsing and added back to reported positions. See walk.go:build. + bomBytes int + // snapshot of the most-recently-returned token, feeding Offset/IndentLevel/Line/Column cur emit @@ -206,6 +210,7 @@ func (l *YL) reset() { l.toks = l.toks[:0] l.pos = 0 l.built = false + l.bomBytes = 0 l.cur = emit{} l.err = nil diff --git a/json/lexers/yaml-lexer/position_test.go b/json/lexers/yaml-lexer/position_test.go index 78d93ae..0c9eed8 100644 --- a/json/lexers/yaml-lexer/position_test.go +++ b/json/lexers/yaml-lexer/position_test.go @@ -261,3 +261,44 @@ func TestPositionsOverTheWholeSuite(t *testing.T) { t.Logf("ordering checked on %d documents, %d skipped for aliases", checked, skipped) } + +// TestBOMPositions pins that stripping a leading byte order mark does not shift the positions +// YL reports: the parser sees the input without the mark, so every position it hands back has to +// be put back on the caller's coordinates (walk.go:stripBOM). +func TestBOMPositions(t *testing.T) { + const bom = "\uFEFF" + + t.Run("offsets stay on the caller's bytes", func(t *testing.T) { + withBOM := lexPositions(t, bom+"{a: 1}\n") + without := lexPositions(t, "{a: 1}\n") + require.Len(t, withBOM, len(without)) + + for i := range withBOM { + assert.Equalf(t, without[i].off+uint64(len(bom)), withBOM[i].off, + "token %d: offset must account for the stripped mark", i) + } + }) + + t.Run("columns shift on line 1 only", func(t *testing.T) { + withBOM := lexPositions(t, bom+"a: 1\nb: 2\n") + without := lexPositions(t, "a: 1\nb: 2\n") + require.Len(t, withBOM, len(without)) + + for i := range withBOM { + assert.Equalf(t, without[i].line, withBOM[i].line, "token %d: line must not move", i) + + want := without[i].col + if without[i].line == 1 { + want++ // the mark is one character, and it is on line 1 + } + assert.Equalf(t, want, withBOM[i].col, "token %d: column", i) + } + }) + + t.Run("a mark that is not at the start is content", func(t *testing.T) { + // only a LEADING mark is a document prefix; anywhere else U+FEFF is an ordinary character + got := lexPositions(t, "a: "+bom+"x\n") + require.Len(t, got, 4) + assert.Equal(t, bom+"x", got[2].val) + }) +} diff --git a/json/lexers/yaml-lexer/testdata/fuzz/FuzzYL/509dec0845c6303e b/json/lexers/yaml-lexer/testdata/fuzz/FuzzYL/509dec0845c6303e new file mode 100644 index 0000000..339e206 --- /dev/null +++ b/json/lexers/yaml-lexer/testdata/fuzz/FuzzYL/509dec0845c6303e @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("\ufeff\t0") diff --git a/json/lexers/yaml-lexer/walk.go b/json/lexers/yaml-lexer/walk.go index 1920c50..f650cc5 100644 --- a/json/lexers/yaml-lexer/walk.go +++ b/json/lexers/yaml-lexer/walk.go @@ -1,6 +1,7 @@ package lexer import ( + "bytes" "strings" codes "github.com/go-openapi/core/json/lexers/error-codes" @@ -20,7 +21,7 @@ func (l *YL) build() { return } - f, err := safeParse(l.data) + f, err := safeParse(l.stripBOM()) if err != nil { l.err = err @@ -50,6 +51,30 @@ func (l *YL) build() { l.merging = nil } +// bomUTF8 is the UTF-8 encoding of U+FEFF, the byte order mark. +var bomUTF8 = []byte{0xEF, 0xBB, 0xBF} //nolint:gochecknoglobals // immutable 3-byte constant + +// stripBOM returns the input with a leading UTF-8 byte order mark removed, recording its width +// so reported positions can be put back on the caller's coordinates. +// +// YAML 1.2 allows a document to be prefixed by a BOM and it is not part of the content, but +// goccy does not strip it: it becomes the first character of the first token, which does not +// merely dirty a value -- it changes the parse. A BOM followed by "{}" comes back as the +// SCALAR "{}" rather than an empty mapping, and a BOM followed by "a: 1" yields the key +// "a". So the mark has to go before the parser sees it, not be trimmed off afterwards. +// +// The JSON lexer L consumes a leading BOM the same way (see input.CheckBOM), which is what the +// FuzzYL JSON-subset differential compares against. +func (l *YL) stripBOM() []byte { + if !bytes.HasPrefix(l.data, bomUTF8) { + return l.data + } + + l.bomBytes = len(bomUTF8) + + return l.data[len(bomUTF8):] +} + // safeParse runs the goccy parser, converting a recoverable panic into an error so a // malformed input never crashes a YL consumer. (A stack overflow from pathological nesting // is fatal and cannot be recovered — bound it with WithMaxContainerStack instead.) @@ -614,6 +639,15 @@ func (l *YL) put(tok token.T, pos *yamltoken.Position, lvl int) { e.off = uint64(pos.Offset) //nolint:gosec // no overflow, no negative values e.line = pos.Line e.col = pos.Column + if l.bomBytes > 0 { + // the parser saw the input without its byte order mark; put the reported position + // back on the caller's coordinates. The mark is 3 bytes and one character, all of + // it on line 1, so only that line's columns shift. + e.off += uint64(l.bomBytes) + if e.line == 1 { + e.col++ + } + } case len(l.toks) > 0: // No source position: an implicit value the document does not spell out (a "key:" with From 3e4dcecfa526d027a9656aa0137ecd2fc54931ef Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Fri, 31 Jul 2026 18:45:21 +0200 Subject: [PATCH 7/7] fix(yaml-lexer)!: Offset is a 0-based byte offset addressing the source YL.Offset reported goccy's Position.Offset unchanged, which is not a byte offset at all. Consumers were subtracting 1 to make it usable; that workaround is only correct for pure-ASCII documents with no comments. goccy's scanner holds the source as []rune and advances its offset once per rune, starting at 1, so Position.Offset is a 1-based RUNE index. It also loses one more per preceding comment line (goccy #856). The two errors have opposite signs, so on an ASCII document with exactly one comment they cancel and the raw value looks correct -- which is why subtracting 1 seemed to work. Line and Column are correct under both defects, so the offset is now DERIVED from them against a line-start index built once per parse, and Position.Offset is not consumed anywhere. src[Offset()] is the token's first byte, for any document: comments, accented text, CJK, emoji, and a leading BOM. Note this deliberately does not match L.Offset despite the shared name: L reports bytes consumed, a cursor pointing PAST the token it returned, while YL reports the token's own start. Both are now documented as distinct, in the API and the README. Costs one O(n) scan of the source per parse and two small slices. Benchmarked over an OpenAPI-shaped document at 2K and 38K: no measurable change (geomean +0.6%, p>0.5 at n=8), since a pure-ASCII line -- almost every line -- resolves in O(1). Also corrects a claim left stale by the earlier block-position work, in both the API docs and the README: block-style closers no longer report 0/0/0. They report the span of what they enclose, so order positions by non-decreasing rather than strictly increasing. BREAKING CHANGE: YL.Offset() values shift. Callers compensating with -1 must drop it. Offsets in documents with comments or non-ASCII text change by more than 1. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- json/lexers/yaml-lexer/README.md | 10 +++- json/lexers/yaml-lexer/bom_test.go | 50 +++++++++++++++++ json/lexers/yaml-lexer/fuzz_test.go | 20 +++++++ json/lexers/yaml-lexer/lexer.go | 22 ++++++-- json/lexers/yaml-lexer/walk.go | 83 ++++++++++++++++++++++++++++- 5 files changed, 178 insertions(+), 7 deletions(-) diff --git a/json/lexers/yaml-lexer/README.md b/json/lexers/yaml-lexer/README.md index afd0023..07ff4d8 100644 --- a/json/lexers/yaml-lexer/README.md +++ b/json/lexers/yaml-lexer/README.md @@ -75,8 +75,14 @@ for each is in **[DESIGN.md](DESIGN.md)**; in short: - **Positions on expanded content.** `Offset` / `Line` / `Column` for tokens produced by expanding an alias or a merge key point at the anchor **definition** site (expansion re-walks the original node), not the usage site. -- **Block-style closers have no position.** goccy records no position for block `}` / `]` - (only flow-style closers carry one), so they report `0/0/0`. +- **Block-style delimiters share a position with their contents.** A block collection has no + `{` / `[` characters to point at, so its opening and closing delimiters report the span of + what they enclose — the first and last token inside. Order positions by *non-decreasing*, + not strictly increasing. Flow-style delimiters report their own real characters. +- **`Offset` is not the same quantity as `L.Offset`.** `YL.Offset` is the 0-based byte offset + at which the token *starts*, so `src[Offset()]` is its first byte. The JSON lexer's `L.Offset` + is a consumption cursor that points *past* the token it just returned. Do not assume code + moving between the two lexers can keep the same arithmetic. - **Invalid UTF-8 in strings.** The JSON lexer `L` rejects unescaped control characters, but does *not* validate UTF-8 well-formedness — it passes malformed high bytes (e.g. a lone `0xA2`) through raw. goccy instead replaces them with U+FFFD. This is the one place the two diff --git a/json/lexers/yaml-lexer/bom_test.go b/json/lexers/yaml-lexer/bom_test.go index 20f48aa..acbeef6 100644 --- a/json/lexers/yaml-lexer/bom_test.go +++ b/json/lexers/yaml-lexer/bom_test.go @@ -1,6 +1,8 @@ package lexer_test import ( + "strconv" + "strings" "testing" "github.com/go-openapi/testify/v2/assert" @@ -116,3 +118,51 @@ func lexKinds(t *testing.T, src string) []string { return out } + +// TestOffsetAddressesTheSource pins that Offset is a 0-based index into the caller's bytes: the +// token's first byte is src[Offset()]. goccy counts from 1, so the walk converts; the property +// asserted here is the one consumers rely on (slicing the source), not the conversion. +// +// Note this deliberately does NOT match L.Offset, which is a consumption cursor pointing past the +// token it returned. See the doc on YL.Offset. +func TestOffsetAddressesTheSource(t *testing.T) { + for _, src := range []string{ + "abc: 1\n", + "a:\n - 10\n - twenty\n", + "{a: 1, bb: 22}\n", + "\ufeffabc: 1\n", // with a BOM, offsets stay on the caller's bytes + "# lead\nk: v\n", // goccy #856: its own offset is short by one per comment line + "# c1\n# c2\nk: v\n", + "a: 1 # trailing\nb: 2\n", + "a:\n # inner\n b: 2\n", + "é: 1\nb: 2\n", // multi-byte runes: goccy's offset counts runes, not bytes + "k: héllo wörld\nb: 2\n", + "ключ: значение\nb: 2\n", + "k: \U0001F600\nlonger: 2\n", // outside the BMP: 4 bytes, one column + "# ünïcode\nk: v\n", // both defects at once + } { + t.Run(strconv.Quote(src), func(t *testing.T) { + for _, got := range lexPositions(t, src) { + val := got.val + if val == "" { + continue // delimiters carry no text to find + } + + require.LessOrEqual(t, got.off, uint64(len(src)), + "offset past end of input for %q", val) + assert.Truef(t, strings.HasPrefix(src[got.off:], val) || + strings.HasPrefix(src[got.off:], `"`+val) || + strings.HasPrefix(src[got.off:], "'"+val), + "src[%d:] should start with %q, got %q", got.off, val, truncate(src[got.off:])) + } + }) + } +} + +func truncate(s string) string { + if len(s) > 12 { + return s[:12] + "..." + } + + return s +} diff --git a/json/lexers/yaml-lexer/fuzz_test.go b/json/lexers/yaml-lexer/fuzz_test.go index 9876ac1..d731f89 100644 --- a/json/lexers/yaml-lexer/fuzz_test.go +++ b/json/lexers/yaml-lexer/fuzz_test.go @@ -238,6 +238,26 @@ func FuzzYL(f *testing.F) { } } + // Positions stay inside the caller's buffer. Offset is DERIVED from goccy's line/column + // (see walk.go:byteOffset) rather than taken from its Position.Offset, so it is arithmetic + // of ours over adversarial input -- exactly what a fuzzer should be pointed at. A caller + // slicing the source with it must never be handed an out-of-range index. + posl := yamllexer.NewWithBytes(data, opts...) + n := 0 + for tok := range posl.Tokens() { + if n++; n > budget { + break + } + if off := posl.Offset(); off > uint64(len(data)) { + t.Fatalf("offset %d out of range for a %d-byte input (token %v)", + off, len(data), tok.Kind()) + } + if posl.Line() < 1 || posl.Column() < 1 { + t.Fatalf("non-positive position L%dC%d (token %v)", + posl.Line(), posl.Column(), tok.Kind()) + } + } + // JSON-subset differential (only for valid UTF-8, and only when both accept). // On INVALID UTF-8 inside a string the two libraries legitimately differ: the JSON // lexer passes the raw bytes through, while goccy replaces them with U+FFFD. That is diff --git a/json/lexers/yaml-lexer/lexer.go b/json/lexers/yaml-lexer/lexer.go index 76ba647..5e58972 100644 --- a/json/lexers/yaml-lexer/lexer.go +++ b/json/lexers/yaml-lexer/lexer.go @@ -37,6 +37,12 @@ type YL struct { built bool // whether toks has been built for the current input err error // sticky lexer error state + // lineStarts holds the byte offset of each source line and lineASCII whether that line is + // free of multi-byte runes, together turning a (line, column) pair back into a byte offset. + // Build-time transient (nil outside build). See walk.go:byteOffset. + lineStarts []int + lineASCII []bool + // bomBytes is 3 when the input opened with a UTF-8 byte order mark, which is stripped // before parsing and added back to reported positions. See walk.go:build. bomBytes int @@ -143,14 +149,24 @@ func (l *YL) Tokens() iter.Seq[token.T] { } } -// Offset yields the byte offset of the most-recently-returned token in the input. +// Offset yields the 0-based byte offset at which the most-recently-returned token STARTS, as an +// index into the bytes the caller supplied: src[Offset()] is the token's first byte, so the source +// text can be sliced with it directly. +// +// Note this is not the same quantity as [lexer.L.Offset], despite the shared name: L reports how +// many bytes it has consumed so far, a cursor that points PAST the token it just returned. YL walks +// a parsed document rather than a byte cursor, so it can report the token's own start, which is the +// more useful of the two. Code moving between the two lexers must not assume they agree. // // Two YAML-specific caveats apply to all of Offset/Line/Column: // - tokens produced by expanding an alias (*x) or a merge key (<<) report the position of // the anchor DEFINITION, not the alias/merge usage site (expansion re-walks the original // node); -// - a block-style closing delimiter (} or ]) reports 0: goccy records no position for it -// (only flow-style {…}/[…] closers carry one). +// - a block collection has no delimiter characters, so its opening and closing delimiters +// report the span of what they enclose — the first and last token inside it — and share +// that neighbour's position rather than sitting strictly before/after it. Order by +// non-decreasing position, not strictly increasing. Flow-style {…}/[…] delimiters report +// their own real characters. func (l *YL) Offset() uint64 { return l.cur.off } diff --git a/json/lexers/yaml-lexer/walk.go b/json/lexers/yaml-lexer/walk.go index f650cc5..e5f1e19 100644 --- a/json/lexers/yaml-lexer/walk.go +++ b/json/lexers/yaml-lexer/walk.go @@ -3,6 +3,7 @@ package lexer import ( "bytes" "strings" + "unicode/utf8" codes "github.com/go-openapi/core/json/lexers/error-codes" "github.com/go-openapi/core/json/lexers/token" @@ -21,13 +22,18 @@ func (l *YL) build() { return } - f, err := safeParse(l.stripBOM()) + src := l.stripBOM() + + f, err := safeParse(src) if err != nil { l.err = err return } + l.indexLines(src) + defer func() { l.lineStarts, l.lineASCII = nil, nil }() + switch len(f.Docs) { case 0: // empty input: a YAML document with no content resolves to null. @@ -51,6 +57,78 @@ func (l *YL) build() { l.merging = nil } +// indexLines records where each line of src starts and whether it is pure ASCII, so a +// (line, column) pair can be turned back into a byte offset. Built once per build and dropped +// when it ends; the ASCII flag rides along for free, since this pass already reads every byte. +func (l *YL) indexLines(src []byte) { + lines := bytes.Count(src, newline) + 1 + l.lineStarts = make([]int, 1, lines) // line 1 starts at 0 + l.lineASCII = make([]bool, 0, lines) + + ascii := true + for i, b := range src { + switch { + case b == '\n': + l.lineASCII = append(l.lineASCII, ascii) + l.lineStarts = append(l.lineStarts, i+1) + ascii = true + case b >= utf8.RuneSelf: + ascii = false + } + } + l.lineASCII = append(l.lineASCII, ascii) // the last line, which need not be terminated +} + +// byteOffset converts a goccy (line, column) pair into a byte offset into the parsed source. +// +// We do NOT use goccy's own Position.Offset, for two independent reasons: +// +// - it is a 1-based RUNE index, not a byte offset (goccy's scanner holds the source as +// []rune), so it cannot address the bytes of any document containing non-ASCII text, and +// drifts further with every multi-byte character before the token; +// - it loses one more per comment line preceding the token (goccy #856). +// +// Those two errors have opposite signs, so on an ASCII document with exactly one comment they +// cancel and pos.Offset looks right. It is not: patching either one alone leaves the other. +// +// Line and Column are correct under both defects, so deriving from them is the only thing that +// can be made correct without a fix upstream. See PROPOSALS-go-openapi.md §1b in the goccy +// checkout for the reproducers and what we intend to ask for. +func (l *YL) byteOffset(line, col int) uint64 { + if line < 1 || line > len(l.lineStarts) || col < 1 { + return 0 // no position we can honestly claim; put() reports the start of the input + } + + src := l.data[l.bomBytes:] + start := l.lineStarts[line-1] + end := len(src) + if line < len(l.lineStarts) { + end = l.lineStarts[line] + } + + // A column counts characters, not bytes, so in general we have to walk the line. On a pure + // ASCII line -- nearly all of them, in nearly every document -- the two coincide and we can + // index straight to it. + i := start + col - 1 + if !l.lineASCII[line-1] { + i = start + for range col - 1 { + if i >= end { + break + } + i++ + for i < end && src[i]&0xC0 == 0x80 { // skip the rune's continuation bytes + i++ + } + } + } + + return uint64(min(i, end)) //nolint:gosec // an index into a []byte is never negative +} + +// newline is the line separator scanned for by indexLines. +var newline = []byte{'\n'} //nolint:gochecknoglobals // immutable 1-byte constant + // bomUTF8 is the UTF-8 encoding of U+FEFF, the byte order mark. var bomUTF8 = []byte{0xEF, 0xBB, 0xBF} //nolint:gochecknoglobals // immutable 3-byte constant @@ -636,7 +714,8 @@ func (l *YL) put(tok token.T, pos *yamltoken.Position, lvl int) { e := emit{tok: tok, lvl: lvl} switch { case pos != nil: - e.off = uint64(pos.Offset) //nolint:gosec // no overflow, no negative values + // derived from line/column rather than taken from pos.Offset -- see byteOffset + e.off = l.byteOffset(pos.Line, pos.Column) e.line = pos.Line e.col = pos.Column if l.bomBytes > 0 {