YAML.stringify: quote strings that parse back as numbers - #30435
Conversation
stringIsNumber rejected a leading '0' followed by 'e'/'E' or '.',
and stringNeedsQuotes had no '+' case — so the stringifier emitted
'0e6836', '0.0', '+1', '+99' etc. unquoted. YAML.parse of that output
reads them as numbers, silently corrupting the round-trip:
YAML.stringify({ id: '0e6836' }) // id: 0e6836
YAML.parse('id: 0e6836') // { id: 0 }
Fix:
- stringIsNumber: after a leading '0', accept 'e'/'E'/'.' as float
continuations (not just 'x'/'X'/'o'/'O'/digit).
- stringNeedsQuotes: add a '+' case that consults stringIsNumber,
mirroring the existing '-' handling.
- Callers of stringIsNumber now pass a local scratch offset instead
of mutating their own scan index on failure; this keeps the main
loop from skipping over a flow indicator (',', '{', etc.) that
the number scanner consumed and rejected.
Closes #30433
|
Updated 4:10 PM PT - May 9th, 2026
❌ @robobun, your commit caeb5e1 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 30435That installs a local version of the PR into your bun-30435 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR updates YAML.stringify's quoting logic: it expands number detection for leading-zero exponent/decimal forms and prevents the number-check helper from mutating the string-iteration index when deciding whether to quote, and it updates tests to cover these cases. ChangesYAML Number-Like String Quoting
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/api/YAMLObject.zig`:
- Around line 744-748: The bare-string detector is missing signed scientific and
signed special floats; update the logic in YAMLObject.zig so stringIsNumber (and
the code paths that call it — the branch checking i == 0 and the similar branch
around the other block) recognizes signed exponents and signed .inf/.nan (e.g.
"+1e+5", "-1e-5", "+.inf", "-.nan") instead of treating them as unsigned
numerics; modify stringIsNumber to allow a leading '+' or '-' before the
mantissa and to accept optional signed exponent parts and signed special values,
and adjust the callers so they consult the enhanced detector; add regression
tests that assert these inputs are quoted/stringified (not treated as numbers)
to cover the previously failing cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 245be747-f741-408a-b5fa-49cdb7888387
📒 Files selected for processing (2)
src/runtime/api/YAMLObject.zigtest/js/bun/yaml/yaml.test.ts
The 10k-iteration 'strings are properly referenced' test fits within the default 5s budget on release builds (~150ms) but overruns under debug+ASAN (~17s). Give it 60s so the gate passes on both lanes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/yaml/yaml.test.ts`:
- Around line 1705-1711: The test "quotes strings whose leading number-like
prefix precedes a flow indicator" currently only round-trips objects; add a
direct emission assertion that the scalar form is quoted to catch regressions in
string quoting. Inside that test iterate the same roundTrippers values and for
each do an assertion on YAML.stringify(value) (not YAML.stringify({id: value})),
verifying the result begins and ends with a quote character (i.e., the
serialized scalar is quoted) so that quoting regressions fail even if parsing
remains permissive.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac4c82e4-aa42-40a1-af12-9ab67e482c7a
📒 Files selected for processing (1)
test/js/bun/yaml/yaml.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/yaml/yaml.test.ts`:
- Around line 1341-1345: Remove the explicit per-test timeout argument (the
trailing ", 60_000") from the test in test/js/bun/yaml/yaml.test.ts and instead
reduce the workload of the loop that calls YAML.stringify(config) — e.g., lower
the iteration count from 10_000 to a much smaller number so the test completes
under the default Jasmine/Jest timeout; ensure the loop and the test name (the
test that contains "for (let i = 0; i < 10000; i++) {
expect(YAML.stringify(config)).toBeString(); }") remain otherwise unchanged and
do not introduce any other per-test timeout calls in this bun test suite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd057009-1c74-4908-bdb4-79f8958ac2e6
📒 Files selected for processing (1)
test/js/bun/yaml/yaml.test.ts
Extend stringIsNumber to accept a sign after 'e'/'E' (so strings like
'+1e+5' and '-1e-5' are recognised as numbers and get quoted) and to
recognise the parser-special '+.inf' / '+.Inf' / '+.INF' suffix after
a leading sign. Refactored the sign tracking to a pair of booleans
(leading_sign, exp_sign) which cleanly expresses 'at most one leading
sign plus at most one exponent sign', replacing the single-per-symbol
@"+"/@"-" flags that conflated the two positions.
Before:
YAML.parse(YAML.stringify({id: '+1e+5'})) // { id: 100000 } ❌
YAML.parse(YAML.stringify({id: '+.inf'})) // { id: null } ❌
After:
YAML.parse(YAML.stringify({id: '+1e+5'})) // { id: '+1e+5' } ✓
YAML.parse(YAML.stringify({id: '+.inf'})) // { id: '+.inf' } ✓
- Add direct YAML.stringify(value) quote assertion in the flow-indicator regression test so a quoting regression fails even if the parser later rejects the bare form. - Reduce 'strings are properly referenced' from 10k to 1k iterations; that's enough to surface reference-lifetime issues and fits the default 5s timeout under debug+ASAN.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Dylan asked why stringIsNumber keeps an unused offset out-parameter. Rewriting the scanner drops it cleanly, and along the way fixes an under-quoting regression that crept in with the leading_sign/exp_sign refactor in 4f82f10. The YAML parser's tryResolveNumber accepts embedded signs mid-token — it only enforces 'no + after 0x' and 'at most one - after the leading sign'. And wtf.parseDouble is a strtod-style prefix parser, so any scan that reaches a valid end-of-token will produce a number from whatever prefix is parseable: YAML.parse(YAML.stringify({ id: '1+5' })) // → { id: 1 } ❌ YAML.parse(YAML.stringify({ id: '0+5' })) // → { id: 0 } ❌ YAML.parse(YAML.stringify({ id: '123-456' })) // → { id: 123 } ❌ Rewriting stringIsNumber as a straightforward 'will the parser's scanner call this valid?' check: - Strips the offset argument — all callers pass 0 and never read it. - Accepts embedded +/- using the same rules the parser uses. - Keeps the signed-infinity short-circuit for '+.inf' etc. - Still detects the previously-covered leading-0 exponent and leading-+ cases ('0e6836', '+1', '+1e+5', etc.). Test coverage extended with the regressing inputs.
| if (i == 0 and stringIsNumber(str)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🟡 nit: this i == 0 check is dead code — '-' is already in the leading-indicator str.charAt(0) switch above (line ~626), which returns true unconditionally before the main loop is entered, so this arm can never execute with i == 0. Since this line was touched as part of the stringIsNumber signature cleanup, it could just be deleted (the sibling callers in the '.', '0'...'9', and '+' arms remain reachable since those characters are not in the leading-indicator set).
Extended reasoning...
What the issue is
The '-' arm of the main while loop in stringNeedsQuotes contains:
if (i == 0 and stringIsNumber(str)) {
return true;
}This condition is provably unreachable. Earlier in the same function, before the main loop is entered, there is a leading-indicator switch on str.charAt(0):
switch (str.charAt(0)) {
'&', '*', '?', '|', '-', '<', '>', '!', '%', '@', ':', ',',
'[', ']', '{', '}', '#', '\'', '"', '`',
' ', '\t', '\n', '\r',
=> return true,
else => {},
}'-' is explicitly listed here, so any string whose first character is '-' returns true from stringNeedsQuotes before reaching the keywords check or the main while loop. Therefore, when execution reaches the main loop's '-' arm, str.charAt(0) is guaranteed not to be '-', which means the loop's first iteration (i == 0) cannot land in the '-' arm. The i == 0 and stringIsNumber(str) test is dead.
Step-by-step proof
Take any string s and trace stringNeedsQuotes(s):
- If
sis empty → returntrueimmediately (never reach the loop). - Trailing-char switch on
s.charAt(len-1)either returnstrueor falls through. - Leading-char switch on
s.charAt(0): ifs.charAt(0) == '-', this arm matches and the function returnstrue. Execution never proceeds past this point for any'-'-leading string. - Keywords check, then the main
whileloop begins withi = 0. - At
i = 0,str.charAt(0)is examined. By step 3, it cannot be'-'. So the'-'switch arm is not entered on the first iteration. - The
'-'arm can only be entered fori >= 1, at which pointi == 0isfalseand thestringIsNumbercall is short-circuited away.
By contrast, the three sibling callers — '.', '0'...'9', and the newly-added '+' — are reachable at i == 0 because none of those characters appear in the leading-indicator set. So this dead code is specific to the '-' arm.
Why existing code doesn't prevent it
Nothing in the function structure prevents the '-' arm from being written this way; it's simply redundant with the earlier guard. The original author presumably duplicated the i == 0 and stringIsNumber(...) pattern across all four "could start a number" arms for symmetry, without noticing that '-' was already handled unconditionally above.
Impact
None — this is purely dead code. The function's behaviour is identical with or without this line. The only cost is a small amount of reader confusion (it suggests '-'-leading strings might reach this point and need numeric classification, which they never do).
Relationship to this PR
The dead i == 0 guard pre-existed this PR (the original stringIsNumber(str, &i) call was equally unreachable). However, this PR touched this exact line as part of the stringIsNumber signature cleanup requested in review (dropping the *usize offset parameter), changing stringIsNumber(str, &i) → stringIsNumber(str). Since the cleanup pass was specifically about tidying up stringIsNumber's callers, this is a reasonable opportunity to delete the dead call rather than mechanically update it.
Suggested fix
Delete the two lines, leaving the '-' arm as:
'-' => {
if (i + 2 < str.length() and str.charAt(i + 1) == '-' and str.charAt(i + 2) == '-') {
// ... existing '---' handling ...
}
i += 1;
},No test changes needed — behaviour is unchanged.
2 participants
What
Fixes #30433.
YAML.stringifyemits number-like strings unquoted when they contain patterns the scanner does not recognise as numbers but the YAML parser does. The resulting YAML round-trips as the number, silently corrupting the value:The issue's other complaint (trailing colons like
"foo:"emitted unquoted) was already fixed on main via #25439. What remained were the number-like-string cases.Root cause
In
src/runtime/api/YAMLObject.zig:stringIsNumbershort-circuited when a leading0was followed by anything other thanx/X/o/O/digit — so"0e6836"and"0.0"were classified as non-numbers even though the YAML parser accepts them as float literals.stringNeedsQuoteshad no+entry in its main loop, so strings starting with+never reachedstringIsNumberat all —"+1","+99","+1.5","+1e5"were all emitted bare.On the failure path, callers of
stringIsNumberreused the scanner's advancedoffsetas their owni, theni += 1. That skipped whatever character made the scan fail — including flow indicators like,,{,}— so"9{","9,",".{",".,b"also slipped out unquoted and brokeYAML.parse.Fix
stringIsNumber: after a leading0, accepte/E/.as float continuations so"0e6836"and"0.0"are recognised as numbers.stringNeedsQuotes: new+case that consultsstringIsNumber, mirroring the existing-handling.-,.,0...9,+callers pass a local scratch offset tostringIsNumberinstead of mutating their own scan index, so the main loop still sees characters the number scanner looked at and rejected.Verification
Added
test/js/bun/yaml/yaml.test.tscoverage for the issue plus the flow-indicator edge cases. Updated two pre-existing assertions (+99and....) that were documenting the bug's output.Before the fix,
YAML.parse(YAML.stringify({ id: "0e6836" }))returned{ id: 0 }. After, it returns{ id: "0e6836" }.