fix(toml): emit TOML literals for Infinity/NaN/Date in arrays#7163
fix(toml): emit TOML literals for Infinity/NaN/Date in arrays#7163spokodev wants to merge 2 commits into
Conversation
Primitive and mixed array stringification went through code paths
that produced invalid TOML for non-finite numbers and Date values:
- `[Infinity, -Infinity, NaN]` rendered as `[null,null,null]`
because `#arrayDeclaration` used `JSON.stringify` which has no
TOML equivalents for those values.
- `[new Date(0)]` rendered as `["1970-01-01T00:00:00.000Z"]` for
the same reason - a quoted ISO string instead of a TOML datetime
literal.
- Mixed arrays like `[Infinity, {}]` and `[new Date(0), {}]` ran
through `#printAsInlineValue` which returned numbers verbatim
(so Infinity printed as the JS identifier) and wrapped Dates in
string quotes.
Introduce `#printPrimitive` that produces TOML-compliant literals
for Date, string, RegExp, number (including inf/-inf/nan) and
boolean. `#arrayDeclaration` and `#printAsInlineValue` now route
primitives through it.
The behaviour for `null`/`undefined` inside arrays is left
unchanged (still throws "Should never reach") since the issue
author flagged it as a design call for the maintainers.
Updates one existing test that encoded the old quoted-Date output
inside a mixed array. The new output matches the TOML 1.0 spec for
datetime literals.
Fixes denoland#7162 (partial - null handling deferred per the issue).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7163 +/- ##
==========================================
+ Coverage 94.57% 94.84% +0.26%
==========================================
Files 636 617 -19
Lines 52142 51682 -460
Branches 9401 9358 -43
==========================================
- Hits 49315 49016 -299
+ Misses 2249 2121 -128
+ Partials 578 545 -33 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
recheck |
bartlomieju
left a comment
There was a problem hiding this comment.
Thanks for this — really thorough PR, and the write-up is excellent (clear before/after, honest about the deferred null design call, and well-scoped). I verified the positive fixes locally and they all work: [Infinity,-Infinity,NaN] -> [inf,-inf,nan], dates emit unquoted and reparse as Date, finite numbers are preserved, and deno test -A toml/ is 689/0 with fmt+lint clean.
There's one regression introduced by the refactor that needs a fix before this can land — RegExp values inside mixed arrays get dropped. Details inline. Once that's handled (plus a small regression test) this is in good shape.
Heads-up: #7171 is an open PR fixing the same issue (#7162) with the same inf/-inf/nan + unquoted-Date approach — worth a look so the two efforts don't collide.
| if (!value) { | ||
| throw new Error("Should never reach"); | ||
| } | ||
| } else if (value && typeof value === "object" && !(value instanceof Date)) { |
There was a problem hiding this comment.
This branch catches RegExp as well as plain objects, which silently drops RegExp values in mixed arrays. Verified against main:
stringify({ x: [/foo/, {}] })
// main: x = ["/foo/",{}] ✅
// this PR: x = [{},{}] ❌ RegExp lostReason: typeof /foo/ === "object" and a RegExp isn't a Date, so a RegExp enters this inline-table branch, Object.keys(/foo/) is [], and it serializes as {}. The old code handled RegExp before the object branch (via the string || RegExp arm), so it never reached here.
Note the asymmetry that hides it: primitive arrays go through #arrayDeclaration -> #printPrimitive, which handles RegExp fine (["/a/","/b/"]). Only the mixed path is broken, and there's no test for RegExp-in-mixed-array, so the suite stays green.
Suggested fix — exclude RegExp the same way Date is:
} else if (
value && typeof value === "object" &&
!(value instanceof Date) && !(value instanceof RegExp)
) {and a regression test, e.g. assertEquals(stringify({ x: [/foo/, {}] }), 'x = ["/foo/",{}]\n').
The inline-table branch in #printAsInlineValue matched any non-Date object,
so a RegExp in a mixed array was serialized as an empty {} and dropped.
Exclude RegExp the same way Date is, so it falls through to the primitive
path. Adds a regression test.
|
Thanks for the careful review and the precise repro. Fixed: excluded On #7171: thanks for the heads-up. This PR is broader (it also covers -Infinity, NaN, and the deferred null design), so I will leave the call to you on which to land; I can rebase around the other if you prefer it. |
Primitive and mixed array stringification went through code paths that produced invalid TOML for non-finite numbers and Date values. The issue author flagged six distinct cases in #7162; this PR addresses the four with a deterministic spec-aligned fix and leaves the two
null-related ones for maintainer guidance, since they are a design call.What changed
#arrayDeclaration(primitive arrays) previously routed values throughJSON.stringify, which has no representation for Infinity, -Infinity, NaN, or TOML datetime literals.#printAsInlineValue(mixed arrays) returned numbers verbatim (so Infinity printed as the JS identifier) and wrapped Dates in string quotes.A new private helper
#printPrimitiveproduces TOML-compliant literals forDate,string,RegExp,number(includinginf/-inf/nan) andboolean. Both array paths now route primitives through it.Behaviour for
null/undefinedinside arrays is unchanged - the code still throwsShould never reach. The issue author flagged this as a design call (TOML has nonull), so it stays as-is until maintainers decide between a clearer error message, silent skip, or another shape.Before/after
Tests
Six new cases in
toml/stringify_test.ts:inf/-inf/nan#printPrimitivepath that replacedJSON.stringify)One existing test (
stringify() handles mixed array) had a date inside a mixed inline-table that was previously emitted with quotes. The expected output is updated to the spec-aligned bare datetime literal.`deno test --allow-all toml/` -> 689 passed, 0 failed, 111 ignored.
`deno fmt` and `deno lint` clean on the changed files.
Fixes #7162 (partial - null handling left for maintainer guidance).