Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Implement JsValue.as_ptr and (bonus) JsValue.downcast_unchecked #3088

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Sep 19, 2022

This PR implements JsValue.as_ptr. The idea is to be able to
retrieve the ptr value of a JsValue from the JS land, which
represents the raw pointer value of the value from within the Wasm
memory.

To achieve this, this patch introduces a new intrinsic:
__wbindgen_jsval_ptr, which maps to Intrinsic::JsvalPtr, which
returns a (new) opt_u32. It generates the following JS code:
{obj}.ptr. If ptr is absent, it will return undefined, so None
once on the Rust side, otherwise Some(u32).

The following Rust code is the “old” way of achieving this behavior:

pub struct JsvalPtr;

let js_val: JsValue = JsvalPtr.into();
let ptr = Reflect::get(&js_val, &JsValue::from_str("ptr").unwrap().as_f64().unwrap() as u32;

The following Rust code is the "new” way of achieving this behavior:

let ptr = js_val.as_ptr().unwrap();

The Reflect API is no longer necessary.

Since the introduction of the new JsValue.as_ptr method, it is now
possible to implement a new handy method: JsValue.downcast_unchecked
(which is unsafe by nature because no check is applied).

downcast_unchecked will downcast the JsValue to T::Anchor where
it implements RefFromWasmAbi<Abi = u32>. It returns a Ref<T> which
can be deref to &T.

I reckon the test summarizes well the API:

assert_eq!(JsValue::from(42).as_ptr(), None);

#[wasm_bindgen]
#[derive(Debug, PartialEq)]
pub struct JsvalPtr {
    foo: u8,
}

let js_val = JsValue::from(JsvalPtr { foo: 42 });

let expected_ptr = {
    let ptr = Reflect::get(&js_val, &JsValue::from_str("ptr"))
        .expect("failed to read the `JsValue` pointer");

    ptr.as_f64()
        .expect("failed to read the `JsValue` pointer as a `f64`") as u32
};

assert_eq!(js_val.as_ptr(), Some(expected_ptr));

let downcast = unsafe { js_val.downcast_unchecked::<JsvalPtr>() };
let downcast = downcast.as_deref();

assert_eq!(downcast, Some(&JsvalPtr { foo: 42 }));

Thoughts?

@Hywan
Copy link
Contributor Author

Hywan commented Feb 22, 2023

ping @alexcrichton :-) ?

@daxpedda
Copy link
Collaborator

Could you describe your use-case for this feature?

I don't think I mind too much having the intrinsic returning the ptr, to at least save the call through Reflect, but JsValue::downcast_unchecked seems a bit overkill without a good use case, considering it's just a convenience function.

@daxpedda daxpedda added the waiting for author Waiting for author to respond label May 10, 2023
@daxpedda daxpedda self-assigned this May 10, 2023
@Hywan
Copy link
Contributor Author

Hywan commented May 11, 2023

@daxpedda My usecase is when I've an Array, and I want to downcast items contained in this array to Rust values. Other use cases exist, like with Set probably, and other collection types which hold opaque values.

Having at least the intrinsic returning the ptr is helpful to avoid calling Reflect (which can be slow). As said, downcast_unchecked is a bonus, I can implement it via custom trait on JsValue of course, but having it de facto on JsValue would be very helpful to convert opaque value to a Rust value. It would solve #2231 by the way.

@daxpedda
Copy link
Collaborator

That makes sense indeed. I think solving the issue around downcasting from JsValue to exported types needs further discussion, but just JsValue::as_ptr() does make sense to me, though I am surprised why into_abi() doesn't return what we want here.

I will get a third opinion on this and get back to you again.

@daxpedda daxpedda added needs discussion Requires further discussion and removed waiting for author Waiting for author to respond labels May 11, 2023
@Hywan
Copy link
Contributor Author

Hywan commented May 11, 2023

into_abi returns JsValue.idx which is different from the ptr. See #2898 (comment).

Also, this PR should be updated since the merge of #3408.

@daxpedda
Copy link
Collaborator

into_abi returns JsValue.idx which is different from the ptr. See #2898 (comment).

Ah! Thank you.

Also, this PR should be updated since the merge of #3408.

Yeah, if you rebase it, it should hopefully fail the test you added.

Hywan added 6 commits May 11, 2023 13:21
This patch implements `JsValue.as_ptr`. The idea is to be able to
retrieve the `ptr` value of a `JsValue` from the JS land, which
represents the raw pointer value of the value from within the Wasm
memory.

To achieve this, this patch introduces a new intrinsic:
`__wbindgen_jsval_ptr`, which maps to `Intrinsic::JsvalPtr`, which
returns a (new) `opt_u32`. It generates the following JS code:
`{obj}.ptr`. If `ptr` is absent, it will return `undefined`, so `None`
once on the Rust side, otherwise `Some(u32)`.

The following Rust code is the “old” way of achieving this behavior:

```rust
pub struct JsvalPtr;

let js_val: JsValue = JsvalPtr.into();
let ptr = Reflect::get(&js_val, &JsValue::from_str("ptr").unwrap().as_f64().unwrap() as u32;
```

The following Rust code is the "new” way of achieving this behavior:

```rust
let ptr = js_val.as_ptr().unwrap();
```

The `Reflect` API is not longer necessary.
Since the introduction of the new `JsValue.as_ptr` method, it is now
possible to implement a new handy method: `JsValue.downcast_unchecked`
(which is unsafe by nature because no check is applied).

`downcast_unchecked` will downcast the `JsValue` to `T::Anchor` where
it implements `RefFromWasmAbi<Abi = u32>`. It returns a `Ref<T>` which
can be deref to `&T`.
@Hywan Hywan force-pushed the feat-jsvalue-as-ptr-and-downcast-unchecked branch from da67411 to fff49af Compare May 11, 2023 11:43
@Hywan
Copy link
Contributor Author

Hywan commented May 11, 2023

Rebased :-).

@daxpedda daxpedda added needs review Needs a review by a maintainer. and removed needs discussion Requires further discussion labels May 11, 2023
@daxpedda daxpedda removed their assignment May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review Needs a review by a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants