From 01cdf22ada44051bdc284e3cd0a50ede4a213cd7 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 25 Jan 2019 14:31:50 -0800 Subject: [PATCH] Fix conditional #[wasm_bindgen] in impls Reported in #1191 the fix requires us to get a bit creative I think. The general gist is that a block like this: #[wasm_bindgen] impl Foo { pub fn foo() {} } was previously expanded all in one go. Now, however, it's expanded into: impl Foo { #[__wasm_bindgen_class_marker(Foo = "Foo")] pub fn foo() {} } // goop generated by orginal #[wasm_bindgen] This method of expansion takes advantage of rustc's recursive expansion feature. It also allows us to expand `impl` blocks and allow inner items to not be fully expanded yet, such as still having `#[cfg]` attributes (like in the original bug report). We use theinternal `__wasm_bindgen_class_marker` to indicate that we're parsing an `ImplItemMethod` unconditionally, and then generation proceeds as usual. The only final catch is that when we're expanding in an `impl` block we have to generate tokens for the `Program` (wasm-bindgen injected goop like the custom section) inside the body of the function itself instead of next to it. Otherwise we'd get syntax errors inside of impl blocks! Closes #1191 --- crates/macro-support/src/lib.rs | 68 +++++++++++++++++ crates/macro-support/src/parser.rs | 117 ++++++++++++++++++----------- crates/macro/src/lib.rs | 13 ++++ src/lib.rs | 2 + tests/wasm/classes.js | 5 ++ tests/wasm/classes.rs | 17 +++++ 6 files changed, 178 insertions(+), 44 deletions(-) diff --git a/crates/macro-support/src/lib.rs b/crates/macro-support/src/lib.rs index b931bc8259c..c9c70494654 100644 --- a/crates/macro-support/src/lib.rs +++ b/crates/macro-support/src/lib.rs @@ -13,8 +13,11 @@ extern crate wasm_bindgen_shared as shared; use backend::{Diagnostic, TryToTokens}; pub use parser::BindgenAttrs; +use quote::ToTokens; use parser::MacroParse; use proc_macro2::TokenStream; +use syn::parse::{Parse, ParseStream, Result as SynResult}; +use quote::TokenStreamExt; mod parser; @@ -36,3 +39,68 @@ pub fn expand(attr: TokenStream, input: TokenStream) -> Result Result { + parser::reset_attrs_used(); + let mut item = syn::parse2::(input)?; + let opts: ClassMarker = syn::parse2(attr)?; + + let mut program = backend::ast::Program::default(); + item.macro_parse(&mut program, (&opts.class, &opts.js_class))?; + parser::assert_all_attrs_checked(); // same as above + + // This is where things are slightly different, we are being expanded in the + // context of an impl so we can't inject arbitrary item-like tokens into the + // output stream. If we were to do that then it wouldn't parse! + // + // Instead what we want to do is to generate the tokens for `program` into + // the header of the function. This'll inject some no_mangle functions and + // statics and such, and they should all be valid in the context of the + // start of a function. + // + // We manually implement `ToTokens for ImplItemMethod` here, injecting our + // program's tokens before the actual method's inner body tokens. + let mut tokens = proc_macro2::TokenStream::new(); + tokens.append_all(item.attrs.iter().filter(|attr| { + match attr.style { + syn::AttrStyle::Outer => true, + _ => false, + } + })); + item.vis.to_tokens(&mut tokens); + item.sig.to_tokens(&mut tokens); + let mut err = None; + item.block.brace_token.surround(&mut tokens, |tokens| { + if let Err(e) = program.try_to_tokens(tokens) { + err = Some(e); + } + tokens.append_all(item.attrs.iter().filter(|attr| { + match attr.style { + syn::AttrStyle::Inner(_) => true, + _ => false, + } + })); + tokens.append_all(&item.block.stmts); + }); + + if let Some(err) = err { + return Err(err) + } + + Ok(tokens) +} + +struct ClassMarker { + class: syn::Ident, + js_class: String, +} + +impl Parse for ClassMarker { + fn parse(input: ParseStream) -> SynResult { + let class = input.parse::()?; + input.parse::()?; + let js_class = input.parse::()?.value(); + Ok(ClassMarker { class, js_class }) + } +} diff --git a/crates/macro-support/src/parser.rs b/crates/macro-support/src/parser.rs index ef85cc5f861..20e3524fbab 100644 --- a/crates/macro-support/src/parser.rs +++ b/crates/macro-support/src/parser.rs @@ -796,7 +796,7 @@ impl<'a> MacroParse<(Option, &'a mut TokenStream)> for syn::Item { } impl<'a> MacroParse for &'a mut syn::ItemImpl { - fn macro_parse(self, program: &mut ast::Program, opts: BindgenAttrs) -> Result<(), Diagnostic> { + fn macro_parse(self, _program: &mut ast::Program, opts: BindgenAttrs) -> Result<(), Diagnostic> { if self.defaultness.is_some() { bail_span!( self.defaultness, @@ -830,7 +830,7 @@ impl<'a> MacroParse for &'a mut syn::ItemImpl { }; let mut errors = Vec::new(); for item in self.items.iter_mut() { - if let Err(e) = (&name, item).macro_parse(program, &opts) { + if let Err(e) = prepare_for_impl_recursion(item, &name, &opts) { errors.push(e); } } @@ -840,77 +840,106 @@ impl<'a> MacroParse for &'a mut syn::ItemImpl { } } -impl<'a, 'b> MacroParse<&'a BindgenAttrs> for (&'a Ident, &'b mut syn::ImplItem) { +// Prepare for recursion into an `impl` block. Here we want to attach an +// internal attribute, `__wasm_bindgen_class_marker`, with any metadata we need +// to pass from the impl to the impl item. Recursive macro expansion will then +// expand the `__wasm_bindgen_class_marker` attribute. +// +// Note that we currently do this because inner items may have things like cfgs +// on them, so we want to expand the impl first, let the insides get cfg'd, and +// then go for the rest. +fn prepare_for_impl_recursion( + item: &mut syn::ImplItem, + class: &Ident, + impl_opts: &BindgenAttrs +) -> Result<(), Diagnostic> { + let method = match item { + syn::ImplItem::Method(m) => m, + syn::ImplItem::Const(_) => { + bail_span!( + &*item, + "const definitions aren't supported with #[wasm_bindgen]" + ); + } + syn::ImplItem::Type(_) => bail_span!( + &*item, + "type definitions in impls aren't supported with #[wasm_bindgen]" + ), + syn::ImplItem::Existential(_) => bail_span!( + &*item, + "existentials in impls aren't supported with #[wasm_bindgen]" + ), + syn::ImplItem::Macro(_) => { + // In theory we want to allow this, but we have no way of expanding + // the macro and then placing our magical attributes on the expanded + // functions. As a result, just disallow it for now to hopefully + // ward off buggy results from this macro. + bail_span!(&*item, "macros in impls aren't supported"); + } + syn::ImplItem::Verbatim(_) => panic!("unparsed impl item?"), + }; + + let js_class = impl_opts + .js_class() + .map(|s| s.0.to_string()) + .unwrap_or(class.to_string()); + + method.attrs.insert(0, syn::Attribute { + pound_token: Default::default(), + style: syn::AttrStyle::Outer, + bracket_token: Default::default(), + path: syn::Ident::new("__wasm_bindgen_class_marker", Span::call_site()).into(), + tts: quote::quote! { (#class = #js_class) }.into(), + }); + + Ok(()) +} + +impl<'a, 'b> MacroParse<(&'a Ident, &'a str)> for &'b mut syn::ImplItemMethod { fn macro_parse( self, program: &mut ast::Program, - impl_opts: &'a BindgenAttrs, + (class, js_class): (&'a Ident, &'a str), ) -> Result<(), Diagnostic> { - let (class, item) = self; - let method = match item { - syn::ImplItem::Method(ref mut m) => m, - syn::ImplItem::Const(_) => { - bail_span!( - &*item, - "const definitions aren't supported with #[wasm_bindgen]" - ); - } - syn::ImplItem::Type(_) => bail_span!( - &*item, - "type definitions in impls aren't supported with #[wasm_bindgen]" - ), - syn::ImplItem::Existential(_) => bail_span!( - &*item, - "existentials in impls aren't supported with #[wasm_bindgen]" - ), - syn::ImplItem::Macro(_) => { - bail_span!(&*item, "macros in impls aren't supported"); - } - syn::ImplItem::Verbatim(_) => panic!("unparsed impl item?"), - }; - match method.vis { + match self.vis { syn::Visibility::Public(_) => {} _ => return Ok(()), } - if method.defaultness.is_some() { + if self.defaultness.is_some() { panic!("default methods are not supported"); } - if method.sig.constness.is_some() { + if self.sig.constness.is_some() { bail_span!( - method.sig.constness, + self.sig.constness, "can only #[wasm_bindgen] non-const functions", ); } - if method.sig.unsafety.is_some() { - bail_span!(method.sig.unsafety, "can only bindgen safe functions",); + if self.sig.unsafety.is_some() { + bail_span!(self.sig.unsafety, "can only bindgen safe functions",); } - let opts = BindgenAttrs::find(&mut method.attrs)?; - let comments = extract_doc_comments(&method.attrs); + let opts = BindgenAttrs::find(&mut self.attrs)?; + let comments = extract_doc_comments(&self.attrs); let is_constructor = opts.constructor().is_some(); let (function, method_self) = function_from_decl( - &method.sig.ident, + &self.sig.ident, &opts, - Box::new(method.sig.decl.clone()), - method.attrs.clone(), - method.vis.clone(), + Box::new(self.sig.decl.clone()), + self.attrs.clone(), + self.vis.clone(), true, Some(class), )?; - let js_class = impl_opts - .js_class() - .map(|s| s.0.to_string()) - .unwrap_or(class.to_string()); program.exports.push(ast::Export { rust_class: Some(class.clone()), - js_class: Some(js_class), + js_class: Some(js_class.to_string()), method_self, is_constructor, function, comments, start: false, - rust_name: method.sig.ident.clone(), + rust_name: self.sig.ident.clone(), }); opts.check_used()?; Ok(()) diff --git a/crates/macro/src/lib.rs b/crates/macro/src/lib.rs index f94d2b52c86..69149fddbca 100755 --- a/crates/macro/src/lib.rs +++ b/crates/macro/src/lib.rs @@ -19,3 +19,16 @@ pub fn wasm_bindgen(attr: TokenStream, input: TokenStream) -> TokenStream { Err(diagnostic) => (quote! { #diagnostic }).into(), } } + +#[proc_macro_attribute] +pub fn __wasm_bindgen_class_marker(attr: TokenStream, input: TokenStream) -> TokenStream { + match macro_support::expand_class_marker(attr.into(), input.into()) { + Ok(tokens) => { + if cfg!(feature = "xxx_debug_only_print_generated_code") { + println!("{}", tokens); + } + tokens.into() + } + Err(diagnostic) => (quote! { #diagnostic }).into(), + } +} diff --git a/src/lib.rs b/src/lib.rs index 7b2ffb94184..7a94632dcbf 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -37,6 +37,8 @@ macro_rules! if_std { /// ``` pub mod prelude { pub use wasm_bindgen_macro::wasm_bindgen; + #[doc(hidden)] + pub use wasm_bindgen_macro::__wasm_bindgen_class_marker; pub use JsValue; if_std! { diff --git a/tests/wasm/classes.js b/tests/wasm/classes.js index 325ae72cddc..1c3ef4ccab7 100644 --- a/tests/wasm/classes.js +++ b/tests/wasm/classes.js @@ -143,3 +143,8 @@ exports.js_renamed_export = () => { x.foo(); x.bar(x); }; + +exports.js_conditional_bindings = () => { + const x = new wasm.ConditionalBindings(); + x.free(); +}; diff --git a/tests/wasm/classes.rs b/tests/wasm/classes.rs index ba64a105d14..e73a1b6e272 100644 --- a/tests/wasm/classes.rs +++ b/tests/wasm/classes.rs @@ -22,6 +22,7 @@ extern "C" { fn js_js_rename(); fn js_access_fields(); fn js_renamed_export(); + fn js_conditional_bindings(); } #[wasm_bindgen_test] @@ -402,3 +403,19 @@ impl RenamedExport { fn renamed_export() { js_renamed_export(); } + +#[cfg_attr(target_arch = "wasm32", wasm_bindgen)] +pub struct ConditionalBindings { +} + +#[cfg_attr(target_arch = "wasm32", wasm_bindgen)] +impl ConditionalBindings { + #[cfg_attr(target_arch = "wasm32", wasm_bindgen(constructor))] + pub fn new() -> ConditionalBindings { + ConditionalBindings {} + } +} +#[wasm_bindgen_test] +fn conditional_bindings() { + js_conditional_bindings(); +}