Conversation
YAML.stringify wrote the first 10 characters of a string `space` as the indentation of each level. Only U+0020 can indent YAML, so "\t" gave output that YAML.parse rejects, and "- " or "#" gave output that parses to other data. A string `space` now sets the indentation width to its length, at most 10, and none of its characters are written. The `yaml` package does the same. A string of spaces gives the same output as before, and the empty string still gives flow style.
|
Status Reproduced on 1.4.3-canary (6a92015) and on main with the script in the Notes of the PR body: const v = { order: { item: ["Tea", "Mug"], paid: null } };
const text = Bun.YAML.stringify(v, null, "\t");
Bun.YAML.parse(text); // YAML Parse error: Tab characters cannot be used as indentation
The new tests in |
|
Updated 5:57 AM PT - Sep 13th, 2026
✅ @robobun, your commit 93f68da7f49e4b24d0246bc962aad906b7c1207d passed in 🧪 To try this PR locally: bunx bun-pr 42591That installs a local version of the PR into your bun-42591 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. Deferring to a human because the PR itself flags an unresolved maintainer call between three behaviour options (throw / count-as-length / document-only), it's stacked on #42483, and packages/bun-types/ has a CODEOWNER.
What was reviewed:
Space::init:unwrap_boxed_primitiveruns beforeas_string(), sonew String(...)andStringsubclasses reachJSString::length()as primitives — theis_string_literaldebug assert holds.- All four former
Space::Strmatch arms are accounted for; the enum is exhaustive so no site is missed. .length().min(10)is UTF-16 code units on ausize, matching the oldtrunc(10)unit and the new tests' astral-char expectation; theas u32cast is bounded.- Tests follow harness conventions (
test.each,Buffer.allocover.repeat, added to the existing file).
Extended reasoning...
Overview
The PR changes Bun.YAML.stringify(value, replacer, space) so that a string space argument contributes only its UTF-16 length (clamped to 10) as an indent width, instead of writing its raw characters. The Space::Str enum variant is removed and every match on Space collapses Str into the Number arm. packages/bun-types/bun.d.ts rewords the @ param space JSDoc, and test/js/bun/yaml/yaml.test.ts gains a describe block covering tabs, YAML metacharacters (#, - ), astral code points, digit strings, clamping past 10, empty/boxed/subclassed/rope strings, and a snapshot of tab-indented output.
Security risks
None identified. The change reads the length of a JSString without flattening it and never writes user-supplied bytes into the output, which strictly reduces the surface compared to the old code. No untrusted length is used unclamped (.min(10) before as u32). unwrap_boxed_primitive already handles the throwing toString path via ?.
Level of scrutiny
Medium. The Rust change is small and mechanical (delete a variant, collapse arms), and Rust's exhaustive match guarantees no site was missed. I traced the one non-obvious risk — as_string() debug-asserts is_string_literal(), while is_string() also admits StringObject/DerivedStringObject — and confirmed line 85's unwrap_boxed_primitive (backed by JSC__JSValue__unwrapBoxedPrimitive in bindings.cpp, which uses inherits<StringObject>() and so covers subclasses) always yields a primitive JSString before that branch is reached. However, this is a deliberate user-facing behaviour change to a Bun API where the author explicitly lists three viable options and states no maintainer has ruled; .claude/docs/landing-prs.md's API-design section applies, and that call belongs to a human.
Other factors
packages/bun-types/bun.d.ts is under a CODEOWNERS entry, which by itself blocks auto-approval. The PR is also stacked on #42483 (sequence-item indentation) and the new JSDoc sentence about "2 columns to the right of the dash" describes #42483's behaviour, so merge order matters. The PR notes two other open PRs (#41116, #39925) that add Space::Str arms and will need rebasing — a coordination point for a human. Test coverage is thorough and follows the repo's harness conventions.
|
No change comes from this review. Two items stay open for a maintainer:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This branch has not been deployed
1 participant
Stacked on #42483. Merge that first.
Problem
Bun.YAML.stringify(value, null, space)writes a stringspaceas the indentation."\t"givesYAML Parse error: Tab characters cannot be used as indentation."- "and"#"give output that parses to other data, with no error.Space::init(src/runtime/api/YAMLObject.rs:98). It keeps the string, andnewline()appends it for each level. ImplementBun.YAML.stringify#22183 took this fromJSON.stringify, where each indent string is legal.Fix
spacenow sets the width to its length, at most 10. None of its characters are written. The@param spacetext inbun.d.tsgives the new rule.yamlpackage does the same (options = options.length). A string of spaces and the empty string give the same output as before.test/js/bun/yaml/yaml.test.ts, block "space parameter with a string": 13 of 15 tests fail without the change, 2 are controls. All oftest/js/bun/yaml/passes. Self-reviewed: 2 concerns addressed (Notes).Background
-starts an item,#a comment.spaceis the third argument, as inJSON.stringify: a count of spaces for each level. Without it the output is one line of flow style."\t"is now width 1.Notes
Reproduction (1.4.3-canary 6a92015 and main):
space2"\t"order: \n\titem: \n\t\t- Tea...,Tab characters cannot be used as indentation1, round-trips"x"order: \nxitem: \nxx- Tea...,Unexpected token1"- "{"order":[{"item":null},[["Tea"]],[["Mug"]],{"paid":null}]}2"#"{"order":null}1The three options. No maintainer has ruled on a string
space, so here they are.TypeErrorfor a string that has a character other than U+0020. "Every accepted option does what it claims or fails loudly" (.claude/docs/landing-prs.md) supports it. It is the shape of theBun.XML.stringifyfix, where the declaration promises well-formed output. It makesYAML.stringify(v, null, "\t")throw in programs that run today with a flat object, where no indentation is written. The one public caller that was found (see below) catches errors and keeps the source text, so with A it leaves the file as it is.yamlpackage does this "for JSON compatibility" (its docs), and Prettier 3.6.2 withuseTabs: truealso writes spaces in a YAML file, with no error. Each otherspaceoutside the domain (NaN, a negative number, more than 10, an object) is already normalised with no error (YAMLObject.rs:86-106). The JSDoc now says that a string counts as its length, so the option does what it claims.docs/runtime/yaml.mdx. The output for"- "and"#"stays wrong with no error.If #42526 lands first, its
spacebullet needs the new wording. I left a comment there.What changes for a caller. Only a string that has a character other than a space changes, and only on lines inside a nested collection. A flat mapping, a top-level sequence of scalars and a sequence of one-key mappings never wrote
space, so they give the same output as before. Each other shape wrote the string. For"\t","#"and"Z"the result was a parse error or other data in all six such shapes of a probe (a sequence under a key, a mapping in a mapping, a sequence of mappings, a sequence of sequences, and two more). One accident did round-trip: a string of line breaks ("\n","\r\n"," \n") with a sequence of scalars under a key, because the items landed at column 0. It now gives spaces and parses to the same value. A search of public code found one caller that passes"\t": thepickierformatter (packages/pickier/src/format.ts,formatYaml) when its indent style is tabs. It writes the result back to the file, so today it gives a file that does not parse. With this PR it gives 1-space indentation.Length unit. UTF-16 code units, as
.lengthand as theyamlpackage:"😀"is 2. The old code clamped withtrunc(10), the same unit. The length comes fromJSString::length(), so a rope is not flattened.Round-trip probe. 3,001 values (3,000 seeded random values to depth 5 with strings that need quotes, plus one with shared references) at 31
spacestrings ("\t","- ","#",": ","&a ","? ","| ","\n","\r\n", U+00A0, U+2028,"\0", quotes, brackets,"---", 16 characters and more): 93,031 round trips. This branch: 0 fail, and each output is the same as for the number. 1.4.3: 36,426 fail (this bug and the one that #42483 fixes).Workaround on a released version. Pass a number:
Bun.YAML.stringify(v, null, typeof s === "string" ? Math.min(10, s.length) : s).Self-review. Two concerns changed this PR. The first JSDoc text said that each level of indentation gets
space. With #42483 a sequence item is always 2 columns, so[[1,[2,3]],[4]]gives the same text at 1 and at 8. The text now says thatspaceindents a nested value from its key, and it has the sentence about sequence items that #42483 left for this change (084b6f3). The first Notes text said that no old output with such a string parsed. One shape did, see "What changes for a caller".Other open PRs. #41116 and #39925 each add a
Space::Strarm. The one that lands after this PR drops that arm.Not changed.
Bun.JSON5.stringifywritesspaceas it is, which is correct for JSON5 and the same as thejson5package.Bun.XML.stringifyhas its own fix. The two tests that already pass"\t"(space parameter with boxed String, and the loop over[2, 4, "\t", undefined]) compare one stringify output with another and pass with no edit.[human-review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file