tsify icon indicating copy to clipboard operation
tsify copied to clipboard

Support for `RefFromWasmAbi` and `RefMutFromWasmAbi`

Open Alfred-Mountfield opened this issue 2 years ago • 7 comments

I've just tried out this crate and the typescript interfaces it's creating are awesome. Unfortunately I don't seem to be able to use it in methods like

#[wasm_bindgen]
modify_some_object(my_wasm_obj: &mut MyWasmObj); 

as tsify(into_wasm_abi, from_wasm_abi) doesn't implement support for RefFromWasmAbi and RefMutFromWasmAbi

Is there any plan to support deriving impl's for these?

Alfred-Mountfield avatar Aug 18 '22 16:08 Alfred-Mountfield

It is possible to implement RefFromWasmAbi and RefMutFromWasmAbi, but I think this expression is misleading. Currently, tsify(into_wasm_abi, from_wasm_abi) merely converts to JSON at a cost.

What do you think?

madonoharu avatar Aug 28 '22 11:08 madonoharu

I could see the concern about hiding the cost, and I think you're probably right that this isn't worth it. I was still learning a fair bit about wasm_bindgen as I opened this issue, and I was running into some strange behaviour where passing by value would cause null-pointer errors in JavaScript due to move mechanics (I assume). As such I thought it was pretty necessary to get the Ref* traits as passing by reference avoided that issue.

I also was reading something about how passing by value can cause issues with garbage collection if things are build with --weak-refs. I'm not sure if that would apply here though because as you say, the impl wraps with a JSON conversion.

I could still see a use for the RefMut impl, but I'm not entirely sure what the actual implementation would look like. The main problem I've found is that you can't create a function (from what I can tell) that modifies a JS object using this library. I think this should be possible with the js_sys methods while still leaving the memory inside JavaScript and not needing to take it into wasm-land.

Alfred-Mountfield avatar Aug 29 '22 09:08 Alfred-Mountfield

As a premise, I do not have a good understanding of wasm_bindgen. And my English skill is poor. Therefore, please forgive me if I am wrong.

The Tsify trait implemented by derive(Tsify) holds the type Tsify::JsType. This type is just a duck-type of wasm_bindgen. This duck-type generated by wasm_bindgen implements IntoWasmAbi, FromWasmAbi and RefFromWasmAbi.

Therefore, this description is possible.

#[derive(Tsify)]
pub struct MyWasmObj;

#[wasm_bindgen]
pub fn modify_some_object(my_wasm_obj: &<MyWasmObj as Tsify>::JsType) {
    js_sys::Reflect::set(my_wasm_obj, &"property_key".into(), &"value".into()).unwrap();
}

In this case, of course, no json conversion occurs and not even MyWasmObj is used.

It may not be impossible to combine this behavior with RefFromWasmAbi for advanced abstraction, but I think #[wasm_bindgen] pub struct Foo is much easier than that.

madonoharu avatar Aug 29 '22 18:08 madonoharu

You make a good point, I'm not sure it'd be worth the attempt to create that abstraction, it might hide a lot of complexity.

#[wasm_bindgen] 
pub struct Foo {
...

means that the memory of the object lives within WASM and you can access it via methods. This is mostly fine for things like classes or things with functionality, but I'm actually using tsify because I specifically want plain JS objects (no methods or anything). wasm_bindgen generated classes don't play particularly nicely (to my knowledge) with things like normal JSON.stringify methods (although there are some work arounds).

So it would have been nice if there was an easy way to get plain (but typed) JS objects, that you can also easily mutate from Rust. Tsify seems to have gotten me most of the way there :)

Alfred-Mountfield avatar Aug 30 '22 09:08 Alfred-Mountfield

Come to think of it, implementing RefFromWasmAbi may not be so problematic because of the lifetime limitation.

It seems to me that RefMutFromWasmAbi can be implemented by using Object.assign only for Object. However, I do not know if this is the correct and safe way to do this...

struct TsifyRefMut<T: Serialize + DeserializeOwned> {
    js: ManuallyDrop<JsValue>,
    owner: T,
}

impl<T: Serialize + DeserializeOwned> TsifyRefMut<T> {
    unsafe fn new(js: <JsValue as RefFromWasmAbi>::Abi) -> Self {
        let js = JsValue::ref_from_abi(js);
        let owner: T = js.into_serde().unwrap();
        Self { js, owner }
    }
}

impl<T: Serialize + DeserializeOwned> Drop for TsifyRefMut<T> {
    fn drop(&mut self) {
        let target = js_sys::Object::try_from(&self.js).unwrap();
        let output = JsValue::from_serde(&self.owner).unwrap();
        let source = js_sys::Object::try_from(&output).unwrap();
        js_sys::Object::assign(target, source);
    }
}

impl RefMutFromWasmAbi for Foo {
    type Abi = <JsValue as RefFromWasmAbi>::Abi;
    type Anchor = TsifyRefMut<Self>;
    unsafe fn ref_mut_from_abi(js: Self::Abi) -> Self::Anchor {
        TsifyRefMut::new(js)
    }
}

impl<T: Serialize + DeserializeOwned> Deref for TsifyRefMut<T> {
    ...
}

impl<T: Serialize + DeserializeOwned> DerefMut for TsifyRefMut<T> {
    ...
}

madonoharu avatar Aug 30 '22 11:08 madonoharu

I'm not sure if I have the appropriate knowledge here to comment on if it's safe or not :(

Reading through the code does make sense to me though.

Alfred-Mountfield avatar Aug 31 '22 08:08 Alfred-Mountfield

This definitely should be possible to implement soundly because of that lifetime limitation.

While yes there wouldn't be any benefit to calling the & version on wasm, there basically never is. However if you're calling the same function from rust in any situation, being able to declare them with a reference would be very useful.

cwfitzgerald avatar Jul 19 '23 02:07 cwfitzgerald