try to fix global / modulaized import ns conflict (#2057)

* use global import map for rename

* fix same ns import

* cargo fmt

* add basic test

* move generate_identifier, add comments, add tests

* remove leading &mut

* remove unnecessary bail

* use import_name for global and some refine

* Add back in error handling, clean up instruction iteration

* Remove unnecessary patch statements

Co-authored-by: Alex Crichton <alex@alexcrichton.com>

crates/cli-support/src/js/mod.rs

@@ -113,7 +113,7 @@ impl<'a> Context<'a> {
         contents: &str,
         comments: Option<&str>,
     ) -> Result<(), Error> {
-        let definition_name = generate_identifier(export_name, &mut self.defined_identifiers);
+        let definition_name = self.generate_identifier(export_name);
         if contents.starts_with("class") && definition_name != export_name {
             bail!("cannot shadow already defined class `{}`", export_name);
         }
@@ -1922,6 +1922,18 @@ impl<'a> Context<'a> {
         require_class(&mut self.exported_classes, name).wrap_needed = true;
     }
 
+    fn add_module_import(&mut self, module: String, name: &str, actual: &str) {
+        let rename = if name == actual {
+            None
+        } else {
+            Some(actual.to_string())
+        };
+        self.js_imports
+            .entry(module)
+            .or_insert(Vec::new())
+            .push((name.to_string(), rename));
+    }
+
     fn import_name(&mut self, import: &JsImport) -> Result<String, Error> {
         if let Some(name) = self.imported_names.get(&import.name) {
             let mut name = name.clone();
@@ -1932,30 +1944,17 @@ impl<'a> Context<'a> {
             return Ok(name.clone());
         }
 
-        let js_imports = &mut self.js_imports;
-        let mut add_module_import = |module: String, name: &str, actual: &str| {
-            let rename = if name == actual {
-                None
-            } else {
-                Some(actual.to_string())
-            };
-            js_imports
-                .entry(module)
-                .or_insert(Vec::new())
-                .push((name.to_string(), rename));
-        };
-
         let mut name = match &import.name {
             JsImportName::Module { module, name } => {
-                let unique_name = generate_identifier(name, &mut self.defined_identifiers);
+                let unique_name = self.generate_identifier(name);
-                add_module_import(module.clone(), name, &unique_name);
+                self.add_module_import(module.clone(), name, &unique_name);
                 unique_name
             }
 
             JsImportName::LocalModule { module, name } => {
-                let unique_name = generate_identifier(name, &mut self.defined_identifiers);
+                let unique_name = self.generate_identifier(name);
                 let module = self.config.local_module_name(module);
-                add_module_import(module, name, &unique_name);
+                self.add_module_import(module, name, &unique_name);
                 unique_name
             }
 
@@ -1967,8 +1966,8 @@ impl<'a> Context<'a> {
                 let module = self
                     .config
                     .inline_js_module_name(unique_crate_identifier, *snippet_idx_in_crate);
-                let unique_name = generate_identifier(name, &mut self.defined_identifiers);
+                let unique_name = self.generate_identifier(name);
-                add_module_import(module, name, &unique_name);
+                self.add_module_import(module, name, &unique_name);
                 unique_name
             }
 
@@ -1998,7 +1997,7 @@ impl<'a> Context<'a> {
             }
 
             JsImportName::Global { name } => {
-                let unique_name = generate_identifier(name, &mut self.defined_identifiers);
+                let unique_name = self.generate_identifier(name);
                 if unique_name != *name {
                     bail!("cannot import `{}` from two locations", name);
                 }
@@ -2055,6 +2054,7 @@ impl<'a> Context<'a> {
     }
 
     pub fn generate(&mut self) -> Result<(), Error> {
+        self.prestore_global_import_identifiers()?;
         for (id, adapter) in crate::sorted_iter(&self.wit.adapters) {
             let instrs = match &adapter.kind {
                 AdapterKind::Import { .. } => continue,
@@ -2084,6 +2084,29 @@ impl<'a> Context<'a> {
         Ok(())
     }
 
+    /// Registers import names for all `Global` imports first before we actually
+    /// process any adapters.
+    ///
+    /// `Global` names must be imported as their exact name, so if the same name
+    /// from a global is also imported from a module we have to be sure to
+    /// import the global first to ensure we don't shadow the actual global
+    /// value. Otherwise we have no way of accessing the global value!
+    ///
+    /// This function will iterate through the import map up-front and generate
+    /// a cache entry for each import name which is a `Global`.
+    fn prestore_global_import_identifiers(&mut self) -> Result<(), Error> {
+        for import in self.aux.import_map.values() {
+            let js = match import {
+                AuxImport::Value(AuxValue::Bare(js)) => js,
+                _ => continue,
+            };
+            if let JsImportName::Global { .. } = js.name {
+                self.import_name(js)?;
+            }
+        }
+        Ok(())
+    }
+
     fn generate_adapter(
         &mut self,
         id: AdapterId,
@@ -3133,6 +3156,21 @@ impl<'a> Context<'a> {
     fn adapter_name(&self, id: AdapterId) -> String {
         format!("__wbg_adapter_{}", id.0)
     }
+
+    fn generate_identifier(&mut self, name: &str) -> String {
+        let cnt = self
+            .defined_identifiers
+            .entry(name.to_string())
+            .or_insert(0);
+        *cnt += 1;
+        // We want to mangle `default` at once, so we can support default exports and don't generate
+        // invalid glue code like this: `import { default } from './module';`.
+        if *cnt == 1 && name != "default" {
+            name.to_string()
+        } else {
+            format!("{}{}", name, cnt)
+        }
+    }
 }
 
 fn check_duplicated_getter_and_setter_names(
@@ -3180,18 +3218,6 @@ fn check_duplicated_getter_and_setter_names(
     Ok(())
 }
 
-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;
-    // We want to mangle `default` at once, so we can support default exports and don't generate
-    // invalid glue code like this: `import { default } from './module';`.
-    if *cnt == 1 && name != "default" {
-        name.to_string()
-    } else {
-        format!("{}{}", name, cnt)
-    }
-}
-
 fn format_doc_comments(comments: &str, js_doc_comments: Option<String>) -> String {
     let body: String = comments.lines().map(|c| format!("*{}\n", c)).collect();
     let doc = if let Some(docs) = js_doc_comments {
@@ -3294,27 +3320,6 @@ impl ExportedClass {
     }
 }
 
-#[test]
-fn test_generate_identifier() {
-    let mut used_names: HashMap<String, usize> = HashMap::new();
-    assert_eq!(
-        generate_identifier("someVar", &mut used_names),
-        "someVar".to_string()
-    );
-    assert_eq!(
-        generate_identifier("someVar", &mut used_names),
-        "someVar2".to_string()
-    );
-    assert_eq!(
-        generate_identifier("default", &mut used_names),
-        "default1".to_string()
-    );
-    assert_eq!(
-        generate_identifier("default", &mut used_names),
-        "default2".to_string()
-    );
-}
-
 struct MemView {
     name: &'static str,
     num: usize,

crates/cli/tests/wasm-bindgen/main.rs

@@ -135,6 +135,34 @@ fn works_on_empty_project() {
     cmd.assert().success();
 }
 
+#[test]
+fn namespace_global_and_noglobal_works() {
+    let (mut cmd, _out_dir) = Project::new("namespace_global_and_noglobal_works")
+        .file(
+            "src/lib.rs",
+            r#"
+                use wasm_bindgen::prelude::*;
+                #[wasm_bindgen(module = "fs")]
+                extern "C" {
+                    #[wasm_bindgen(js_namespace = window)]
+                    fn t1();
+                }
+                #[wasm_bindgen]
+                extern "C" {
+                    #[wasm_bindgen(js_namespace = window)]
+                    fn t2();
+                }
+                #[wasm_bindgen]
+                pub fn test() {
+                    t1();
+                    t2();
+                }
+            "#,
+        )
+        .wasm_bindgen("");
+    cmd.assert().success();
+}
+
 mod npm;
 
 #[test]

tests/wasm/imports.js

@@ -127,3 +127,13 @@ exports.receive_some = val => {
 };
 
 exports.get_some_val = () => VAL;
+
+exports.Math = {
+  func_from_module_math: (a) => a * 2
+}
+
+exports.same_name_from_import = (a) => a * 3;
+
+exports.same_js_namespace_from_module = {
+  func_from_module_1_same_js_namespace: (a) => a * 5
+}

tests/wasm/imports.rs

@@ -69,11 +69,32 @@ extern "C" {
     fn receive_some_ref(arg: Option<&PassOutOptionUndefined>);
     #[wasm_bindgen(js_name = "receive_some")]
     fn receive_some_owned(arg: Option<PassOutOptionUndefined>);
+
+    #[wasm_bindgen(js_namespace = Math)]
+    fn func_from_module_math(a: i32) -> i32;
+
+    #[wasm_bindgen(js_name = "same_name_from_import")]
+    fn same_name_from_import_1(s: i32) -> i32;
+
+    #[wasm_bindgen(js_namespace = same_js_namespace_from_module)]
+    fn func_from_module_1_same_js_namespace(s: i32) -> i32;
+}
+
+#[wasm_bindgen(module = "tests/wasm/imports_2.js")]
+extern "C" {
+    #[wasm_bindgen(js_name = "same_name_from_import")]
+    fn same_name_from_import_2(s: i32) -> i32;
+
+    #[wasm_bindgen(js_namespace = same_js_namespace_from_module)]
+    fn func_from_module_2_same_js_namespace(s: i32) -> i32;
 }
 
 #[wasm_bindgen]
 extern "C" {
     fn parseInt(a: &str) -> u32;
+
+    #[wasm_bindgen(js_namespace = Math, js_name = "sqrt")]
+    fn func_from_global_math(s: f64) -> f64;
 }
 
 #[wasm_bindgen_test]
@@ -274,3 +295,21 @@ fn pass_out_options_as_undefined() {
     receive_some_owned(Some(v.clone()));
     receive_some_owned(Some(v));
 }
+
+#[wasm_bindgen_test]
+fn func_from_global_and_module_same_js_namespace() {
+    assert_eq!(func_from_global_math(4.0), 2.0);
+    assert_eq!(func_from_module_math(2), 4);
+}
+
+#[wasm_bindgen_test]
+fn func_from_two_modules_same_js_name() {
+    assert_eq!(same_name_from_import_1(1), 3);
+    assert_eq!(same_name_from_import_2(1), 4);
+}
+
+#[wasm_bindgen_test]
+fn func_from_two_modules_same_js_namespace() {
+    assert_eq!(func_from_module_1_same_js_namespace(2), 10);
+    assert_eq!(func_from_module_2_same_js_namespace(2), 12);
+}

tests/wasm/imports_2.js

@@ -0,0 +1,5 @@
+exports.same_name_from_import = (a) => a * 4;
+
+exports.same_js_namespace_from_module = {
+  func_from_module_2_same_js_namespace: (a) => a * 6
+}