Don't use reflect in webidl dictionary setters

[Davidster]

Implements solution 1 from #3468 (comment).

Solution 3 will come after assuming this goes smoothly

[Davidster]

woops, looks like the CI is failing. will check that tmrw.

[Davidster]

I've run into the following error with the codegen:

it generates some shim functions now such as:

#![allow(unused_imports)]
#![allow(clippy::all)]
use super::*;
use wasm_bindgen::prelude::*;
#[wasm_bindgen]
extern "C" {
    # [wasm_bindgen (extends = :: js_sys :: Object , js_name = RequestInit)]
    #[derive(Debug, Clone, PartialEq, Eq)]
    #[doc = "The `RequestInit` dictionary."]
    #[doc = ""]
    #[doc = "*This API requires the following crate features to be activated: `RequestInit`*"]
    pub type RequestInit;
    # [wasm_bindgen (method , setter = body)]
    fn body_shim(this: &RequestInit, val: Option<&::wasm_bindgen::JsValue>);
    #[cfg(feature = "RequestCache")]
    # [wasm_bindgen (method , setter = cache)]
    fn cache_shim(this: &RequestInit, val: RequestCache);
    #[cfg(feature = "RequestCredentials")]
    # [wasm_bindgen (method , setter = credentials)]
    fn credentials_shim(this: &RequestInit, val: RequestCredentials);
    # [wasm_bindgen (method , setter = headers)]
    fn headers_shim(this: &RequestInit, val: &::wasm_bindgen::JsValue);
    # [wasm_bindgen (method , setter = integrity)]
    fn integrity_shim(this: &RequestInit, val: &str);
    # [wasm_bindgen (method , setter = method)]
    fn method_shim(this: &RequestInit, val: &str);
    #[cfg(feature = "RequestMode")]
    # [wasm_bindgen (method , setter = mode)]
    fn mode_shim(this: &RequestInit, val: RequestMode);
    #[cfg(feature = "ObserverCallback")]
    # [wasm_bindgen (method , setter = observe)]
    fn observe_shim(this: &RequestInit, val: &ObserverCallback);
    #[cfg(feature = "RequestRedirect")]
    # [wasm_bindgen (method , setter = redirect)]
    fn redirect_shim(this: &RequestInit, val: RequestRedirect);
    # [wasm_bindgen (method , setter = referrer)]
    fn referrer_shim(this: &RequestInit, val: &str);
    #[cfg(feature = "ReferrerPolicy")]
    # [wasm_bindgen (method , setter = referrerPolicy)]
    fn referrer_policy_shim(this: &RequestInit, val: ReferrerPolicy);
    #[cfg(feature = "AbortSignal")]
    # [wasm_bindgen (method , setter = signal)]
    fn signal_shim(this: &RequestInit, val: Option<&AbortSignal>);
}
impl RequestInit {
    #[doc = "Construct a new `RequestInit`."]
    #[doc = ""]
    #[doc = "*This API requires the following crate features to be activated: `RequestInit`*"]
    pub fn new() -> Self {
        #[allow(unused_mut)]
        let mut ret: Self = ::wasm_bindgen::JsCast::unchecked_into(::js_sys::Object::new());
        ret
    }
    #[doc = "Change the `body` field of this object."]
    #[doc = ""]
    #[doc = "*This API requires the following crate features to be activated: `RequestInit`*"]
    pub fn body(&mut self, val: Option<&::wasm_bindgen::JsValue>) -> &mut Self {
        self.body_shim(val);
        self
    }
    #[cfg(feature = "RequestCache")]
    #[doc = "Change the `cache` field of this object."]
    #[doc = ""]
    #[doc = "*This API requires the following crate features to be activated: `RequestCache`, `RequestInit`*"]
    pub fn cache(&mut self, val: RequestCache) -> &mut Self {
        self.cache_shim(val);
        self
    }
    #[cfg(feature = "RequestCredentials")]
    #[doc = "Change the `credentials` field of this object."]
    #[doc = ""]
    #[doc = "*This API requires the following crate features to be activated: `RequestCredentials`, `RequestInit`*"]
    pub fn credentials(&mut self, val: RequestCredentials) -> &mut Self {
        self.credentials_shim(val);
        self
    }
    #[doc = "Change the `headers` field of this object."]
    #[doc = ""]
    #[doc = "*This API requires the following crate features to be activated: `RequestInit`*"]
    pub fn headers(&mut self, val: &::wasm_bindgen::JsValue) -> &mut Self {
        self.headers_shim(val);
        self
    }
    #[doc = "Change the `integrity` field of this object."]
    #[doc = ""]
    #[doc = "*This API requires the following crate features to be activated: `RequestInit`*"]
    pub fn integrity(&mut self, val: &str) -> &mut Self {
        self.integrity_shim(val);
        self
    }
    #[doc = "Change the `method` field of this object."]
    #[doc = ""]
    #[doc = "*This API requires the following crate features to be activated: `RequestInit`*"]
    pub fn method(&mut self, val: &str) -> &mut Self {
        self.method_shim(val);
        self
    }
    #[cfg(feature = "RequestMode")]
    #[doc = "Change the `mode` field of this object."]
    #[doc = ""]
    #[doc = "*This API requires the following crate features to be activated: `RequestInit`, `RequestMode`*"]
    pub fn mode(&mut self, val: RequestMode) -> &mut Self {
        self.mode_shim(val);
        self
    }
    #[cfg(feature = "ObserverCallback")]
    #[doc = "Change the `observe` field of this object."]
    #[doc = ""]
    #[doc = "*This API requires the following crate features to be activated: `ObserverCallback`, `RequestInit`*"]
    pub fn observe(&mut self, val: &ObserverCallback) -> &mut Self {
        self.observe_shim(val);
        self
    }
    #[cfg(feature = "RequestRedirect")]
    #[doc = "Change the `redirect` field of this object."]
    #[doc = ""]
    #[doc = "*This API requires the following crate features to be activated: `RequestInit`, `RequestRedirect`*"]
    pub fn redirect(&mut self, val: RequestRedirect) -> &mut Self {
        self.redirect_shim(val);
        self
    }
    #[doc = "Change the `referrer` field of this object."]
    #[doc = ""]
    #[doc = "*This API requires the following crate features to be activated: `RequestInit`*"]
    pub fn referrer(&mut self, val: &str) -> &mut Self {
        self.referrer_shim(val);
        self
    }
    #[cfg(feature = "ReferrerPolicy")]
    #[doc = "Change the `referrerPolicy` field of this object."]
    #[doc = ""]
    #[doc = "*This API requires the following crate features to be activated: `ReferrerPolicy`, `RequestInit`*"]
    pub fn referrer_policy(&mut self, val: ReferrerPolicy) -> &mut Self {
        self.referrer_policy_shim(val);
        self
    }
    #[cfg(feature = "AbortSignal")]
    #[doc = "Change the `signal` field of this object."]
    #[doc = ""]
    #[doc = "*This API requires the following crate features to be activated: `AbortSignal`, `RequestInit`*"]
    pub fn signal(&mut self, val: Option<&AbortSignal>) -> &mut Self {
        self.signal_shim(val);
        self
    }
}
impl Default for RequestInit {
    fn default() -> Self {
        Self::new()
    }
}

Which seems to look ok except that it produces the following error:

5 | #[wasm_bindgen]
  | ^^^^^^^^^^^^^^^ the trait `OptionIntoWasmAbi` is not implemented for `&wasm_bindgen::JsValue`

Looks like this is a case of #2139. I'm not quite sure how to solve it. Any ideas?

[Davidster]

Found a way to workaround the issue with some dark magic... let me know what you think. It basically modifies the shim from Option to JsValue and calls val.unwrap_or(&::wasm_bindgen::JsValue::NULL) to convert the Option into a JsValue in the body.

[Davidster]

I will fix the merge conflict once this is ready to be merged. I needed to work on an older version in order be able to build my engine against my wasm_bindgen fork

[Davidster]

I noticed when compiling my engine against this branch that the generated js file got substantially larger since the new 'shim' functions added by the generator are getting added regardless of whether the function is actually used. Any idea about how to mitigate this?

[Davidster]

Ran into another problem with this approach...

error: expected `,`
  --> crates/web-sys/src/features/gen_WidevineCdmManifest.rs:19:42
   |
19 |     # [wasm_bindgen (method , setter = x - cdm - codecs)]
   |                                          ^
error: expected `,`
  --> crates/web-sys/src/features/gen_WidevineCdmManifest.rs:21:42
   |
21 |     # [wasm_bindgen (method , setter = x - cdm - host - versions)]
   |                                          ^
error: expected `,`
  --> crates/web-sys/src/features/gen_WidevineCdmManifest.rs:23:42
   |
23 |     # [wasm_bindgen (method , setter = x - cdm - interface - versions)]
   |                                          ^
error: expected `,`
  --> crates/web-sys/src/features/gen_WidevineCdmManifest.rs:25:42
   |
25 |     # [wasm_bindgen (method , setter = x - cdm - module - versions)]
   |  

is there any way to escape the - characters here?

[daxpedda]

is there any way to escape the - characters here?

You will have to use quotation marks and parse it as a string instead of an ident.

---

Why are the *_shim functions necessary?

[Davidster]

Wouldn't adding quotation marks be a breaking change though?

I didn't consider whether the shims were needed or not tbh. But they would be necessary for the conversion from Option to JsValue since OptionIntoWasmAbi is not implemented for JsValue, like for RequestInit::body()

[daxpedda]

Wouldn't adding quotation marks be a breaking change though?

You don't have to require it, just supporting it.
I didn't look deeper in whats going on though and I don't know why you would need - in the first place.

I didn't consider whether the shims were needed or not tbh. But they would be necessary for the conversion from Option to JsValue since OptionIntoWasmAbi is not implemented for JsValue, like for RequestInit::body()

I'm not following.
But I remembered why this was necessary: to keep the API the same by returning &mut Self.
So all good there!

[Davidster]

the - is required because the WidevineCDMManifest webidl type has some attributes with - in the name 😢 see

wasm-bindgen/crates/web-sys/webidls/enabled/WidevineCDMManifest.webidl

Line 11 in 88efe46

required DOMString x-cdm-module-versions;

Edit: 👍 about not requiring it. I will give that a shot.

---

For the Option thing, it's like I described in my comment: #3898 (comment).

I'm not 100% sure I understand the compile error completely, but my guess is that the wasm-bindgen macro for body shim:

#[wasm_bindgen(method, setter = body)]
fn body_shim(this: &RequestInit, val: Option<&::wasm_bindgen::JsValue>);

Is trying to use the OptionIntoWasmAbi trait on the val parameter in order to be able to pass it to JS. So the fix was to change the signature to this:

#[wasm_bindgen(method, setter = body)]
fn body_shim(this: &RequestInit, val: &::wasm_bindgen::JsValue);

And convert from Option -> JsValue before calling the shim, like this:

#[doc = "Change the `body` field of this object."]
#[doc = ""]
#[doc = "*This API requires the following crate features to be activated: `RequestInit`*"]
pub fn body(&mut self, val: Option<&::wasm_bindgen::JsValue>) -> &mut Self {
    self.body_shim(val.unwrap_or(&::wasm_bindgen::JsValue::NULL));
    self
}

This sadly requires a special check for Option<&::wasm_bindgen::JsValue> in the code:

c67717d#diff-d8976bd33550e367803742179ae21879225e06a871ccfe1e29d8b89af9106d02R697

[Davidster]

I've added support for the hyphens, this PR should be ready for review now 😄 thanks!

[daxpedda]

LGTM so far.

Are you having issues generating the WebIDL?
That's still the last step missing.

[Davidster]

Nope I'm all good with the webidl, it's just a huge diff and I wasn't sure if those are generated by the CI. I'll commit them

[daxpedda]

Small nits, otherwise LGTM!

This is a rather large change, so I will pull in @Liamolucko for a second approval as well.

[Liamolucko]

LGTM!

[Liamolucko]

I noticed when compiling my engine against this branch that the generated js file got substantially larger since the new 'shim' functions added by the generator are getting added regardless of whether the function is actually used. Any idea about how to mitigate this?

Could you elaborate a bit more? I'm not sure if you mean that the shims are never even referenced in Rust (in which case no JS for them should be getting generated), or that they could theoretically be used but aren't in practice (in which case I don't really have a good solution).

[Davidster]

Looking at it again I think I was just confused at the time. It makes sense that a bunch of shims would get added to my JS file since they are indeed referenced from rust.

The part that confused me was that even the non-shim versions weren't showing up in my JS file before the changes from this PR. But this makes sense because those are compiled into the wasm, not the JS 😄

[daxpedda]

Thank you!