rustwasm / wasm-bindgen

Facilitating high-level interactions between Wasm modules and JavaScript
https://rustwasm.github.io/docs/wasm-bindgen/
Apache License 2.0
7.84k stars 1.08k forks source link

Added type annotations for a few glue code functions #4189

Open RunDevelopment opened 1 month ago

RunDevelopment commented 1 month ago

I added type annotations to a few commonly-used JS glue code functions. This makes it a lot easier to understand the glue, because I have TS to help me out.

Those type annotations also make it easier to verify the correctness of glue. And just to prove that: TS found 2 minor bugs in the JS glue code of our tests:

Fixes moved to #4192.

1. ~`debugString` (this is the JS glue code version of `format!("{:?}", value)`) had a bug where it checked for a regex mismatch incorrectly. This would lead to dereferencing `null` at runtime, which throws a `TypeError`.~ 2. ~`_assertClass` returned `instance.ptr`. Here's the full code of the function:~ ```js function _assertClass(instance, klass) { if (!(instance instanceof klass)) { throw new Error(`expected instance of ${klass.name}`); } return instance.ptr; } ``` ~Returning `instance.ptr` is weird for 2 reasons:~ - ~It's not used anywhere in the code. `_assertClass` is only used in one place and its return value is ignored.~ - ~There is no `ptr` field. The author probably meant `__wbg_ptr` to get the internal pointer of a JS object.~ ~So I just removed the return statement and `instance.ptr`.~

Aside from that, I did one additional change: I inline lTextDecoder:

// before

const lTextDecoder = typeof TextDecoder === 'undefined' ? (0, module.require)('util').TextDecoder : TextDecoder;

let cachedTextDecoder = new lTextDecoder('utf-8', { ignoreBOM: true, fatal: true });

// after

/** @type {TextDecoder} */
let cachedTextDecoder = new (typeof TextDecoder === 'undefined' ? (0, module.require)('util').TextDecoder : TextDecoder)('utf-8', { ignoreBOM: true, fatal: true });

This just made typing easier, but the expression something to get used to...

daxpedda commented 1 month ago

This is amazing work by the way, thank you!

RunDevelopment commented 1 month ago

I think it would be good to split off the bugfixes you made into a separate PR.

Will do.

I would also appreciate if you could do some code archeology into instance.ptr and see where this originated and the context around it.

Seems like the pointer field changed names a few times. 240d3cd1a1e9c5a75e8e1ae5a48469d8f229633e (7 years ago) changed it from __wasmPtr to ptr and dbea2a29e1ddf14d01bc16d370828cce409d2d11 (1 year ago) changed it from ptr to __wbg_ptr. So it's not that the author of _assertClass used the wrong field name, the code of _assertClass just wasn't updated when the pointer field was changed to the current __wbg_ptr.

daxpedda commented 1 month ago

I also found https://github.com/rustwasm/wasm-bindgen/commit/5d697c196f46d8449b0654c326a6b70fb44b469d, which was the point where the return value stopped being used anymore but didn't get removed.