nanvm: document operator test follow-ups - #1507

Confirmed documentation-only: the diff against the merge base is two new files under
nanvm-lib/todo/, +156/-0, nothing under fjs/nanvm/ or nanvm-lib/tests/test/generated.rs.
So no CHANGELOG entry (AGENTS.md §8.3), and I did not run the generator round-trip or the Rust
gates as gates on this change — but I did use them to test the proposal in the second file, and
one of the two TODOs prescribes something that does not compile.

operator-test-operation-model.md — accurate

Every claim checks out against the tree:

• fjs/nanvm/types.ts:66 is verbatim export type Op = 'unaryPlus' | 'unaryMinus' | 'mul' | 'stringCoercion'.
• Case.args is readonly Value[], so nothing ties arity to the case — correct.
• commutative?: boolean is indeed a general optional property on Group, not restricted to binary.
• Tuple<N, T> exists (fjs/types/array/types.ts) and is already used this way in fjs/crypto/sha2 and fjs/crypto/sign.
• Eq does carry shared for reference identity, so keeping it out of scope is well-founded.
• The snakeCase warning is real: fjs/nanvm/rust/module.f.mjs:132 builds function names as `fn ${snakeCase(g.op)}<A: IVm>() {` (also lines 145, 163), which would produce nonsense from + / *.
• All three #1489 review links resolve, and their bodies match what the TODOs say they asked for.

generated-rust-module-rustfmt-skip.md — the proposed attribute does not compile

The proposal is to emit #![rustfmt::skip] once at the top of the generated module. I applied it
exactly — added the inner attribute, deleted the five per-function #[rustfmt::skip] — and:

error[E0658]: custom inner attributes are unstable
 --> nanvm-lib/tests/test/generated.rs:4:4
  |
4 | #![rustfmt::skip]
  |    ^^^^^^^^^^^^^
  = note: see issue #54726 <https://github.com/rust-lang/rust/issues/54726>

cargo check --tests fails on stable. Tool attributes in inner position are still unstable, so
the shape this TODO specifies is not implementable as written.

I also confirmed the per-function skips are load-bearing rather than decorative: with them
removed and no replacement, cargo fmt -- --check rewraps every check_eq::<A>(…) call onto six
lines, destroying the one-statement-per-case layout.

A working form of the same idea exists. Put a single outer attribute on the module
declaration in the hand-written nanvm-lib/tests/test/main.rs:

#[rustfmt::skip]
mod generated;

With that and no per-function attributes, cargo fmt -- --check exits 0 with an empty diff and
cargo check --tests passes — one skip for the whole module, which is what #1489's review
comment asked for. The caveat worth writing into the TODO: that line lives in main.rs, not in
generated.rs, so it is not something fjs/nanvm/rust/module.f.mjs emits — the task changes
from "emit one inner attribute" to "add one attribute to the mod declaration, once, by hand,
and stop emitting the per-function ones".

Nit

"currently emits #[rustfmt::skip] before every generated function" — it is 5 of the 6 functions;
pub fn all<A: IVm>() at generated.rs:204 has none. The Proposal section gets this right by
naming eq and the per-operation functions. Under a module-wide skip all would become skipped
too, which is fine but is a small behaviour change worth a line.

Draft status is not a blocker for me. Both files are well-shaped, match the sibling
nanvm-lib/todo/*.md format, and the link check is byte-identical to origin/main; it is only
the #![rustfmt::skip] spelling that needs to change before someone tries to implement it.

Re-reviewed at 89b55cb. My previous review was at f0e687a3; the head moved twice while I
was working, so to be explicit about what I tested: the Rust experiments below were run at
0618447, and generated-rust-module-rustfmt-skip.md is byte-identical between 0618447
and 89b55cb (the only file changed in that step is operator-test-operation-model.md),
so they apply unchanged. Everything about the operator-model doc was checked at 89b55cb.

Still docs-only — git diff <merge-base> HEAD --stat is two files under nanvm-lib/todo/,
so §8.3 means no CHANGELOG entry, and no Rust gate or generator round-trip is owed by the
diff itself. I ran the Rust toolchain anyway, because the doc's central claim is a claim
about what the Rust toolchain does.

Both prior findings are resolved. One new issue in the operator-model doc, below.

1. #![rustfmt::skip] — resolved, and the replacement actually works

Last time the proposal prescribed an inner #![rustfmt::skip], which does not compile on
stable. The doc now says so itself and prescribes #[rustfmt::skip] on mod generated; in
the hand-written main.rs. I applied the new proposal exactly and it holds up
(rustc/cargo 1.95.0, rustfmt 1.9.0-stable, with RUSTC_WRAPPER= RUSTC=$(rustup which rustc)):

• Baseline, unmodified head: cargo fmt -- --check clean.
• Negative control — strip all 5 per-function #[rustfmt::skip] from generated.rs and add
  nothing: cargo fmt -- --check fails. So the doc's added sentence "The per-function
  skips are still necessary today" is correct, and the negative control means the pass below
  is meaningful rather than vacuous.
• Proposal applied — #[rustfmt::skip] above mod generated; in
  nanvm-lib/tests/test/main.rs, per-function skips removed:
  cargo fmt -- --check → clean; cargo check --tests → clean.
• Old proposal re-checked — #![rustfmt::skip] prepended to generated.rs:
  error[E0658]: custom inner attributes are unstable → could not compile 'nanvm-lib' (test "test"). The doc's new sentence about cargo check --tests failing is exactly right,
  and adding cargo check --tests to the task list is the right guard.

Tree restored; git status --porcelain clean.

2. The all nit — resolved

grep -n 'rustfmt::skip' nanvm-lib/tests/test/generated.rs gives 5 hits, on eq,
unary_plus, unary_minus, mul and string_coercion; pub fn all at line 204 has none.
The doc now states precisely that and explains that the module-wide skip covering all is
intentional. Accurate.

Supporting checks: nanvm-lib/tests/test/main.rs does declare mod generated;;
fjs/nanvm/rust/module.f.mjs emits the attribute at two sites (lines 131, 142) and
fjs/nanvm/rust/proof.f.mjs pins it in expected output at three places — so "Update the
Rust generator proof if its expected output covers these attributes" is not hypothetical, it
is required. The module doc comment at fjs/nanvm/rust/module.f.mjs:13 also says "Every
emitted function carries #[rustfmt::skip]", which is already inaccurate for all; the
comment task covers it.

3. New: dropping the stored case name cannot preserve unique proof keys

The new section says "Do not store a name on each case. The inputs, operation, and expected
result already contain enough information to generate a useful semantic diagnostic", and then
"FunctionalScript can use this generated expression directly as the emergent-test proof key."

The diagnostic half is fine. The proof key half is falsified by the current corpus.

fjs/nanvm/proof.f.mjs builds a group's leaves with fromEntries(ok) — leaves are object
properties keyed by name, so equal keys silently collapse and a test disappears without any
error. Uniqueness across argument orders is provided today by orders, which emits
[c.name, c.args] and [${c.name}Swapped, c.args.toReversed()].

For a commutative group with symmetric arguments, the case and its swap have identical
operation, arguments and expected result, so no renderer of those three inputs can tell them
apart. mul is the only commutative group and it has 7 such cases:

mul commutative=true cases=35 symmetric-arg=7
  nullByNull, zeroByZero, oneByOne, tenByTen, bigZeroByZero, bigOneByOne, bigTenByTen

(derived by importing fjs/nanvm/module.f.mjs and filtering Object.is(c.args[0], c.args[1]))

Under the proposed scheme each of those 7 pairs renders to one string, fromEntries keeps
one, and 7 leaves vanish silently. The coverage lost is arguably redundant — swapping equal
arguments is the same test — but that should be a stated decision, not an accident of object
construction. The Rust side is unaffected: check::<A>("nullByNull", …) and
check::<A>("nullByNullSwapped", …) are two statements, so there the name is purely a
diagnostic string and swapping it for a rendered expression is a clean drop-in. It is only
the FunctionalScript proof key that needs uniqueness.

A second, milder instance the doc half-anticipates: stringCoercion has zero (args: [0],
expected '0') and negativeZero (args: [-0], expected '0'). A renderer that prints -0
via ordinary string conversion produces String(0) === '0' for both — same collision, plus a
description that is untrue. The doc's Object.is caveat is aimed at the comparison form;
it should cover argument rendering too.

Suggested resolution, in doc terms rather than code: either say that commutative groups emit
one leaf when the arguments are equal (and that the corpus's 7 such mul cases are expected
to drop from the proof), or keep an explicit disambiguating suffix for the swapped order.
Either is fine; leaving it implicit is what I would not do, because fromEntries fails
silently.

Everything else in the operator-model doc checks out: Op is the string union at
fjs/nanvm/types.ts:66; Tuple<N, T> really exists at fjs/types/array/types.ts:21;
Case currently carries name/args/expected/rust? and Group carries commutative?;
Eq's shared-reference requirement is real (the ref variant of Info resolves against
data.eq.shared), so scoping it out is justified; and the claim that generated cases are
statements inside generic group functions rather than individual #[test] functions is
correct — the only #[test] in the crate's test binary is fn test() in main.rs. The
tuple shape is also consistent with the file's existing Info type, which already uses
readonly [...].

One nit on rationale: "not an object with independently mutable fields" is not quite the
right argument against the shape it replaced — that shape had readonly name / readonly argsN, so it was not mutable either. The compactness argument stands on its own.

What I ran

cargo fmt -- --check, cargo check --tests in four configurations (baseline, negative
control, new proposal, old proposal); grep/git ls-files surveys of the generator, proof
and generated Rust; a corpus import to count symmetric commutative cases; link check against
origin/main at 76dff875 — broken-link sets identical, not just counts. No cargo test
/ clippy and no generator round-trip: no code file is touched by this PR, so those gates
would be measuring main.

Commenting rather than approving on the strength of §3 alone — it is a one-paragraph fix to
the doc, and the rest of the PR is in good shape.

Re-reviewed at 2f72627. The head moved only by merging main — git diff 89b55cb..2f72627 touches one file, todo/migrate-typescript-to-mjs.md, and that arrives via 3bc32ab (#1506). The PR's own contribution against the merge base is unchanged and still docs-only:

$ git diff --stat $(git merge-base origin/main HEAD) HEAD
 nanvm-lib/todo/generated-rust-module-rustfmt-skip.md |  71 ++++++++++
 nanvm-lib/todo/operator-test-operation-model.md      | 140 +++++++++++++++++++
 2 files changed, 211 insertions(+)

Nothing under fjs/nanvm/ or nanvm-lib/tests/test/generated.rs is touched, so I did not run the Rust gates or the generator round-trip — they have no input to act on here. Per §8.3, todo/ docs take no CHANGELOG entry, so none is expected. Link check is clean: node bin/linkcheck.mjs produces a byte-identical report on this head and on origin/main (same broken-link set, not just the same count), and both new files are self-contained — nothing else in the tree references them, and they reference nothing that does not exist.

The rustfmt items are settled. I re-confirmed the doc's factual claims against the tree: fjs/nanvm/rust/module.f.mjs emits #[rustfmt::skip] at exactly two sites (lines 131 and 142), producing 5 occurrences in generated.rs — before eq, unary_plus, unary_minus, mul, string_coercion — and pub fn all at line 204 carries none, exactly as the doc now states and justifies. The #[rustfmt::skip] mod generated; replacement was verified in the previous round. Nothing further from me on that file.

1. The expression-as-proof-key proposal still loses 7 mul leaves (unchanged, still open)

operator-test-operation-model.md line 80 and task line 118 still say the generated semantic expression is used directly as the FunctionalScript proof key, while the same section removes the stored case name (line 65, task line 114). proof.f.mjs builds each group with fromEntries (lines 105–111), so leaf keys are last-write-wins.

For a commutative group the key would have to distinguish a case from its own swap, and for a symmetric-argument case no expression renderer can: 2 * 2 === 4 is its own mirror image. I enumerated the current corpus with a type-faithful literal renderer (bigints as 1n, strings quoted, -0 printed as -0) across all four groups:

unaryPlus       leaves 26  distinct 26
unaryMinus      leaves 27  distinct 27
mul             leaves 70  distinct 63   <-- 7 lost
stringCoercion  leaves 21  distinct 21

The seven that collapse are nullByNull, zeroByZero, oneByOne, tenByTen, bigZeroByZero, bigOneByOne, bigTenByTen — every case in mulCases whose two arguments are the same value. mul is the only group with commutative: true, so it is the only group affected today, but the loss is silent: 70 leaves become 63 with no error, and a future symmetric case added to any commutative group would vanish the same way.

The fix is small and belongs in the doc, not left to the implementer to discover: either keep an explicit disambiguator for the swapped order (the current ${name}Swapped suffix works), or state that a commutative group emits the swapped leaf only when the arguments differ — which is defensible, since swapping identical operands re-runs the identical computation — and make that a deliberate decision rather than a fromEntries accident. Worth a sentence either way.

Correction to my previous round. I earlier called stringCoercion's zero / negativeZero a milder second instance of the same problem. That was imprecise, and re-deriving it against the data gives a better answer. With a type-faithful literal renderer all 21 stringCoercion keys are distinct — including String(0) vs String(-0). The collisions only appear if operands are interpolated through String() rather than printed as literals, and then there are four, not one:

String(123) === "123"                       <- number + bigint
String(-456) === "-456"                      <- negativeNumber + negativeBigint
String(0) === "0"                            <- zero + negativeZero
String([object Object]) === "[object Object]" <- emptyObject + object

So this one is a renderer requirement, not an unavoidable collision: the doc's existing +0n throws example already implies literal rendering, and saying so explicitly ("operands are printed as source literals, so 123 and 123n are distinguishable") would close it. The mul case above is the one that no renderer can fix.

2. Nit: the "independently mutable fields" rationale (unchanged)

Line 51: "an operation is just a semantic descriptor, not an object with independently mutable fields." The shape being replaced has no mutable fields — types.ts line 93 declares Group with readonly op: Op, and every field of every type in that file is readonly. The object alternative to readonly [name, argsN] would equally be { readonly name: string, readonly argsN: N }. Mutability is not the axis on which the tuple wins; compactness and positional destructuring in Case<O[1]> are, and the sentence already says that. Dropping the "independently mutable fields" clause would leave the rationale accurate.

Both points are on prose in an open P3 planning document — neither blocks anything, and the rest of both files checks out against the tree (Tuple<N, T> does exist at fjs/types/array/types.ts:21; snakeCase(g.op) is applied at rust/module.f.mjs:132 and :163, so the warning about not snake-casing + is well-founded; commutative is indeed set only on the one binary group). Commenting rather than approving only because #1 changes what the implementer should build.

Approving. Both items I left open at 2f72627 are resolved, and I re-derived the replacement rather than assuming the fix works.

Finding (proof-key collisions) — resolved, and resolved correctly

At 2f72627 the doc said «Do not store a name on each case» and «FunctionalScript can use this generated expression directly as the emergent-test proof key». I measured that mul has 70 leaves but only 63 distinct generated expressions — exactly 7 collide (nullByNull, zeroByZero, oneByOne, tenByTen, bigZeroByZero, bigOneByOne, bigTenByTen), because for a commutative case with equal arguments the swapped form renders identically.

The doc now keeps name and states the reason in general form (a commutative case with equal arguments is indistinguishable from its swapped form), which is a better statement of the defect than the enumeration I gave.

I re-derived the replacement scheme rather than trusting it. Keys under the retained name + Swapped scheme, computed by walking the live proof export at this head:

leaf functions: 181   distinct paths: 181
mul leaves: 70        distinct: 70      (of which 35 are `…Swapped`)

No collisions anywhere, and mul's 70 leaves stay 70. The doc's description of the mechanism is also accurate against the code: proof.f.mjs derives [c.name, args] / [${c.name}Swapped, args.toReversed()], and the collapse it warns about would indeed happen through Object.fromEntries (used at proof.f.mjs:110, :111, :129).

Semantic expressions are now demoted to supplemental diagnostics with the faithful-literal requirement (123 vs 123n, 0 vs -0) spelled out, which addresses the same failure mode for the diagnostic path without making it load-bearing. Good.

Nit (Group/Case mutability) — resolved

The sentence «not an object with independently mutable fields» is gone; the replacement («makes the arity available directly as O[1] for types such as Case<O[1]>») states a real benefit and no longer mischaracterises types that are readonly throughout.

Claims re-checked against the tree

Every quoted or asserted fact in operator-test-operation-model.md holds at this head:

• fjs/nanvm/types.ts:66 is verbatim export type Op = 'unaryPlus' | 'unaryMinus' | 'mul' | 'stringCoercion'.
• Case (types.ts:80) is exactly { name, args: readonly Value[], expected, rust? } — so the proposed Case<N> differs from today only in args, as described.
• Tuple<N, T> exists at fjs/types/array/types.ts:21.
• commutative?: boolean really is a general optional property on Group (types.ts:95), so «restrict to binary groups» is a live task.
• The snakeCase-on-punctuation warning is well-founded: rust/module.f.mjs builds function names as snakeCase(g.op) (:132, :163), which would be applied to '+' under the new model.
• All four cited paths exist. #1489 is «nanvm-lib: one source of truth for operator tests», merged — it is the PR that introduced the corpus.

Spot-checked generated-rust-module-rustfmt-skip.md too (unchanged since my last review): nanvm-lib/tests/test/main.rs:9 is a bare mod generated; with no attribute, the generator emits #[rustfmt::skip] at two sites, and pub fn all<A: IVm>() in generated.rs indeed carries none — which is what the doc says.

node bin/linkcheck.mjs output is byte-identical between this head and origin/main (162 lines, same broken-link set), so no stranded references. Docs-only, so no CHANGELOG entry is required per §8.3, and none is present.

Optional, take it or leave it: since the Swapped twin is derived for every commutative case, the 7 equal-argument mul cases each get a twin that re-runs identical work under a distinct key. Not a correctness problem — and deriving unconditionally is simpler than special-casing — but if you ever want the count tightened, that is where the 7 redundant leaves live.