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

try to fix global / modulaized import ns conflict #2057

Merged
merged 10 commits into from
Apr 15, 2020
137 changes: 77 additions & 60 deletions crates/cli-support/src/js/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -1920,6 +1920,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();
Expand All @@ -1930,30 +1942,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);
add_module_import(module.clone(), name, &unique_name);
let unique_name = self.generate_identifier(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
}

Expand All @@ -1965,8 +1964,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);
add_module_import(module, name, &unique_name);
let unique_name = self.generate_identifier(name);
self.add_module_import(module, name, &unique_name);
unique_name
}

Expand Down Expand Up @@ -1995,13 +1994,7 @@ impl<'a> Context<'a> {
format!("l{}", name)
}

JsImportName::Global { name } => {
let unique_name = generate_identifier(name, &mut self.defined_identifiers);
if unique_name != *name {
bail!("cannot import `{}` from two locations", name);
}
unique_name
}
JsImportName::Global { name } => name.to_string(),
};
self.imported_names
.insert(import.name.clone(), name.clone());
Expand Down Expand Up @@ -2053,6 +2046,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,
Expand Down Expand Up @@ -2082,6 +2076,47 @@ impl<'a> Context<'a> {
Ok(())
}

fn prestore_global_import_identifiers(&mut self) -> Result<(), Error> {
for (_id, adapter) in crate::sorted_iter(&self.wit.adapters) {
let instrs = match &adapter.kind {
AdapterKind::Import { .. } => continue,
AdapterKind::Local { instructions } => instructions,
};
let mut call = None;
for instr in instrs {
match instr.instr {
Instruction::CallAdapter(id) => {
if call.is_some() {
continue;
} else {
call = Some(id);
}
}
Instruction::CallExport(_)
| Instruction::CallTableElement(_)
| Instruction::Standard(wit_walrus::Instruction::CallCore(_))
| Instruction::Standard(wit_walrus::Instruction::CallAdapter(_)) => continue,
_ => {}
}
}
if let Some(id) = call {
let js = match &self.aux.import_map[&id] {
AuxImport::Value(AuxValue::Bare(js)) => Some(js),
_ => None,
};
if let Some(js) = js {
match &js.name {
JsImportName::Global { name } => {
self.generate_identifier(name);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will still want to forward to import_name which does caching and error handling, it should just only call that method with JsImportName::Global values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sry but I don't quite understand what you suggest by "it should just only call that method with JsImportName::Global values." Could you please explain a little bit more?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh so my point is that I don't think we want to call generate_identifier as a bare function since it always generates a new identifier. This means that if the same global is referenced multiple times it'll throw off generation. Instead this should use import_name which has error handling and caching which should be used instead of the raw generate_identifier

}
_ => {}
}
}
}
}
Ok(())
}

fn generate_adapter(
&mut self,
id: AdapterId,
Expand Down Expand Up @@ -3118,6 +3153,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(
Expand Down Expand Up @@ -3165,18 +3215,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 {
Expand Down Expand Up @@ -3269,27 +3307,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,
Expand Down
28 changes: 28 additions & 0 deletions crates/cli/tests/wasm-bindgen/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
10 changes: 10 additions & 0 deletions tests/wasm/imports.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
39 changes: 39 additions & 0 deletions tests/wasm/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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);
}
5 changes: 5 additions & 0 deletions tests/wasm/imports_2.js
Original file line number Diff line number Diff line change
@@ -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
}