nanvm: format bigint string coercion as decimal - #1709

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fb01c968f4

The formatting is correct: 2,044 cases, 0 mismatches, against two independent oracles — Node BigInt(...).toString() and Python int, both reconstructed from the raw hex limb dump so neither shares code with the implementation. 1,316 fit u128, 728 exceed it (up to 8 limbs). Corpus included non-normalized all-zero limb vectors, the 10^19 group boundary, 2^k ± 1 for k=0..127, 10^n ± 1 for n=0..38, and ~500 random vectors with interior zero limbs, each in both signs. No leading zeros, no lone -, digits only. -0 renders "0", matching JS.

generated.rs is genuine regeneration output, not hand-edited — I ran the generator (no diff), then appended a tamper line and re-ran; it was overwritten and git went clean. All three todo tasks are genuinely done.

Rust gates clean (fmt, test 110+1+5, clippy), npm test 3378/0, tsc clean. The --all-targets clippy failure is pre-existing — confirmed at the merge-base, same single eq_op error.

Two things to fix:

changelog/unreleased/1709.md is missing. The body's Changelog: section is correctly shaped and its bullet is right, but CONTRIBUTING is explicit that the file and the section are duplicated on purpose — "the file is what today's release process reads, the section is what a generator reading Git history would read. Neither is derived from the other, so keep them identical." The bullet can be copied verbatim.

fjs/nanvm/README.md:87 still carries a "Known divergence" section whose link points at the file this PR deletes, and whose every claim is now false — the divergence is gone and neither case carries a rust reason any more. That README's own text says the per-case arrangement exists because "a table of them goes stale the moment someone fixes one"; this is that. No link checker in CI will catch it. It was the only remaining reference.

Not verified: only the generator half of npm run ci-update was run (deno/bun unavailable here, same limitation you reported); the tamper test covers the one file this change regenerates.

Delta is merges of main only — git log --oneline --no-merges fb01c968f..da2e90863 --not origin/main is empty, so nothing branch-authored landed. Main moved a lot underneath (#1699, #1708, #1710-#1715, #1718), so I re-ran the gates at the merged head: npm test 3400/0, tsc --noEmit exit 0, cargo fmt -- --check exit 0, cargo test 110 + 1 + 5 doctests all passing, plain cargo clippy clean. The --all-targets eq_op failure at nanvm-lib/src/vm/mod.rs:40 is still pre-existing — identical on origin/main, and this PR doesn't touch that file.

Both of my findings are still open.

changelog/unreleased/1709.md is still missing. The body's section is correctly shaped and last before the Codex trailer, and its bullet is right — this is a behaviour change, so an entry is owed. CONTRIBUTING is explicit that the file and the section duplicate each other on purpose and must be kept identical; creating the file with that exact bullet closes it.

fjs/nanvm/README.md's "Known divergence" section is still there, now at lines 84-91, and it is still the only remaining reference to the file this PR deletes. All three of its claims are false at head: the link target nanvm-lib/todo/bigint-decimal-string-coercion.md no longer exists (dead link — grep across fjs, nanvm-lib, todo and changelog finds only this line); the "0x7Bn" divergence is what the PR fixes; and "the two affected cases carry a rust reason" is false because the PR deletes both — fjs/nanvm/module.f.mjs drops hexadecimalBigint and both rust: fields, and generated.rs:188-193 uncomments both check::<A> calls. The section should go with the file it documents.

Measured at 975745950. The delta since my last review is a merge of main only — git log --oneline --no-merges 975745950 --not origin/main is still just fb01c968f — so nothing branch-authored landed and both earlier findings stand.

Gates are green and match main: cargo build 0, cargo test 0 (110 + 1 + 5 doctests, 0 failed), cargo fmt --check 0, cargo clippy 0, npm test 3447/0. The behaviour is right: I ran the new Display on 14 bigints via BigInt::<Naive>::normalize_new and diffed against node's String(BigInt(...)) for the same limb values — 14/14 exact, including 0, -0 → "0", 2^64, 2^128 and 2^128+1 (internal zero limb), -2^128-1, 10^19, 10^19+1 and a 4-limb negative. generated.rs matches a fresh npm run ci-update with an empty git diff, so no hand edits. Deleting nanvm-lib/todo/bigint-decimal-string-coercion.md is right — it described exactly this work.

New finding: the zero-padding of non-leading decimal groups is not pinned. Mutating {group:019} → {group} leaves cargo test at exit 0, 110/1/5 all passing — SURVIVED. It is a genuine defect, not a harmless one: under that mutant 10^19 prints "10". The control mutant, deleting f.write_char('-')?, is KILLED (exit 101, 1 failed), so the harness does bite — the gap is that every value in bigint_display_format (2^64, u128::MAX) happens to have full 19-digit non-leading groups. One case whose second group has leading zeros closes it, e.g. normalize_new(Positive, [10_000_000_000_000_000_000]) → "10000000000000000000".

Still open from before:

• changelog/unreleased/1709.md is missing. The body's Changelog: section is present and correctly shaped, so only the file is owed.
• fjs/nanvm/README.md:84-91 "Known divergence" outlives what it documents: its link target is the todo file this PR deletes (dead link), the "0x7Bn" claim is what the PR fixes, and "the two affected cases carry a rust reason" is now false — both rust: fields are gone from module.f.mjs and both check::<A> calls are uncommented in generated.rs. Delete the section.

Not ready to leave draft yet: behaviour is correct, but the padding is untested and the two chores above have not moved.

Re-measured at 61bddaab5. The delta since 975745950 is a merge of main and nothing else — git log --oneline --no-merges 61bddaab5 --not origin/main is still the single commit fb01c968f, and git diff --stat 975745950 61bddaab5 -- fjs/nanvm nanvm-lib changelog/unreleased/1709.md is empty. So all three findings stand unchanged; I re-ran the padding mutant rather than assuming, and {group:019} → {group} at nanvm-lib/src/vm/bigint/display.rs:37 still leaves cargo test at exit 0 with 110 + 1 + 5 passing — SURVIVED.

One new data point: dropping .rev() from for group in groups.iter().rev() is KILLED (exit 101, bigint_display_format panics at main.rs:126). Group ordering is pinned; only the padding is not.

Gates remain green on both sides — cargo test 110+1+5, build, fmt --check, clippy all exit 0, npm test 3465/0.

Still not ready to leave draft: the padding case, changelog/unreleased/1709.md, and the fjs/nanvm/README.md:84-91 removal.