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

Add support for cfg_attr for struct fields #4351

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

Conversation

fl0rek
Copy link

@fl0rek fl0rek commented Dec 11, 2024

closes #2703

Overview

  • During #[wasm_bindgen] attribute macro expansion on struct, instead of generating ast for Program, it puts a #[derive(BindgenedStruct)] on it and preserves all the wasm_bindgen atributes as-is.
  • When #[derive(BindgenedStruct)] is expanded, cfg_attr are already evaluated, so macro can just processes the leftover attributes and generate the glue code required, as it's done now. Attributes are then automatically removed by the compiler, since macro is defined as #[proc_macro_derive(BindgenedStruct, attributes(wasm_bindgen))].

@fl0rek fl0rek force-pushed the feat/cfg_attr-structs-rel branch from 0fd7da9 to b172100 Compare December 11, 2024 09:31
@fl0rek fl0rek force-pushed the feat/cfg_attr-structs-rel branch from b172100 to 71a9eca Compare December 11, 2024 15:13
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, this is a great improvement!

Comment on lines +31 to +58
s.attrs.insert(
0,
syn::Attribute {
pound_token: Default::default(),
style: syn::AttrStyle::Outer,
bracket_token: Default::default(),
meta: syn::parse_quote! {
derive(wasm_bindgen::prelude::BindgenedStruct)
},
},
);
if !attr.is_empty() {
s.attrs.insert(
1,
syn::Attribute {
pound_token: Default::default(),
style: syn::AttrStyle::Outer,
bracket_token: Default::default(),
meta: syn::parse_quote! {
wasm_bindgen(#attr)
},
},
);
}

let mut tokens = proc_macro2::TokenStream::new();
s.to_tokens(&mut tokens);
return Ok(tokens);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets just quote here instead.

if !self.generics.params.is_empty() {
bail_span!(
self.generics,
"structs with #[wasm_bindgen] cannot have lifetime or \
type parameters currently"
);
}
let struct_attrs = BindgenAttrs::find(&mut self.attrs)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let struct_attrs = BindgenAttrs::find(&mut self.attrs)?;
let attrs = BindgenAttrs::find(&mut self.attrs)?;

Lets just keep it as attrs, or was there a reason for the rename?

@@ -480,6 +480,19 @@ fn renamed_field() {
js_renamed_field();
}

#[cfg_attr(
target_arch = "wasm32",
wasm_bindgen(inspectable, js_name = "ConditionalSkipClass")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
wasm_bindgen(inspectable, js_name = "ConditionalSkipClass")
wasm_bindgen(js_name = "ConditionalSkipClass")

Or is there some reason this is here?

wasm_bindgen(inspectable, js_name = "ConditionalSkipClass")
)]
pub struct ConditionalSkip {
#[cfg_attr(target_arch = "wasm32", wasm_bindgen(skip))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should check on the JS side if this is properly applied.

Comment on lines +491 to +493
/// cfg_attr annotated field
#[cfg_attr(target_arch = "wasm32", wasm_bindgen(getter_with_clone))]
pub needs_clone: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might want to say in the comment instead that this wouldn't compile otherwise, to explain how we are testing this.

@@ -101,6 +101,7 @@ pub mod prelude {
pub use crate::JsCast;
pub use crate::JsValue;
pub use crate::UnwrapThrowExt;
pub use wasm_bindgen_macro::BindgenedStruct;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets put it in __rt instead. It shouldn't be visible after all.

style: syn::AttrStyle::Outer,
bracket_token: Default::default(),
meta: syn::parse_quote! {
derive(wasm_bindgen::prelude::BindgenedStruct)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have to parse the attributes to extract the #[wasm_bindgen(wasm_bindgen = <path>)] attribute and use it here.

@daxpedda daxpedda added the waiting for author Waiting for author to respond label Dec 15, 2024
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.

wasm_bindgen(skip) in cfg_attr
2 participants