Skip to content

yaml: clear remaining 3 non-cyclic test.todo entries (BOM strip, 2-anchor e-node key, flow cmi gap) #31569

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dylan-conway merged 14 commits into from
May 29, 2026
Merged

yaml: clear remaining 3 non-cyclic test.todo entries (BOM strip, 2-anchor e-node key, flow cmi gap) #31569

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6f088e2
yaml: clear remaining 3 non-cyclic test.todo entries
dylan-conway May 29, 2026
8687f17
[autofix.ci] apply automated fixes
autofix-ci[bot] May 29, 2026
544b779
yaml: §5.2 byte-order-mark encoding detection; fix `<BOM>#` regression
dylan-conway May 29, 2026
6187d11
yaml: review fixes — mirror [147] FlowIn early-return to Alias/SeqSta…
dylan-conway May 29, 2026
6885852
yaml: validate Core schema number regex before parse_double
dylan-conway May 29, 2026
86ec4aa
[autofix.ci] apply automated fixes
autofix-ci[bot] May 29, 2026
e341f05
yaml: add test.todo for 14 bug categories surfaced by multi-modal bug…
dylan-conway May 29, 2026
755f18a
[autofix.ci] apply automated fixes
autofix-ci[bot] May 29, 2026
938fb32
yaml: review nits — is_multiple_of, Infinity sign asserts, UTF-16 Loc…
dylan-conway May 29, 2026
fc12ed7
[autofix.ci] apply automated fixes
autofix-ci[bot] May 29, 2026
89c9b9b
yaml: clippy — drop redundant `_, ..` in detect_encoding match
dylan-conway May 29, 2026
fcd70c0
yaml: review nits — split mirrored-arm asserts to own test; toThrow f…
dylan-conway May 29, 2026
12c16e5
yaml: revert §5.2 encoding detection — keep only UTF-8 BOM strip
dylan-conway May 29, 2026
efcfffe
yaml: review nits — signed-hex/octal as test.todo, YAML.parse import,…
dylan-conway May 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
237 changes: 212 additions & 25 deletions src/parsers/yaml.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,32 @@ use bun_core::{self, StackCheck};

pub struct YAML;

/// Spec §5.2 byte-order-mark / null-byte encoding detection.
#[derive(Clone, Copy, PartialEq, Eq)]
enum DetectedEncoding {

@Jarred-Sumner Jarred-Sumner May 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go in a bun_core::bom module

@dylan-conway dylan-conway May 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — though detect_encoding was just removed in 12c16e5 (Dylan flagged that Parser<Utf16> has never been exercised, so shipping it or erroring on detected UTF-16 byte input both trade one untested behavior for another). Only the UTF-8 BOM strip in Parser::init remains.

When §5.2 detection ships properly (after a full Parser<Utf16> test sweep), bun_core::bom is the right home so JSON5/TOML/JSONC can share it.

Utf8,
Utf16Le,
Utf16Be,
Utf32Le,
Utf32Be,
}

fn detect_encoding(bytes: &[u8]) -> DetectedEncoding {
use DetectedEncoding::*;
let b = |i: usize| bytes.get(i).copied().unwrap_or(0xFF);
match (b(0), b(1), b(2), b(3)) {
(0x00, 0x00, 0xFE, 0xFF) => Utf32Be,
(0xFF, 0xFE, 0x00, 0x00) => Utf32Le,
(0x00, 0x00, 0x00, _) => Utf32Be,
(_, 0x00, 0x00, 0x00) => Utf32Le,
(0xFE, 0xFF, _, _) => Utf16Be,
(0xFF, 0xFE, _, _) => Utf16Le,
(0x00, ..) => Utf16Be,
(_, 0x00, _, _) => Utf16Le,
_ => Utf8,
}
}

impl YAML {
pub fn parse(
source: &bun_ast::Source,
Expand All @@ -34,12 +60,59 @@ impl YAML {
// Zig: `bun.analytics.Features.yaml_parse += 1;`
bun_core::analytics::Features::yaml_parse_inc();

let mut parser: Parser<Utf8> = Parser::init(bump, source.contents());
let bytes = source.contents();
match detect_encoding(bytes) {
DetectedEncoding::Utf8 => Self::parse_with::<Utf8>(source, bytes, log, bump),
enc @ (DetectedEncoding::Utf16Le | DetectedEncoding::Utf16Be) => {
let be = matches!(enc, DetectedEncoding::Utf16Be);
if !bytes.len().is_multiple_of(2) {
log.add_error(
Some(source),
bun_ast::Loc::EMPTY,
b"UTF-16 input has odd byte length",
);
return Err(YamlParseError::SyntaxError);
}
// The Expr tree may hold slices into the input; allocate the
// transcoded buffer in the same arena so it shares its lifetime.
// TODO: Parser<Utf16> tracks `pos` as a u16 index, but
// `Pos::loc()` is interpreted as a byte offset into
// `source.contents` — error positions for UTF-16 byte input are
// off. Fixing requires mapping u16 index → byte offset before
// constructing `Loc`.
let units: &mut [u16] = bump.alloc_slice_fill_default(bytes.len() / 2);
for (i, c) in bytes.chunks_exact(2).enumerate() {
units[i] = if be {
u16::from_be_bytes([c[0], c[1]])
} else {
u16::from_le_bytes([c[0], c[1]])
};
}
Self::parse_with::<Utf16>(source, units, log, bump)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}
DetectedEncoding::Utf32Le | DetectedEncoding::Utf32Be => {
log.add_error(
Some(source),
bun_ast::Loc::EMPTY,
b"UTF-32 input is not supported",
);
Err(YamlParseError::SyntaxError)
}
}
}

fn parse_with<Enc: Encoding>(
source: &bun_ast::Source,
input: &[Enc::Unit],
log: &mut bun_ast::Log,
bump: &bun_alloc::Arena,
) -> Result<Expr, YamlParseError> {
let mut parser: Parser<Enc> = Parser::init(bump, input);

let stream = match parser.parse() {
Ok(s) => s,
Err(e) => {
let err = ParseResult::<Utf8>::fail(e, &parser);
let err = ParseResult::<Enc>::fail(e, &parser);
if let ParseResult::Err(err) = err {
err.add_to_log(source, log)?;
}
Expand Down Expand Up @@ -463,6 +536,9 @@ pub trait Encoding: Copy + 'static {
/// buffer (see `EncLit` doc for the const-generics rationale).
fn literal(s: &'static [u8]) -> EncLit<Self::Unit>;

/// Number of leading units to skip if `input` starts with [3] c-byte-order-mark.
fn bom_len(input: &[Self::Unit]) -> usize;

/// Reinterpret a `&[Unit]` slice as `&[u8]` for `StringHashMap` keying
/// (`anchors` / `tag_handles`). Zig's `bun.StringHashMap` is keyed by
/// `[]const u8`; calls like `tag_handles.put(handle.slice(self.input), {})`
Expand Down Expand Up @@ -523,6 +599,10 @@ impl Encoding for Latin1 {
// Only reachable from `EncodingKind::Utf16`-gated arms.
unreachable!("unit_from_u16 on Latin1")
}
#[inline]
fn bom_len(_input: &[u8]) -> usize {
0
}
}

impl Encoding for Utf8 {
Expand Down Expand Up @@ -551,6 +631,14 @@ impl Encoding for Utf8 {
// Only reachable from `EncodingKind::Utf16`-gated arms.
unreachable!("unit_from_u16 on Utf8")
}
#[inline]
fn bom_len(input: &[u8]) -> usize {
if input.len() >= 3 && input[0] == 0xEF && input[1] == 0xBB && input[2] == 0xBF {
3
} else {
0
}
}
}

impl Encoding for Utf16 {
Expand Down Expand Up @@ -589,6 +677,10 @@ impl Encoding for Utf16 {
u
}
#[inline]
fn bom_len(input: &[u16]) -> usize {
if input.first() == Some(&0xFEFF) { 1 } else { 0 }
}
#[inline]
fn as_u16_slice(s: &[u16]) -> &[u16] {
s
}
Expand Down Expand Up @@ -1476,7 +1568,12 @@ impl<'i, Enc: Encoding> ScalarResolverCtx<'i, Enc> {
let mut minus = false;
let mut hex = false;

if first_char != FirstChar::Negative && first_char != FirstChar::Positive {
// For Negative/Positive the sign was consumed by the caller; the first
// body char (digit or `.`) is at `pos`. For Other/Dot the caller left
// `pos` at the first body char too. Either way, advance past it so the
// loop starts at the second body char with the `decimal`/digit flags
// already reflecting the first.
if !matches!(first_char, FirstChar::Negative | FirstChar::Positive) || decimal {
parser!().inc(1);
}

Expand Down Expand Up @@ -1630,15 +1727,23 @@ impl<'i, Enc: Encoding> ScalarResolverCtx<'i, Enc> {
return Ok(());
}

let lexed = parser!().slice(start, end);
let mut scalar: NodeScalar<Enc> = 'scalar: {
if x || o || hex {
let unsigned = match parse_unsigned_radix0::<Enc>(parser!().slice(start, end)) {
let unsigned = match parse_unsigned_radix0::<Enc>(lexed) {
Ok(v) => v,
Err(_) => return Ok(()),
};
break 'scalar NodeScalar::Number(unsigned as f64);
}
let float = match parse_double_generic::<Enc>(parser!().slice(start, end)) {
// [10.2.1.4] Core schema float/int regex. The lexer loop above is
// permissive (accepts `+`/`-`/`e`/`.` at any position) and
// `wtf::parse_double` prefix-parses, so `1+1` would resolve as 1.
// Validate the consumed slice matches the schema before parsing.
if !is_core_schema_number::<Enc>(lexed, first_char) {
return Ok(());
}
Comment thread
claude[bot] marked this conversation as resolved.
let float = match parse_double_generic::<Enc>(lexed) {
Ok(v) => v,
Err(_) => return Ok(()),
};
Expand All @@ -1664,6 +1769,59 @@ impl<'i, Enc: Encoding> ScalarResolverCtx<'i, Enc> {
}
}

/// [10.2.1.4] Core schema int/float pattern. The slice may already have had a
/// leading `.` or `+`/`-` consumed by the caller before `start` was captured;
/// `first_char` carries that.
/// `[-+]? ( \. [0-9]+ | [0-9]+ ( \. [0-9]* )? ) ( [eE] [-+]? [0-9]+ )?`
fn is_core_schema_number<Enc: Encoding>(s: &[Enc::Unit], first_char: FirstChar) -> bool {
let mut i = 0usize;
let len = s.len();
let at = |j: usize| Enc::wide(s[j]);
let is_digit = |c: u32| (0x30..=0x39).contains(&c);

// Mantissa: \. [0-9]+ | [0-9]+ ( \. [0-9]* )?
let saw_leading_dot = first_char == FirstChar::Dot
|| (i < len && at(i) == 0x2E && {
i += 1;
true
});
if saw_leading_dot {
if i >= len || !is_digit(at(i)) {
return false;
}
while i < len && is_digit(at(i)) {
i += 1;
}
} else {
if i >= len || !is_digit(at(i)) {
return false;
}
while i < len && is_digit(at(i)) {
i += 1;
}
if i < len && at(i) == 0x2E {
i += 1;
while i < len && is_digit(at(i)) {
i += 1;
}
}
}
// Optional exponent: [eE] [-+]? [0-9]+
if i < len && matches!(at(i), 0x65 | 0x45) {
i += 1;
if i < len && matches!(at(i), 0x2B | 0x2D) {
i += 1;
}
if i >= len || !is_digit(at(i)) {
return false;
}
while i < len && is_digit(at(i)) {
i += 1;
}
}
i == len
}

/// Port of `bun.jsc.wtf.parseDouble(slice)` over an encoding-generic slice.
/// `bun_core::wtf::parse_double` takes `&[u8]`; for `Utf8`/`Latin1` we narrow
/// via `Enc::key_bytes` (identity). For `Utf16` the lexer guarantees the
Expand Down Expand Up @@ -2359,11 +2517,13 @@ impl<'i, Enc: Encoding> Parser<'i, Enc> {
pub const MAX_ALIAS_EXPANSION: usize = 16 * 1024 * 1024;

pub fn init(bump: &'i bun_alloc::Arena, input: &'i [Enc::Unit]) -> Self {
// [206] l-document-prefix ::= c-byte-order-mark? l-comment*
let start = Pos::from(Enc::bom_len(input));
Self {
input,
bump,
pos: Pos::from(0),
line_start_pos: Pos::from(0),
pos: start,
line_start_pos: start,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
claude[bot] marked this conversation as resolved.
Comment on lines +2447 to +2453

@claude claude Bot May 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 🟣 Pre-existing, non-blocking: this strips the BOM at stream-start only, but [211] l-yaml-stream admits l-document-prefix (with c-byte-order-mark?) before each document — and lists c-byte-order-mark directly in its trailing alternation — so per §5.2 "byte order marks may appear at the start of any document". a: 1\n...\n---\nb: 2 (concatenated UTF-8-with-BOM files) won't have the second-document BOM stripped; the U+FEFF reaches scan_plain_scalar and --- parses as content instead of c-byte-order-mark + c-directives-end. Also makes the new test comment at yaml.test.ts:1753 ("only [206] document-prefix") slightly imprecise — only the first [206] instance is handled. Strictly improved vs main; worth a test.todo alongside the §5.2 deferral already discussed in the thread.

Extended reasoning...

What

Parser::init (yaml.rs:2447-2453) strips a leading BOM by setting pos/line_start_pos to Pos::from(Enc::bom_len(input)), citing [206] l-document-prefix ::= c-byte-order-mark? l-comment*. This handles the BOM at byte 0 only. But [206] is invoked from [211] l-yaml-stream:

l-yaml-stream ::= l-document-prefix* l-any-document?
                  ( ( l-document-suffix+ l-document-prefix* l-any-document? )
                    | c-byte-order-mark | l-comment | l-explicit-document )*

so l-document-prefix (with its optional c-byte-order-mark) recurs after each l-document-suffix+ (...), and c-byte-order-mark is also a direct alternative in the trailing repetition. §5.2 says this explicitly: "byte order marks may appear at the start of any document". The PR implements only the first [206] instance at stream-start.

Code path

Enc::bom_len() is called exactly once, in Parser::init. The document loop (parse() → parse_stream / next-document re-entry) does not re-check for a BOM after DocumentEnd / .... There is no 0xFEFF / 0xEF arm in scan() that would consume a mid-stream BOM at a document boundary; after ...\n, the next scan() reads 0xEF (UTF-8) or 0xFEFF (UTF-16), which falls to the catch-all _ arm → scan_plain_scalar, so --- lexes as a plain scalar rather than c-byte-order-mark followed by c-directives-end.

Step-by-step proof

Input (UTF-8): a: 1\n...\n---\nb: 2 i.e. 61 3A 20 31 0A 2E 2E 2E 0A EF BB BF 2D 2D 2D 0A 62 3A 20 32.

  1. Parser::init: Enc::bom_len([0x61, …]) = 0 (input does not start with EF BB BF), so pos = line_start_pos = 0. No BOM stripped.
  2. First document parses as {a: 1}; scanner reaches ... → DocumentEnd; pos advances past the trailing \n to byte 9.
  3. Next-document scan: self.next() at pos=9 returns 0xEF. The scan() dispatch has no arm for 0xEF/BOM bytes, so it falls to the _ catch-all → scan_plain_scalar.
  4. scan_plain_scalar consumes EF BB BF 2D 2D 2D as scalar content (BOM is nb-char, - is ns-plain-safe). is_at_line_start() is not consulted for --- recognition here because the scanner already committed to the plain-scalar path on the first byte; even if it were, pos(9) != line_start_pos(9) only at the moment - is reached after the BOM advanced pos.
  5. Result: the second document's --- is treated as content, not as c-byte-order-mark? + c-directives-end. The BOM is not stripped per [211]/[206].

This is the realistic shape of cat doc1.yaml doc2.yaml where each was saved by an editor that emits a UTF-8 BOM, with ... terminating the first.

Why nothing else catches it

bom_len() is only invoked from init — there is no per-document hook. The new test at yaml.test.ts:1753 documents the opposite (BOM mid-content is preserved as scalar text), which is correct for U+FEFF inside a node, but the document-suffix→document-prefix boundary is a different grammar position where [211] explicitly admits a BOM. The test comment "BOM mid-stream is not stripped (only [206] document-prefix)" is itself slightly imprecise: [206] l-document-prefix can recur per [211]; what's actually implemented is "only the stream-start instance of [206]".

Impact

Obscure (multi-document streams with per-document BOM, e.g. concatenated UTF-8-with-BOM files), and strictly improved vs main — before this PR no BOM was stripped at all. Not a regression. The §5.2 encoding-detection deferral already discussed in the thread (Jarred/Dylan re bun_core::bom, commit 12c16e5) is about UTF-16/UTF-32 detection, not per-document BOM at [211] boundaries, so this isn't already covered by that follow-up. This is the immediate [211] sibling of the [206] rule the PR cites at yaml.rs:2447 — same pattern as the other pre-existing siblings (Alias/SequenceStart/MappingStart [147] mirrors, set_tag analogue) already filed and addressed on this PR.

Fix

When the document loop sees DocumentEnd (or before scanning the next document's first token), check Enc::bom_len(&self.input[self.pos.cast()..]) and advance pos/line_start_pos past it — or add a 0xFEFF / EF BB BF arm in scan() that, at is_at_line_start() between documents, consumes the BOM and continues. Either way, also tighten the test comment at yaml.test.ts:1753 to "only the stream-start [206] instance", and add a test.todo for YAML.parse("a: 1\n...\n---\nb: 2", { all: true }) (or equivalent multi-doc API) alongside the existing §5.2 deferral.

line_indent: Indent::NONE,
tab_after_indent: false,
line: Line::from(1),
Expand Down Expand Up @@ -3467,7 +3627,7 @@ impl<Enc: Encoding> NodeProperties<Enc> {

pub fn set_anchor(&mut self, anchor_token: Token<Enc>) -> Result<(), ParseError> {
if let Some(previous_anchor) = &self.has_anchor {
if previous_anchor.line == anchor_token.line {
if previous_anchor.line == anchor_token.line || self.has_mapping_anchor.is_some() {
return Err(ParseError::MultipleAnchors);
}
self.has_mapping_anchor = Some(previous_anchor.clone());
Expand Down Expand Up @@ -3977,6 +4137,11 @@ impl<'i, Enc: Encoding> Parser<'i, Enc> {
}
}

// [147] flow-map value is ns-flow-node, not a pair.
if self.context.get() == Context::FlowIn && !opts.flow_pair_allowed {
return Ok(copy);
}

let map = self.parse_block_mapping(
copy,
alias_start,
Expand Down Expand Up @@ -4028,6 +4193,11 @@ impl<'i, Enc: Encoding> Parser<'i, Enc> {
}
}

// [147] flow-map value is ns-flow-node, not a pair.
if self.context.get() == Context::FlowIn && !opts.flow_pair_allowed {
break 'node seq;
}

let implicit_key_anchors =
node_props.implicit_key_anchors(sequence_line)?;

Expand Down Expand Up @@ -4115,6 +4285,11 @@ impl<'i, Enc: Encoding> Parser<'i, Enc> {
}
}

// [147] flow-map value is ns-flow-node, not a pair.
if self.context.get() == Context::FlowIn && !opts.flow_pair_allowed {
break 'node map;
}

let implicit_key_anchors = node_props.implicit_key_anchors(mapping_line)?;

if let Some(key_anchor) = implicit_key_anchors.key_anchor {
Expand Down Expand Up @@ -4234,14 +4409,13 @@ impl<'i, Enc: Encoding> Parser<'i, Enc> {
self.anchors
.put(Enc::key_bytes(mapping_anchor.slice(self.input)), mapping)?;
}
// Clear has_anchor so the post-loop fallback doesn't
// re-register the inner anchor on the mapping. The
// remaining fields reach the post-loop guards: this
// matches main's behavior (rejects `!!a\n!!b\n: x` and
// `&a\n&b\n&c : x`), but also over-rejects the valid
// `&outer\n&inner : x` — pre-existing; the Scalar arm
// avoids it via `return Ok`.
// Anchors are fully consumed by implicit_key_anchors;
// clear both so the post-loop fallback doesn't re-register
// (or over-reject `&outer\n&inner : x` via the
// has_mapping_anchor guard). Tag fields stay so the
// has_mapping_tag guard still catches `!!a\n!!b\n: x`.
node_props.has_anchor = None;
node_props.has_mapping_anchor = None;
Comment thread
claude[bot] marked this conversation as resolved.
break 'node mapping;
}

Expand Down Expand Up @@ -4325,6 +4499,16 @@ impl<'i, Enc: Encoding> Parser<'i, Enc> {
}
}

// [147] flow-map value is ns-flow-node, not a pair.
// Return the bare scalar; the leftover `:` reaches the
// caller's [140] check. The cmi==scalar_indent path
// above already handles the same-line case; this
// covers the multiline-property case where they
// diverge.
if self.context.get() == Context::FlowIn && !opts.flow_pair_allowed {
break 'node scalar.data.to_expr(scalar_start, self.input, self.bump);
}

Comment thread
claude[bot] marked this conversation as resolved.
let implicit_key = scalar.data.to_expr(scalar_start, self.input, self.bump);

let implicit_key_anchors = node_props.implicit_key_anchors(scalar_line)?;
Expand Down Expand Up @@ -4575,9 +4759,11 @@ impl<'i, Enc: Encoding> Parser<'i, Enc> {
}

0x23 /* '#' */ => {
let prev = parser!().input[parser!().pos.sub(1).cast()];
if parser!().pos == Pos::ZERO
|| matches!(Enc::wide(prev), 0x20 | 0x09 | 0x0D | 0x0A)
if parser!().is_at_line_start()
|| matches!(
Enc::wide(parser!().input[parser!().pos.sub(1).cast()]),
0x20 | 0x09 | 0x0D | 0x0A
)
{
return Ok(ctx.done());
}
Expand Down Expand Up @@ -5857,13 +6043,14 @@ impl<'i, Enc: Encoding> Parser<'i, Enc> {
}
0x23 /* '#' */ => {
let start = self.pos;
let prev = if start == Pos::ZERO { 0 } else { Enc::wide(self.input[start.cast() - 1]) };
match prev {
0 | 0x20 | 0x09 | 0x0A | 0x0D => {}
_ => {
// TODO: prove this is unreachable
return Err(ParseError::UnexpectedCharacter);
}
if !self.is_at_line_start()
&& !matches!(
Enc::wide(self.input[start.cast() - 1]),
0x20 | 0x09 | 0x0A | 0x0D
)
{
// TODO: prove this is unreachable
return Err(ParseError::UnexpectedCharacter);
}
self.inc(1);
while !self.is_b_char_or_eof() {
Expand Down
Loading
Loading