From d876475ce34fb81a591505fac7741fd077de3cdc Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 30 Jul 2018 10:50:43 -0700 Subject: [PATCH] Fix some situations with duplicate imports (#589) * Fix importing the same identifier from two modules This needed a fix in two locations: * First the generated descriptor function needed its hash to include the module that the import came from in order to generate unique descriptor functions. * Second the generation of the JS shim needed to handle duplicate identifiers in a more uniform fashion, ensuring that imported names didn't clash. * Fix importing the same name in two modules Previously two descriptor functions with duplicate symbols were emitted, and now only one function is emitted by using a global table to keep track of state across macro invocations. --- crates/backend/Cargo.toml | 1 + crates/backend/src/codegen.rs | 125 ++++++++++++++++------------- crates/backend/src/lib.rs | 2 + crates/cli-support/src/js/mod.rs | 110 ++++++++++++++++++------- crates/cli-support/src/lib.rs | 1 + crates/macro-support/src/parser.rs | 8 +- tests/all/duplicates.rs | 78 ++++++++++++++++++ tests/all/main.rs | 1 + 8 files changed, 241 insertions(+), 85 deletions(-) create mode 100644 tests/all/duplicates.rs diff --git a/crates/backend/Cargo.toml b/crates/backend/Cargo.toml index 00999c8c..2142fb89 100644 --- a/crates/backend/Cargo.toml +++ b/crates/backend/Cargo.toml @@ -15,6 +15,7 @@ spans = ["proc-macro2/nightly"] extra-traits = ["syn/extra-traits"] [dependencies] +lazy_static = "1.0.0" log = "0.4" proc-macro2 = "0.4.8" quote = '0.6' diff --git a/crates/backend/src/codegen.rs b/crates/backend/src/codegen.rs index 5ece1108..f14bbd94 100644 --- a/crates/backend/src/codegen.rs +++ b/crates/backend/src/codegen.rs @@ -1,4 +1,5 @@ use std::collections::HashSet; +use std::sync::Mutex; use std::sync::atomic::{AtomicUsize, Ordering, ATOMIC_USIZE_INIT}; use ast; @@ -221,10 +222,6 @@ impl ToTokens for ast::StructField { let ty = &self.ty; let getter = &self.getter; let setter = &self.setter; - let desc = Ident::new( - &format!("__wbindgen_describe_{}", getter), - Span::call_site(), - ); (quote! { #[no_mangle] #[doc(hidden)] @@ -246,13 +243,10 @@ impl ToTokens for ast::StructField { &mut GlobalStack::new(), ) } + }).to_tokens(tokens); - #[no_mangle] - #[doc(hidden)] - pub extern fn #desc() { - use wasm_bindgen::describe::*; - <#ty as WasmDescribe>::describe(); - } + Descriptor(&getter, quote! { + <#ty as WasmDescribe>::describe(); }).to_tokens(tokens); if self.readonly { @@ -421,13 +415,11 @@ impl ToTokens for ast::Export { } None => quote! { inform(0); }, }; - let descriptor_name = format!("__wbindgen_describe_{}", export_name); - let descriptor_name = Ident::new(&descriptor_name, Span::call_site()); let nargs = self.function.arguments.len() as u32; let argtys = self.function.arguments.iter().map(|arg| &arg.ty); let attrs = &self.function.rust_attrs; - let tokens = quote! { + (quote! { #(#attrs)* #[export_name = #export_name] #[allow(non_snake_case)] @@ -444,35 +436,31 @@ impl ToTokens for ast::Export { }; #convert_ret } + }).to_tokens(into); - // In addition to generating the shim function above which is what - // our generated JS will invoke, we *also* generate a "descriptor" - // shim. This descriptor shim uses the `WasmDescribe` trait to - // programmatically describe the type signature of the generated - // shim above. This in turn is then used to inform the - // `wasm-bindgen` CLI tool exactly what types and such it should be - // using in JS. - // - // Note that this descriptor function is a purely an internal detail - // of `#[wasm_bindgen]` and isn't intended to be exported to anyone - // or actually part of the final was binary. Additionally, this is - // literally executed when the `wasm-bindgen` tool executes. - // - // In any case, there's complications in `wasm-bindgen` to handle - // this, but the tl;dr; is that this is stripped from the final wasm - // binary along with anything it references. - #[no_mangle] - #[cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))] - #[doc(hidden)] - pub extern fn #descriptor_name() { - use wasm_bindgen::describe::*; - inform(FUNCTION); - inform(#nargs); - #(<#argtys as WasmDescribe>::describe();)* - #describe_ret - } - }; - tokens.to_tokens(into); + // In addition to generating the shim function above which is what + // our generated JS will invoke, we *also* generate a "descriptor" + // shim. This descriptor shim uses the `WasmDescribe` trait to + // programmatically describe the type signature of the generated + // shim above. This in turn is then used to inform the + // `wasm-bindgen` CLI tool exactly what types and such it should be + // using in JS. + // + // Note that this descriptor function is a purely an internal detail + // of `#[wasm_bindgen]` and isn't intended to be exported to anyone + // or actually part of the final was binary. Additionally, this is + // literally executed when the `wasm-bindgen` tool executes. + // + // In any case, there's complications in `wasm-bindgen` to handle + // this, but the tl;dr; is that this is stripped from the final wasm + // binary along with anything it references. + let export = Ident::new(&export_name, Span::call_site()); + Descriptor(&export, quote! { + inform(FUNCTION); + inform(#nargs); + #(<#argtys as WasmDescribe>::describe();)* + #describe_ret + }).to_tokens(into); } } @@ -853,25 +841,18 @@ impl<'a> ToTokens for DescribeImport<'a> { ast::ImportKind::Type(_) => return, ast::ImportKind::Enum(_) => return, }; - let describe_name = format!("__wbindgen_describe_{}", f.shim); - let describe_name = Ident::new(&describe_name, Span::call_site()); let argtys = f.function.arguments.iter().map(|arg| &arg.ty); let nargs = f.function.arguments.len() as u32; let inform_ret = match &f.js_ret { Some(ref t) => quote! { inform(1); <#t as WasmDescribe>::describe(); }, None => quote! { inform(0); }, }; - (quote! { - #[no_mangle] - #[allow(non_snake_case)] - #[doc(hidden)] - pub extern fn #describe_name() { - use wasm_bindgen::describe::*; - inform(FUNCTION); - inform(#nargs); - #(<#argtys as WasmDescribe>::describe();)* - #inform_ret - } + + Descriptor(&f.shim, quote! { + inform(FUNCTION); + inform(#nargs); + #(<#argtys as WasmDescribe>::describe();)* + #inform_ret }).to_tokens(tokens); } } @@ -1016,3 +997,39 @@ impl ToTokens for ast::Const { } } } + +/// Emits the necessary glue tokens for "descriptor", generating an appropriate +/// symbol name as well as attributes around the descriptor function itself. +struct Descriptor<'a, T>(&'a Ident, T); + +impl<'a, T: ToTokens> ToTokens for Descriptor<'a, T> { + fn to_tokens(&self, tokens: &mut TokenStream) { + // It's possible for the same descriptor to be emitted in two different + // modules (aka a value imported twice in a crate, each in a separate + // module). In this case no need to emit duplicate descriptors (which + // leads to duplicate symbol errors), instead just emit one. + // + // It's up to the descriptors themselves to ensure they have unique + // names for unique items imported, currently done via `ShortHash` and + // hashing appropriate data into the symbol name. + lazy_static! { + static ref DESCRIPTORS_EMITTED: Mutex> = Default::default(); + } + if !DESCRIPTORS_EMITTED.lock().unwrap().insert(self.0.to_string()) { + return + } + + let name = Ident::new(&format!("__wbindgen_describe_{}", self.0), self.0.span()); + let inner = &self.1; + (quote! { + #[no_mangle] + #[allow(non_snake_case)] + #[doc(hidden)] + #[cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))] + pub extern fn #name() { + use wasm_bindgen::describe::*; + #inner + } + }).to_tokens(tokens); + } +} diff --git a/crates/backend/src/lib.rs b/crates/backend/src/lib.rs index 75e104c0..4edb13d3 100755 --- a/crates/backend/src/lib.rs +++ b/crates/backend/src/lib.rs @@ -2,6 +2,8 @@ #![cfg_attr(feature = "extra-traits", deny(missing_debug_implementations))] #![doc(html_root_url = "https://docs.rs/wasm-bindgen-backend/0.2")] +#[macro_use] +extern crate lazy_static; #[macro_use] extern crate log; extern crate proc_macro2; diff --git a/crates/cli-support/src/js/mod.rs b/crates/cli-support/src/js/mod.rs index 33d4391b..1f04bb9a 100644 --- a/crates/cli-support/src/js/mod.rs +++ b/crates/cli-support/src/js/mod.rs @@ -26,7 +26,20 @@ pub struct Context<'a> { pub required_internal_exports: HashSet<&'static str>, pub config: &'a Bindgen, pub module: &'a mut Module, - pub imported_names: HashSet, + + /// A map which maintains a list of what identifiers we've imported and what + /// they're named locally. + /// + /// The `Option` key is the module that identifiers were imported + /// from, `None` being the global module. The second key is a map of + /// identifiers we've already imported from the module to what they're + /// called locally. + pub imported_names: HashMap, HashMap>, + + /// A set of all imported identifiers to the number of times they've been + /// imported, used to generate new identifiers. + pub imported_identifiers: HashMap, + pub exported_classes: HashMap, pub function_table_needed: bool, pub run_descriptor: &'a Fn(&str) -> Option>, @@ -1980,39 +1993,82 @@ impl<'a, 'b> SubContext<'a, 'b> { } fn import_name(&mut self, import: &shared::Import, item: &str) -> Result { - if let Some(ref module) = import.module { - if self.cx.config.no_modules { + // First up, imports don't work at all in `--no-modules` mode as we're + // not sure how to import them. + if self.cx.config.no_modules { + if let Some(module) = &import.module { bail!( "import from `{}` module not allowed with `--no-modules`; \ use `--nodejs` or `--browser` instead", module ); } - - let name = import.js_namespace.as_ref().map(|s| &**s).unwrap_or(item); - - if self.cx.imported_names.insert(name.to_string()) { - if self.cx.use_node_require() { - self.cx.imports.push_str(&format!( - "\ - const {} = require('{}').{};\n\ - ", - name, module, name - )); - } else { - self.cx.imports.push_str(&format!( - "\ - import {{ {} }} from '{}';\n\ - ", - name, module - )); - } - } } - Ok(match import.js_namespace { - Some(ref s) => format!("{}.{}", s, item), - None => item.to_string(), - }) + + // Figure out what identifier we're importing from the module. If we've + // got a namespace we use that, otherwise it's the name specified above. + let name_to_import = import.js_namespace + .as_ref() + .map(|s| &**s) + .unwrap_or(item); + + // Here's where it's a bit tricky. We need to make sure that importing + // the same identifier from two different modules works, and they're + // named uniquely below. Additionally if we've already imported the same + // identifier from the module in question then we'd like to reuse the + // one that was previously imported. + // + // Our `imported_names` map keeps track of all imported identifiers from + // modules, mapping the imported names onto names actually available for + // use in our own module. If our identifier isn't present then we + // generate a new identifier and are sure to generate the appropriate JS + // import for our new identifier. + let use_node_require = self.cx.use_node_require(); + let imported_identifiers = &mut self.cx.imported_identifiers; + let imports = &mut self.cx.imports; + let identifier = self.cx.imported_names.entry(import.module.clone()) + .or_insert_with(Default::default) + .entry(name_to_import.to_string()) + .or_insert_with(|| { + let name = generate_identifier(name_to_import, imported_identifiers); + if let Some(module) = &import.module { + if use_node_require { + imports.push_str(&format!( + "const {} = require('{}').{};\n", + name, module, name_to_import + )); + } else if name_to_import == name { + imports.push_str(&format!( + "import {{ {} }} from '{}';\n", + name, module + )); + } else { + imports.push_str(&format!( + "import {{ {} as {} }} from '{}';\n", + name_to_import, name, module + )); + } + } + name + }); + + // If there's a namespace we didn't actually import `item` but rather + // the namespace, so access through that. + if import.js_namespace.is_some() { + Ok(format!("{}.{}", identifier, item)) + } else { + Ok(identifier.to_string()) + } + } +} + +fn generate_identifier(name: &str, used_names: &mut HashMap) -> String { + let cnt = used_names.entry(name.to_string()).or_insert(0); + *cnt += 1; + if *cnt == 1 { + name.to_string() + } else { + format!("{}{}", name, cnt) } } diff --git a/crates/cli-support/src/lib.rs b/crates/cli-support/src/lib.rs index 7108c70f..d8cb8fdb 100644 --- a/crates/cli-support/src/lib.rs +++ b/crates/cli-support/src/lib.rs @@ -198,6 +198,7 @@ impl Bindgen { exposed_globals: Default::default(), required_internal_exports: Default::default(), imported_names: Default::default(), + imported_identifiers: Default::default(), exported_classes: Default::default(), config: &self, module: &mut module, diff --git a/crates/macro-support/src/parser.rs b/crates/macro-support/src/parser.rs index 254d2d57..703e09ea 100644 --- a/crates/macro-support/src/parser.rs +++ b/crates/macro-support/src/parser.rs @@ -353,10 +353,10 @@ impl<'a> ConvertToAst<()> for &'a mut syn::ItemStruct { } } -impl ConvertToAst for syn::ForeignItemFn { +impl<'a> ConvertToAst<(BindgenAttrs, &'a Option)> for syn::ForeignItemFn { type Target = ast::ImportKind; - fn convert(self, opts: BindgenAttrs) -> Self::Target { + fn convert(self, (opts, module): (BindgenAttrs, &'a Option)) -> Self::Target { let js_name = opts.js_name().unwrap_or(&self.ident).clone(); let wasm = function_from_decl(&js_name, self.decl, self.attrs, self.vis, false).0; let catch = opts.catch(); @@ -458,7 +458,7 @@ impl ConvertToAst for syn::ForeignItemFn { ast::ImportFunctionKind::Normal => (0, "n"), ast::ImportFunctionKind::Method { ref class, .. } => (1, &class[..]), }; - let data = (ns, &self.ident); + let data = (ns, &self.ident, module); format!("__wbg_{}_{}", js_name, ShortHash(data)) }; ast::ImportKind::Function(ast::ImportFunction { @@ -803,7 +803,7 @@ impl MacroParse for syn::ItemForeignMod { .map(|s| s.to_string()); let js_namespace = item_opts.js_namespace().or(opts.js_namespace()).cloned(); let mut kind = match item { - syn::ForeignItem::Fn(f) => f.convert(item_opts), + syn::ForeignItem::Fn(f) => f.convert((item_opts, &module)), syn::ForeignItem::Type(t) => t.convert(()), syn::ForeignItem::Static(s) => s.convert(item_opts), _ => panic!("only foreign functions/types allowed for now"), diff --git a/tests/all/duplicates.rs b/tests/all/duplicates.rs new file mode 100644 index 00000000..d70f042b --- /dev/null +++ b/tests/all/duplicates.rs @@ -0,0 +1,78 @@ +use super::project; + +#[test] +fn same_function_different_locations() { + project() + .file( + "src/lib.rs", + r#" + #![feature(use_extern_macros)] + + extern crate wasm_bindgen; + use wasm_bindgen::prelude::*; + + pub mod a { + use wasm_bindgen::prelude::*; + #[wasm_bindgen(module = "./foo")] + extern { + pub fn foo(); + } + } + + pub mod b { + use wasm_bindgen::prelude::*; + #[wasm_bindgen(module = "./foo")] + extern { + pub fn foo(); + } + } + + #[wasm_bindgen] + pub fn test() { + a::foo(); + b::foo(); + } + "#, + ) + .file("foo.js", "export function foo() {}") + .test(); +} + +#[test] +fn same_function_different_modules() { + project() + .file( + "src/lib.rs", + r#" + #![feature(use_extern_macros)] + + extern crate wasm_bindgen; + use wasm_bindgen::prelude::*; + + pub mod a { + use wasm_bindgen::prelude::*; + #[wasm_bindgen(module = "./foo")] + extern { + pub fn foo() -> bool; + } + } + + pub mod b { + use wasm_bindgen::prelude::*; + #[wasm_bindgen(module = "./bar")] + extern { + pub fn foo() -> bool; + } + } + + #[wasm_bindgen] + pub fn test() { + assert!(a::foo()); + assert!(!b::foo()); + } + "#, + ) + .file("foo.js", "export function foo() { return true; } ") + .file("bar.js", "export function foo() { return false; } ") + .test(); +} diff --git a/tests/all/main.rs b/tests/all/main.rs index 563c7b6b..ec6592b6 100644 --- a/tests/all/main.rs +++ b/tests/all/main.rs @@ -8,6 +8,7 @@ mod classes; mod closures; mod comments; mod dependencies; +mod duplicates; mod enums; mod import_class; mod imports;