From 1624ac946f85b448a5222b2f644c10a223e19462 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 17 Jul 2018 15:52:16 -0700 Subject: [PATCH] Fix symbol conflicts defined in two crates Not a lot of attention has been paid to dealing with conflicts of symbols between crates and different `#[wasm_bindgen]` blocks. This commit starts to fix this issue by unblocking #486 which first ran into this. Currently there's a bug where if two independent crates bind the same JS API they'll generate the same symbol which causes conflicts for things like LTO or linking in general. This commit starts to add a "salt" to all symbols generated by `wasm-bindgen` (these are all transparent to the user) to ensure that each crate's invocations are kept apart from one another and using the correct bindings. --- crates/backend/src/codegen.rs | 31 +-------- crates/backend/src/util.rs | 41 ++++++++++++ crates/macro/src/parser.rs | 10 +-- tests/all/dependencies.rs | 117 ++++++++++++++++++++++++++++++++++ 4 files changed, 167 insertions(+), 32 deletions(-) diff --git a/crates/backend/src/codegen.rs b/crates/backend/src/codegen.rs index c9dc2f4dec0..09de4ecbc0c 100644 --- a/crates/backend/src/codegen.rs +++ b/crates/backend/src/codegen.rs @@ -1,6 +1,4 @@ -use std::borrow::Cow; use std::collections::HashSet; -use std::env; use std::sync::atomic::{AtomicUsize, Ordering, ATOMIC_USIZE_INIT}; use ast; @@ -9,24 +7,7 @@ use quote::ToTokens; use serde_json; use shared; use syn; - -fn to_ident_name(s: &str) -> Cow { - if s.chars().all(|c| match c { - 'a'...'z' | 'A'...'Z' | '0'...'9' | '_' => true, - _ => false, - }) { - return Cow::from(s); - } - - Cow::from( - s.chars() - .map(|c| match c { - 'a'...'z' | 'A'...'Z' | '0'...'9' | '_' => c, - _ => '_', - }) - .collect::(), - ) -} +use util::ShortHash; impl ToTokens for ast::Program { // Generate wrappers for all the items that we've found @@ -72,15 +53,9 @@ impl ToTokens for ast::Program { static CNT: AtomicUsize = ATOMIC_USIZE_INIT; - let crate_name = env::var("CARGO_PKG_NAME").expect("should have CARGO_PKG_NAME env var"); - let crate_vers = - env::var("CARGO_PKG_VERSION").expect("should have CARGO_PKG_VERSION env var"); - let generated_static_name = format!( - "__WASM_BINDGEN_GENERATED_{}_{}_{}", - to_ident_name(&crate_name), - to_ident_name(&crate_vers), - CNT.fetch_add(1, Ordering::SeqCst) + "__WASM_BINDGEN_GENERATED_{}", + ShortHash(CNT.fetch_add(1, Ordering::SeqCst)), ); let generated_static_name = Ident::new(&generated_static_name, Span::call_site()); diff --git a/crates/backend/src/util.rs b/crates/backend/src/util.rs index 9bdb96a37b0..50b1942ae17 100644 --- a/crates/backend/src/util.rs +++ b/crates/backend/src/util.rs @@ -1,4 +1,11 @@ +use std::collections::hash_map::DefaultHasher; +use std::env; +use std::fmt; +use std::hash::{Hash, Hasher}; use std::iter::FromIterator; +use std::sync::atomic::Ordering::SeqCst; +use std::sync::atomic::{AtomicBool, ATOMIC_BOOL_INIT}; +use std::sync::atomic::{AtomicUsize, ATOMIC_USIZE_INIT}; use ast; use proc_macro2::{self, Ident}; @@ -88,3 +95,37 @@ pub fn wrap_import_function(function: ast::ImportFunction) -> ast::Import { kind: ast::ImportKind::Function(function), } } + +/// Small utility used when generating symbol names. +/// +/// Hashes the public field here along with a few cargo-set env vars to +/// distinguish between runs of the procedural macro. +pub struct ShortHash(pub T); + +impl fmt::Display for ShortHash { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + static HASHED: AtomicBool = ATOMIC_BOOL_INIT; + static HASH: AtomicUsize = ATOMIC_USIZE_INIT; + + // Try to amortize the cost of loading env vars a lot as we're gonna be + // hashing for a lot of symbols. + if !HASHED.load(SeqCst) { + let mut h = DefaultHasher::new(); + env::var("CARGO_PKG_NAME") + .expect("should have CARGO_PKG_NAME env var") + .hash(&mut h); + env::var("CARGO_PKG_VERSION") + .expect("should have CARGO_PKG_VERSION env var") + .hash(&mut h); + // This may chop off 32 bits on 32-bit platforms, but that's ok, we + // just want something to mix in below anyway. + HASH.store(h.finish() as usize, SeqCst); + HASHED.store(true, SeqCst); + } + + let mut h = DefaultHasher::new(); + HASH.load(SeqCst).hash(&mut h); + self.0.hash(&mut h); + write!(f, "{:016x}", h.finish()) + } +} diff --git a/crates/macro/src/parser.rs b/crates/macro/src/parser.rs index 93d97f72cc1..0045678f8b4 100644 --- a/crates/macro/src/parser.rs +++ b/crates/macro/src/parser.rs @@ -1,4 +1,5 @@ -use backend::{ast, util::ident_ty}; +use backend::ast; +use backend::util::{ident_ty, ShortHash}; use proc_macro2::{Ident, Span, TokenStream, TokenTree}; use quote::ToTokens; use shared; @@ -427,10 +428,11 @@ impl ConvertToAst for syn::ForeignItemFn { let shim = { let ns = match kind { - ast::ImportFunctionKind::Normal => "n", - ast::ImportFunctionKind::Method { ref class, .. } => class, + ast::ImportFunctionKind::Normal => (0, "n"), + ast::ImportFunctionKind::Method { ref class, .. } => (1, &class[..]), }; - format!("__wbg_f_{}_{}_{}", js_name, self.ident, ns) + let data = (ns, &self.ident); + format!("__wbg_{}_{}", js_name, ShortHash(data)) }; ast::ImportKind::Function(ast::ImportFunction { function: wasm, diff --git a/tests/all/dependencies.rs b/tests/all/dependencies.rs index d4b48a780d7..372084d2a66 100644 --- a/tests/all/dependencies.rs +++ b/tests/all/dependencies.rs @@ -90,3 +90,120 @@ fn dependencies_work() { ) .test(); } + +#[test] +fn same_api_two_crates() { + project() + .file( + "src/lib.rs", + r#" + #![feature(use_extern_macros, wasm_custom_section, wasm_import_module)] + extern crate wasm_bindgen; + extern crate a; + extern crate b; + + use wasm_bindgen::prelude::*; + + #[wasm_bindgen(module = "./foo")] + extern { + fn assert_next_undefined(); + fn assert_next_ten(); + } + + #[wasm_bindgen] + pub fn test() { + assert_next_undefined(); + a::test(); + assert_next_ten(); + b::test(); + } + "#, + ) + .file( + "foo.js", + r#" + import { strictEqual } from "assert"; + + let next = null; + + export function assert_next_undefined() { + next = undefined; + } + + export function assert_next_ten() { + next = 10; + } + + export function foo(a) { + console.log(a, next); + strictEqual(a, next); + next = null; + } + "#, + ) + .add_local_dependency("a", "a") + .file( + "a/Cargo.toml", + &format!(r#" + [package] + name = 'a' + version = '0.0.0' + + [dependencies] + wasm-bindgen = {{ path = '{}' }} + "#, + env!("CARGO_MANIFEST_DIR") + ), + ) + .file( + "a/src/lib.rs", + " + #![feature(use_extern_macros, wasm_custom_section, wasm_import_module)] + extern crate wasm_bindgen; + + use wasm_bindgen::prelude::*; + + #[wasm_bindgen(module = \"./foo\")] + extern { + fn foo(); + } + + pub fn test() { + foo(); + } + ", + ) + .add_local_dependency("b", "b") + .file( + "b/Cargo.toml", + &format!(r#" + [package] + name = 'b' + version = '0.0.0' + + [dependencies] + wasm-bindgen = {{ path = '{}' }} + "#, + env!("CARGO_MANIFEST_DIR") + ), + ) + .file( + "b/src/lib.rs", + " + #![feature(use_extern_macros, wasm_custom_section, wasm_import_module)] + extern crate wasm_bindgen; + + use wasm_bindgen::prelude::*; + + #[wasm_bindgen(module = \"./foo\")] + extern { + fn foo(x: u32); + } + + pub fn test() { + foo(10); + } + ", + ) + .test(); +}