Skip to content

Name types for the descriptor, without changing what it says yet - #129

Open
TimoLehnertz wants to merge 1 commit into
madonoharu:mainfrom
TimoLehnertz:name-plumbing
Open

TimoLehnertz wants to merge 1 commit into
madonoharu:mainfrom
TimoLehnertz:name-plumbing

Conversation

@TimoLehnertz

Copy link
Copy Markdown
Contributor

Half A of the split you suggested. Nothing here reaches a .d.ts: it is the carrier for #76 and a set of names for the types the declaration path already renders, so it can be read for whether the protocol is right rather than for output diffs. B is stacked on top at #109.

The config is the root's, not the type's own. Taking #125 as the contract: one serializer, built from the root type's attributes, writes the whole value, so every impl threads CONFIG down unchanged and a nested HashMap is named the way the root writes it. My earlier suggestion — switching to the lexical inner config — would have made that mismatch type-check, which is what you were guarding against.

Encoding it as a const-generic parameter is what keeps NAME_LEN a compile-time constant; reading it from an associated const in a bound needs generic_const_exprs. The bits are a protocol between the two crates, not an API: unknown ones are ignored, so a newer macro crate names a type the same way an older library does rather than failing to compile.

Two rules the descriptor imposes, both measured at 0.2.104. Every character must arrive as an immediate — and it is not only &str: for c in ['a','b'] walks a materialized array, informs nothing, and fails as a truncated descriptor. And no loops, since the interpreter only learned loop/if/branches in 0.2.126. Conditions on a const fold at every profile, so [T; N] is unrolled behind them and makes the same split at sixteen the declaration makes. Verified at =0.2.104 with the loop kept as a control, which reproduced your unknown instruction Loop; [T; 0], [T; 1], [T; 2], [T; 16], [T; 17] all come out right in dev and release. No MSRV bump needed.

Result, tuples, Duration, SystemTime and the ranges are here because a type argument that stops compiling would be worse than the bug. tests/name.rs pins every length against the name it spells — worth doing: it caught three wrong constants I had written by hand.

#[diagnostic::on_unimplemented] is dropped and both manifests are untouched, as you asked. Everything is behind #[doc(hidden)] pub mod __macro_support; no public trait, no ts_name!.

Not covered: ByteBuf, which cannot be named without an optional serde_bytes dependency. Tell me if you want that added.

All e2e references are byte-identical. Both feature sets, fmt and clippy clean.

Nothing here reaches a `.d.ts`. It is the carrier for madonoharu#76: a way for
generated code to spell a type's TypeScript name one character at a time,
and a set of names for the types the declaration path already renders. The
switch that makes `describe` use it is separate, so this half can be read
for whether the protocol is right rather than for output diffs.

wasm-bindgen takes the type in a signature from a descriptor the module
informs at build time, not from the `typescript_custom_section`. The
`extern` type tsify declares carries one fixed name for all of its uses, so
it can only ever say `Response`, never `Response<UserInfo>`. `TsName`
rebuilds the name per monomorphization instead.

**The config is the root's, not the type's own.** One
`serde_wasm_bindgen::Serializer`, built from the root type's attributes,
writes the whole value; deriving `Tsify` for a nested type installs nothing
when that type is reached through an outer one (madonoharu#125). So `CONFIG` is
threaded down unchanged by every impl, and a nested `HashMap` is named the
way the root writes it rather than the way its own container declares it.
Naming it lexically would have made that mismatch type-check.

Encoding it as a const generic parameter is what keeps `NAME_LEN` a
compile-time constant. The alternative -- reading the config from an
associated const in a bound -- needs `generic_const_exprs`. The bits are a
protocol between the two crates rather than an API: unknown ones are
ignored, so a newer macro crate names a type the same way an older library
does instead of failing to compile.

**Two rules the descriptor imposes.** Every character has to arrive as an
immediate: the interpreter mirrors the size of linear memory but leaves it
zero-filled, so anything read from memory arrives as a NUL -- not only a
`&str`, but `for c in ['a', 'b']`, which walks a materialized array and
informs nothing at all. And no loops: the interpreter only learned `loop`,
`if`/`else` and branches in wasm-bindgen 0.2.126, and this crate supports
0.2.104, where a loop fails with `unknown instruction Loop` in dev builds
and is folded away in release ones. Conditions on a `const` fold at every
profile, so `[T; N]` is unrolled behind them to make the same split at
sixteen elements that the declaration path makes.

A length that disagrees with its characters does not truncate a name, it
desynchronizes the rest of the descriptor -- so `tests/name.rs` pins every
length against the name it is meant to spell. Writing the impls by hand was
worth testing: it caught three wrong constants.

`Result`, tuples, `Duration`, `SystemTime` and the ranges are here because
the declaration path already renders them, and a type argument that stops
compiling would be a worse regression than the bug being fixed. `ByteBuf` is
the one gap, since naming it needs a dependency on `serde_bytes`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@madonoharu

Copy link
Copy Markdown
Owner

First: thank you for turning this around so quickly. This revision handles the
A/B split, root-config propagation, the version-floor constraint and the
visibility boundary I asked for, and the overall design is right.

You adopted #125 as the contract, replacing your earlier — and reasonable —
proposal once the measurement pointed the other way. That was the right call,
and it is the part I would least want lost in what follows.

I independently reproduced the array result at =0.2.104: a probe of the same
shape, a const-generic trip count behind if N > k, comes out right in dev and
in release, where the loop version fails with unknown instruction Loop. So no
version bump, confirmed. I will settle the remaining floor separately, as I
said.

Then I wired the rest of TsName into a descriptor and ran that at =0.2.104.
That uncovered one blocking issue before B can land.

Four paths fail in dev builds at 0.2.104

describe does not call TsName yet, so nothing is broken today. It breaks the
moment B lands. One exported function per row, nothing else in the crate:

type dev release --no-opt
u32 number number
(u32, u32) <(u32, u32) as TsName>::describe_name: Condition failed: address % 4 == 0 (3 vs 0) [number, number]
Option<u32> describe_nullish: Condition failed: address % 4 == 0 (3 vs 0) (number | undefined)
HashMap<String, u32> unknown instruction Block(Block Map<string, number>
u64 unknown instruction Block(Block number

Two failure modes. HashMap and u64 reach a branch the 0.2.104 interpreter
cannot execute. The tuple and Option do not even get that far: the flag lives
on the shadow stack, and a byte store at an unaligned offset fails the
interpreter's alignment check first. So for those two, folding the branch is
not sufficient on its own.

The names are right in release, so the design holds. The remaining problem is
the code generation for three conditions at opt-level=0:

  • describe_nullish(config: u8) takes the config as an ordinary argument
    rather than a const parameter, so its if survives. () and Option<T> go
    through it.
  • ts_name_map! and ts_name_by_config! test C through a plain fn, which
    is not enough at opt-level=0.
  • ts_name_tuple! uses a let mut first flag, so there is no const to fold at
    all. The separator should be decided when the macro expands, the way
    expand_ts_name already does it with (i > 0).then(...).

ArrayName is the one that gets it right, and it does because N is compared
directly.

This does not need the carrier or the protocol reworked. For this PR I am
asking for two bounded changes: make those three sites fold in dev builds, and
pick a generated config parameter name that cannot collide with the type's own
generics — wasm_bindgen.rs appends const __TSIFY_CONFIG: u8 unconditionally,
so a type that already declares one by that name ends up with two of the same
name and stops compiling, having derived Tsify fine before.

The missing coverage is ours

tests-e2e/build_all.sh runs wasm-pack build with no profile, so every e2e
case is a release build. Our setup could not have caught any of those four rows,
and would not catch them after B either. I will add dev-build coverage
separately; I am not asking you to take that on in this PR.

Not asking for anything here

The protocol, the bit layout, NAME_LEN and the split all look right, and
tests/name.rs pinning the lengths against the spelled names is the check I
would have asked for if it were not already there.

No change is needed for ByteBuf in this PR. An optional serde_bytes
dependency for one name can be handled in a follow-up with its own rationale.

madonoharu added a commit that referenced this pull request Aug 30, 2026
wasm-bindgen evaluates descriptors with its own interpreter, and the one at
our declared minimum cannot execute control flow or unaligned byte access left
in a dev module. Release optimization folds both away; newer interpreters
accept both. So a descriptor can be wrong in a way that only a dev build at
0.2.104 will say out loud, and `tests-e2e/build_all.sh` builds every crate in
release.

Measured, with a probe that reaches four descriptor paths:

    =0.2.104  --dev                 4 of 4 fail
    =0.2.104  --release --no-opt    all pass
    0.2.127   --dev                 all pass

That last row is why this is not simply "also build in dev": at the version
Cargo resolves today the failures are gone, because 0.2.126 taught the
interpreter about branches and 0.2.127 relaxed the alignment check. The gate
has to be dev *and* pinned to the floor. Neither half is a test on its own.

`build_minimum_wasm_bindgen.sh` reads the floor out of the root manifest with
`cargo metadata` rather than repeating it, and accepts only a plain `^x.y.z`
requirement — if that ever becomes a range, the extraction fails instead of
quietly testing a different version. The crate it generates lives in a
temporary directory, so it neither joins the `tests-e2e/*` workspace glob nor
overwrites the `pkg/` output the release pass compares against.

The probe is four arguments through the public API rather than a snapshot to
diff. Nothing about it is asserted except that the build finishes: when a
descriptor is wrong at this version, wasm-pack fails and says which function.

It passes on `main` today. #129 is where it starts earning its keep — the four
types are the ones whose names that PR's descriptor code will evaluate once B
connects it.

`build_all.sh` now resolves its own directory instead of assuming the caller
is at the repository root, since it has a sibling script to find.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants