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

Don't use reflect in webidl dictionary setters #3898

Merged

Conversation

Davidster
Copy link
Contributor

Implements solution 1 from #3468 (comment).

Solution 3 will come after assuming this goes smoothly

@Davidster Davidster changed the title don't use reflect in webidl dictionary setters Don't use reflect in webidl dictionary setters Mar 19, 2024
@Davidster
Copy link
Contributor Author

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

@Davidster Davidster marked this pull request as draft March 19, 2024 03:01
@Davidster
Copy link
Contributor Author

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
Copy link
Contributor Author

Davidster commented Mar 20, 2024

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 Davidster force-pushed the no_reflect_in_webidl_dictionary_setters branch from 75b3058 to c67717d Compare March 21, 2024 02:22
@Davidster
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

Davidster commented Mar 27, 2024

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

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 Davidster marked this pull request as ready for review March 29, 2024 02:00
@Davidster
Copy link
Contributor Author

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

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM so far.

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

CHANGELOG.md Outdated Show resolved Hide resolved
@daxpedda daxpedda added the waiting for author Waiting for author to respond label Apr 1, 2024
crates/webidl/Cargo.toml Outdated Show resolved Hide resolved
@Davidster
Copy link
Contributor Author

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

@Davidster Davidster requested a review from daxpedda April 1, 2024 17:27
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nits, otherwise LGTM!

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

CHANGELOG.md Outdated Show resolved Hide resolved
crates/webidl/Cargo.toml Outdated Show resolved Hide resolved
@daxpedda daxpedda requested a review from Liamolucko April 2, 2024 07:30
@Davidster Davidster requested a review from daxpedda April 2, 2024 22:58
Copy link
Collaborator

@Liamolucko Liamolucko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

crates/webidl/src/generator.rs Outdated Show resolved Hide resolved
@Liamolucko
Copy link
Collaborator

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
Copy link
Contributor Author

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 😄

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author Waiting for author to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants