Wasm-bindgen: js-sys crate fails to mark many throwy methods with `catch`

[bakkot]

The js-sys crate has a comment which reads

// * If a function or method can throw an exception, make it catchable by adding
//   `#[wasm_bindgen(catch)]`.

And this is indeed used in some places throughout. But there are a great many methods which can throw and which are not marked with catch. For example, isFinite(Symbol()) throws in JS, but the implementation of isFinite in js-sys is not marked as throwing. Should it be marked with catch?

I can probably enumerate all methods which can throw but are not marked as throwing, but it'd be a long list.

[fitzgen]

In general, we don't mark things with catch that would only throw due to type errors that we can prevent at compile time, and we don't bother with "someone messed with the prototype chain" either.

It seems, though, that isFinite should be annotated with catch indeed!

I can probably enumerate all methods which can throw but are not marked as throwing, but it'd be a long list.

It would be helpful for us though!

---

Adding catch post-facto would be a breaking change. Alternatively, we could also add try_foo variants for each foo that can throw but wasn't originally annotated as such. @alexcrichton do you have an opinion on whether it is worth trying to avoid breaking backwards compat for js-sys?

[fitzgen]

Oh, and thanks for filing an issue @bakkot!

[bakkot]

we don't bother with "someone messed with the prototype chain" either

Can you also comment on whether you'd worry about these?

• new Uint8Array(-1)
• Object.getPrototypeOf(null)
• Array.from({ get length(){ throw 0; } })
• [{ toString(){ throw 0; } }].toString()
• Object.defineProperty([0], '0', { get: () => { throw 0; } }).forEach(() => {})

These are typical examples of the more common categories of possibly-throwing functions, I think.

[fitzgen]

new Uint8Array(-1)

Our Uint8Array::new that takes JsValue should be catch. The new_with_length should not since it takes an unsigned integer.

Object.getPrototypeOf(null)

Yes, we should have catch on this.

Array.from({ get length(){ throw 0; } })

Should be catch

[{ toString(){ throw 0; } }].toString()

Should be catch since it is expected that toString is customized and who knows if it is buggy and can throw

Object.defineProperty([0], '0', { get: () => { throw 0; } }).forEach(() => {})

I think this one falls into the "messing with prototypes common properties" category. Not something I think we need to support.

[bakkot]

Thanks! One more example: detached array buffers. Consider:

let buffer = new ArrayBuffer(10);
postMessage(null, '*', [buffer]); // web platform API
buffer.slice(0); // throws

Should ArrayBuffer's slice be marked with catch?

[fitzgen]

Should ArrayBuffer's slice be marked with catch?

Probably, yeah.

[bakkot]

OK, here's the omissions I noticed at a first glance. I may have missed some.

In each case I've listed one or more examples which cause the relevant API to throw.

---

is_finite

is_finite(Symbol())

---

Array.from

Array.from(null)

---

Array.prototype.join

[{ toString(){ throw 0; } }].join()

---

Array.prototype.toLocaleString

[{ toLocaleString(){ throw 0; } }].toLocaleString()

---

Array.prototype.toString

[{ toString(){ throw 0; } }].toString()

---

ArrayBuffer.prototype.slice

let buffer = new ArrayBuffer(10);
postMessage(null, '*', [buffer]);
buffer.slice(0);

---

ArrayBuffer.prototype.slice with length

let buffer = new ArrayBuffer(10);
postMessage(null, '*', [buffer]);
buffer.slice(0, 0);

---

DataView constructor

new DataView(new ArrayBuffer(10), 0, 20)

---

DataView.prototype.{get,set}_*

(new DataView(new ArrayBuffer(10), 0, 10)).getInt8(20);

(new DataView(new ArrayBuffer(10), 0, 10)).setInt8(20, 0);



md5-529fb3ce76ba374c8e92e7d11c6935e7



new Float32Array(-1);



md5-2c17fffd92b0cf7f7489e5483856311d



new Float32Array({ get length(){ throw 0; } });



md5-2c17fffd92b0cf7f7489e5483856311d



new Float32Array(Symbol());



md5-c07c79ce50560f1329ede970db63143c



new Float32Array(new ArrayBuffer(20), 1);



md5-2c17fffd92b0cf7f7489e5483856311d



new Float32Array(new ArrayBuffer(20), 40);



md5-c16620021bdcf9124e9b13271c7b2bb4



new Float32Array(new ArrayBuffer(20), 0, 10)



md5-d79893ae1a095cbd2de28c9e048eb5ea



let buffer = new ArrayBuffer(12);
let array = new Float32Array(buffer);
postMessage(null, '*', [buffer]);
array.fill(0, 0, 0);



md5-918051d4bcea890155557da56e31137b



(new Float32Array(10)).set([0], 20)



md5-d91a6cd3c18d82b0f2d9d14e238ec623



new Function('.')



md5-dd383c3d3370c73ac1d8f7f2cfd94056



new Number({ valueOf(){ throw 0; } })



md5-8c3492d10a1d0801c80c07e91e55a836



(new Number(0)).toLocaleString('not a language tag')



md5-35b61e6b8896226940082e467166ae26



new Date(Symbol())



md5-ab803c485efc3f6f7a53a815ed7df890



(new Date).toLocaleString('not a language tag')



md5-430d90aac0c4ba34f47c9bcd0dbe489d



Object.assign({}, { get x(){ throw 0; } })



md5-0d28c54330a4fedc44ffd7617239c629



Object.defineProperty({}, { toString(){ throw 0; } }, {} );



md5-2c17fffd92b0cf7f7489e5483856311d



Object.defineProperty({}, '', { get value(){ throw 0; } });



md5-2c17fffd92b0cf7f7489e5483856311d



Object.defineProperty({}, '', { get: 0 });



md5-3f37c8432421ea3b9092f31541b3dd53



Object.defineProperties({}, { get x(){ throw 0; }} );



md5-2c17fffd92b0cf7f7489e5483856311d



Object.defineProperties({}, { x: { get value(){ throw 0; } } });



md5-2c17fffd92b0cf7f7489e5483856311d



Object.defineProperties({}, { x: { get: 0 } });



md5-32ea08582402a5d2af0605a57811b8f8



Object.getOwnPropertyDescriptor({}, { toString(){ throw 0; } })



md5-c6086876aecfb02ef839c6795058a36e



Object.getPrototypeOf(null)



md5-8d87986e17745d2b761d2ce9b4b34f85



({}).hasOwnProperty({ toString(){ throw 0; } })



md5-3a4c5f02feada32687969cc9fbe87335



({ toString(){ throw 0; } }).toLocaleString()



md5-79da8904dcfaf85b7c362bba37229085



new Proxy(null, {})



md5-58c11c7bc68881f48ada095c5fb1ec2f



Proxy.revocable(null, {})



md5-7f9680c8fe970a05610195603b7a64e0



new RegExp('(', '')



md5-4f802a39cc8a47cc512897b8b42cc7cb



new Set(0)

[fitzgen]

Thanks for putting this list together @bakkot!

[najamelan]

This is open since Feb. Any progress on this? I run into this on the WebSocket type in web-sys. I'm implementing a rust idiomatic API over websockets, but methods like: send_with_str throw instead of returning an error (eg. when the connections is closed before trying to write). I need to return an error in a Sink impl rather than panicking.

It's unclear to me how I can correctly implement a rust library on top of this. Can I manually catch the exceptions for the time being? Or can we try to fix this, or should I patch web-sys and use a patched version in my dependencies (seems like a PITA to keep up to date)?

I wouldn't mind marking things with catch and filing pull requests if it's that simple, but at least there needs to be a decision whether we can have breaking changes if I interprete the above discussion correctly. I don't quite understand why no longer panicking when the documentation said it was going to return a Result is a breaking change though. Which code might that break? It just seems like bugs that need to be fixed, but surely I miss something here.

From what I can tell, the WEB IDL are correctly marked with [Throw], but the bindings generated from wasm-bindgen-webidl::compile do not have catch.

[alexcrichton]

@najamelan hm could you perhaps clarify a bit? This issue is for js-sys not web-sys, and historically web-sys has been in a different bucket of "too much is marked [Throw]", not too little. Also from looking at the documentation send_with_str looks like it's marked as catching exceptions.

If this issue is about an API in web-sys, mind opening a new issue as well?

[najamelan]

@alexcrichton It's marked as [Throw] in the web IDL, but the generated method is not marked with catch, and I saw it panic in real code. Maybe I'm not interpreting something right, but the file: target/build/websys-xxx/out/bindings.rs in my crate which looks to me like the generated code, does not even contain the word catch anywhere.

I will make a self contained test that shows the panic in action and publish that in a different issue.

[alexcrichton]

A smaller test case would indeed help figure out what's going on here!

[najamelan]

@alexcrichton I'm sorry, my bad. I got this error:
wasm-bindgen: imported JS function that was not marked ascatchthrew an error: Failed to write to websocket.

I hadn't seen that before, but I called expect on a result in rust, but since that is in an async block, I suppose it's in a javascript promise, so the error refers to a imported JS function.

Sorry for the confusion.