Repository navigation
Translate wasm-bindgen imports - #657
Conversation
Quest-Claim: 18ed39698efc8bb2
Read each import from the program statics `#[wasm_bindgen]` leaves in the crate, by schema 0.2.128 or 0.2.130, and write the JS call the CLI would. `JsValue` and imported types are the JS value itself; the boundary conversions, `catch`, statics, string enums, casts, and js-sys's intrinsics map to the runtime's `rs/js.ts`. Custom JS modules, `suspending`, `experimental_generic_mono`, closures, WASM-only intrinsics, and exports are refused at their spans. The `bindgen` fixture compares a harness of each kind of import against native Rust run through the CLI's glue, at both schemas. Also flushes effectful pending temps before a loop header, which fixes a call that was sunk into a loop in stock's hashlink and rangemap output. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…gen/imports # Conflicts: # ts/src/lower/library.rs # ts/src/lower/mod.rs
- A namespace's variadic function is applied on its namespace. - A variadic call spreads its last argument by its iterator. - A `catch` import catches what the JS glue's conversion throws, not the WebAssembly ABI's. - An optional `bool` reads by its truthiness. - A global no JS defines throws a `ReferenceError`. - Number results coerce through `ToNumber`, so a BigInt throws the ABI's error. - Only a struct the `#[wasm_bindgen]` attribute wrote is a JS value. - An optional mutable slice is refused, and so is 0.2.130's `Nullable`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… at the ABI A variadic call now reads what it is a method of once, then its function, and then spreads its arguments, as a spread call does. An `Option` of a `u8`, `i8`, `u16`, or `i16` crosses the ABI's `i32` as it is: a `catch` import doesn't catch its coercion, and `0xFFFFFF` is `None`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 37 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head 57655c6 against base 41ea814. The current head is seven commits ahead of that base; there was no prior submitted review to compare against.
Findings:
- P2: Preserve every Rust wrapper that shares an import shim. Identical declarations in separate Rust modules can have the same wasm-bindgen shim name, but the new map retains only one wrapper.
- P2: Preserve sparse-array formatting in JsValue Debug. The new Array.map implementation skips holes that wasm-bindgen's index-based implementation formats as undefined.
Direction: Address the two inline findings and add differential cases for duplicate imports and sparse arrays before merging. The encoded-program approach and whole-wrapper replacement otherwise fit the intended direction.
Scope and limits: GitHub-only static review of the decoder, import discovery and lowering, boundary/runtime conversions, related MIR/printer changes, and fixture/differential harness. Checked the relevant behavior against wasm-bindgen 0.2.129 source. No builds or tests were run, and this is not an approval or a claim that runtime behavior has been fully validated.
(Written by review (OpenAI))
| } | ||
| DefKind::Fn if tcx.is_foreign_item(did) && in_placeholder(tcx, did) => { | ||
| if let Some(wrapper) = enclosing_fn(tcx, did) { | ||
| shims.insert(tcx.item_name(did).to_string(), wrapper); |
There was a problem hiding this comment.
[P2] Keep all wrappers for a shared shim name
wasm-bindgen hashes a function's signature, JS namespace, module and cfg attributes, but not its enclosing Rust module. Two modules can therefore both declare the same #[wasm_bindgen(js_namespace = Math)] fn abs(x: f64) -> f64 and produce the same shim name. This insert overwrites the first wrapper, and the later shims.remove(&import.shim) registers only the surviving DefId. When translation reaches the other wrapper it falls through into the Wasm ABI glue rather than becoming the JS import, so an otherwise supported declaration is refused. Store all wrapper DefIds for each shim and apply the decoded import to each; add a differential fixture that calls identical declarations from two Rust modules. The upstream naming logic is in wasm-bindgen 0.2.129 crates/macro-support/src/parser.rs:1149-1182.
| const name = (val as { name: unknown }).name; | ||
| return typeof name === "string" && name.length > 0 ? `Function(${name})` : "Function"; | ||
| } | ||
| if (Array.isArray(val)) return `[${val.map(jsDebugString).join(", ")}]`; |
There was a problem hiding this comment.
[P2] Visit holes when formatting a JS array
Array.prototype.map skips missing elements, whereas wasm-bindgen's debugString explicitly reads every index. For a normal sparse array such as js_sys::Array::new_with_length(2), the native helper produces [undefined, undefined], but this branch produces [, ]. This changes the observable Debug text of a JsValue even though both values are supported. Port the index-based traversal (also avoiding calls to an array's overridden map) and add a sparse-array differential case. The matching upstream implementation is wasm-bindgen 0.2.129 crates/cli-support/src/js/mod.rs:7029-7039.
Finishes and deletes
quest/m3/ts-wasm-bindgen/imports.md. Its links in the epic's Required list and inclosures.mdare removed too.Problem
web-codecs, and any crate that calls the browser, reaches JS through
wasm-bindgen,js-sys, andweb-sys. Translation refused all of it. A translated crate had no way to call JS, even though the target already runs inside JS.Approach
__wasm_bindgen_unstableprogram statics that#[wasm_bindgen]leaves in the crate. These are the same statics the CLI reads.ts/src/lower/bindgen/decode.rsis vendored from the CLI's at 0.2.130, with fields gated by schema. It decodes schemas 0.2.128 (wasm-bindgen 0.2.129, pinned by js-sys 0.3.106) and 0.2.130 (pinned by js-sys 0.3.107).JsValue, and every type anextern "C"block declares, isrs.JsValue, which is TSunknown.objfield, the aggregate,Clone, the macro'sFrom/AsRef/Derefimpls, andJsCast's unchecked casts.Dropdoes nothing.LazyCellisrs.LazyCell.ts/runtime/rs/js.tsholds the boundary conversions, re-exported fromrs.ts:char, strings, typed arrays, and arrays;Optionthroughundefined, and string enums;catchinto aResult;instanceof, vendor prefixes, and js-sys's intrinsics.debug_stringis a verbatim port of the CLI's.module,raw_module,inline_js);suspendingandexperimental_generic_monoimports;Closure;memory());#[wasm_bindgen]exports.bindgenfixture (its own workspace) has 15 harness cases over imports of each kind: its own, js-sys's, and web-sys's (TextEncoder,TextDecoder,console).wasm-bindgen --target bundlerglue, with the CLI at 0.2.129 from the dev shell. Each WASM instance gets a fresh glue instance, because the glue caches its memory views.wasm-bindgen-0.2.130.lock. Both translations are compared with the same native run. Both schemas showed 0 differences.refused.tomlsnapshots the refusals.Impact
rs.tsgainsJsValueand thejs*helpers fromrs/js.ts.hashlinkandrangemapsnapshots, and those changes are fixes (see below).wasm-bindgen-cli_0_2_129.ts/README.mddocuments the mapping and the supported schemas under "Well-known crates", and the fixture under "Tests".Alternatives
macro_rules!are on web-codecs' path, and the encoded program is what the CLI itself trusts.Choices made without asking
globalThis. Lexicallet/classbindings at the top of a script are not seen; the CLI's ES-module glue sees them.thread_local_v2static is anrs.LazyCellthat is read once, as native code caches it.differential.tsandwasm.tsgain a "thrown" outcome. When glued, a JS error that neither side catches is compared by its message.module = "pkg"encodes as a raw module, so it is reported as "its JS modulepkg".Fx::nodenow flushes effectful pending temps before a loop header, so a call is no longer moved into the loop. This fixed real miscompilations in stock'shashlink(DropFilteredValues) andrangemap.loop_headerchecks reachability before it asks about dominance.newwhose callee contains a call.#[wasm_bindgen]attribute (a macro fromwasm_bindgen_macro, under any path) wrote itsobjfield. A crate's own#[repr(transparent)] struct { obj: JsValue }stays a struct and keeps itsDrop.ReferenceError("X is not defined"), which is V8's text; the differential runs onnode. Other engines word it differently.catchimport catches result conversions that the CLI's glue does (strings,char, vectors,Options, 128-bit integers). It does not catch the WebAssembly ABI's coercion of a plain number or of anOption<u8/i8/u16/i16>, which happens after the glue returns. AnOption<u8/i8/u16/i16>also takes0xFFFFFFasNone, as the ABI sentinel does.+first, so a BigInt throws the ABI'sToNumbererror text.Option, since only a top-level argument is written back.Nullable, since nixpkgs has no 0.2.130 CLI to compare its glue against.LazyCell, persists across inputs.Follow-ups
Suggested, not planned:
module,inline_js) by emitting the JS beside the output and importing it. Recommend drop until a consumer needs one; web-codecs doesn't.experimental_generic_monoby writing the JS per instance, as the CLI does. Recommend drop for now: it is experimental, and web-codecs doesn't use it.NullableandOption<Nullable<T>>once nixpkgs packageswasm-bindgen-cli0.2.130, compared against its glue. Recommend plan when a consumer's js-sys reaches 0.3.107.decode.rseach time js-sys bumps. Recommend plan once a consumer bumps past 0.2.130, rather than a recurring quest.closures.md.Models
(Written by Claude Opus 5.5 with high effort)
🤖 Generated with Claude Code
Review
Rewritten by
quest reviewafter each round.01a1b7ea-c984-75e0-89a6-3ba7380578a3, round 4The merge adds no new branch-specific implementation, and the snapshot update preserves the Rust loops’ behavior; no builds or tests were run.