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

@@ -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'

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::*;
+        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,6 +436,7 @@ 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"
@@ -461,18 +454,13 @@ impl ToTokens for ast::Export {
         // 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::*;
+        let export = Ident::new(&export_name, Span::call_site());
+        Descriptor(&export, quote! {
             inform(FUNCTION);
             inform(#nargs);
             #(<#argtys as WasmDescribe>::describe();)*
             #describe_ret
-            }
-        };
-        tokens.to_tokens(into);
+        }).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::*;
+
+        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<HashSet<String>> = 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);
+    }
+}

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;

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<String>,
+
+    /// A map which maintains a list of what identifiers we've imported and what
+    /// they're named locally.
+    ///
+    /// The `Option<String>` 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<Option<String>, HashMap<String, String>>,
+
+    /// A set of all imported identifiers to the number of times they've been
+    /// imported, used to generate new identifiers.
+    pub imported_identifiers: HashMap<String, usize>,
+
     pub exported_classes: HashMap<String, ExportedClass>,
     pub function_table_needed: bool,
     pub run_descriptor: &'a Fn(&str) -> Option<Vec<u32>>,
@@ -1980,39 +1993,82 @@ impl<'a, 'b> SubContext<'a, 'b> {
     }
 
     fn import_name(&mut self, import: &shared::Import, item: &str) -> Result<String, Error> {
-        if let Some(ref module) = import.module {
+        // 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);
+        // 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);
 
-            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
+        // 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 {
-                    self.cx.imports.push_str(&format!(
-                        "\
-                         import {{ {} }} from '{}';\n\
-                         ",
+                    } 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())
         }
-        Ok(match import.js_namespace {
-            Some(ref s) => format!("{}.{}", s, item),
-            None => item.to_string(),
-        })
+    }
+}
+
+fn generate_identifier(name: &str, used_names: &mut HashMap<String, usize>) -> String {
+    let cnt = used_names.entry(name.to_string()).or_insert(0);
+    *cnt += 1;
+    if *cnt == 1 {
+        name.to_string()
+    } else {
+        format!("{}{}", name, cnt)
     }
 }
 

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,

crates/macro-support/src/parser.rs

@@ -353,10 +353,10 @@ impl<'a> ConvertToAst<()> for &'a mut syn::ItemStruct {
     }
 }
 
-impl ConvertToAst<BindgenAttrs> for syn::ForeignItemFn {
+impl<'a> ConvertToAst<(BindgenAttrs, &'a Option<String>)> for syn::ForeignItemFn {
     type Target = ast::ImportKind;
 
-    fn convert(self, opts: BindgenAttrs) -> Self::Target {
+    fn convert(self, (opts, module): (BindgenAttrs, &'a Option<String>)) -> 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<BindgenAttrs> 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<BindgenAttrs> 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"),

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();
+}

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;