fix(yaml): quote leading-zero integer-shaped strings in stringify#7164
fix(yaml): quote leading-zero integer-shaped strings in stringify#7164LeSingh1 wants to merge 1 commit into
Conversation
resolveYamlInteger accepts "07" as octal so stringify already quoted
it, but "08" and "09" fell through because they aren't valid octal.
They still read as numbers to humans and to other YAML parsers though,
so they need to be quoted too.
Force-quote any string matching /^[-+]?0[0-9]+$/ to cover the
non-octal leading-zero cases ("08", "09", "089", "-012", ...).
Fixes denoland#7040
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7164 +/- ##
==========================================
- Coverage 94.57% 94.57% -0.01%
==========================================
Files 636 636
Lines 52142 52144 +2
Branches 9401 9402 +1
==========================================
+ Hits 49315 49316 +1
Misses 2249 2249
- Partials 578 579 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bartlomieju
left a comment
There was a problem hiding this comment.
Fix looks correct and is at the right layer (verified locally: round-trips clean, deno test -A yaml/ 90/90, fmt+lint clean). Just two non-blocking notes inline.
| // non-octal digit, but they still read as numbers to humans and to other | ||
| // YAML parsers, so force-quote them for cross-parser safety. | ||
| const AMBIGUOUS_LEADING_ZERO_INT_REGEXP = /^[-+]?0[0-9]+$/; | ||
|
|
There was a problem hiding this comment.
Non-blocking: the same cross-parser ambiguity exists for other leading-zero number shapes this regex doesn't cover. On this branch these still emit plain:
08.5->08.5(a YAML 1.2 parser reads this as the float8.5-- the direct float analogue of the bug being fixed here)0o7->0o7(YAML 1.2 octal ->7)
These are arguably out of scope for #7040 (which is specifically leading-zero integers), so no need to expand the fix here -- but worth a one-line note in the PR description or a follow-up issue so it isn't assumed that all leading-zero numeric strings are now safe.
| }); | ||
|
|
||
| Deno.test({ | ||
| name: "stringify() quotes leading-zero numeric strings", |
There was a problem hiding this comment.
Two small suggestions (optional):
- Consider also asserting the round-trip directly, e.g.
assertEquals(parse(stringify("08")), "08"). That's the actual contract this fix protects (verified it holds), and it guards against future parser changes. - Given the
08.5/0o7gaps noted on the regex, the name"quotes leading-zero numeric strings"slightly overstates scope --"...leading-zero integer-shaped strings"matches what the regex actually covers.
Fixes #7040.
resolveYamlInteger accepts "07" as octal so stringify already quoted it, but "08" and "09" fell through because they aren't valid octal:
These still read as numbers to humans and to other YAML parsers, so they should be quoted too. The fix adds a
/^[-+]?0[0-9]+$/guard inchooseScalarStyleso any integer-shaped string with a leading zero forces single-quoted style. After the patch:Tests added in
yaml/stringify_test.tscover the issue cases plus edge cases (signed, mixed digits, non-integer-shaped strings).