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

Skip generating JS import shims when unnecessary #1654

Merged
merged 7 commits into from
Jul 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/backend/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ pub struct ImportFunction {
pub catch: bool,
pub variadic: bool,
pub structural: bool,
pub assert_no_shim: bool,
pub kind: ImportFunctionKind,
pub shim: Ident,
pub doc_comment: Option<String>,
Expand Down
1 change: 1 addition & 0 deletions crates/backend/src/encode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ fn shared_import_function<'a>(
shim: intern.intern(&i.shim),
catch: i.catch,
method,
assert_no_shim: i.assert_no_shim,
structural: i.structural,
function: shared_function(&i.function, intern),
variadic: i.variadic,
Expand Down
2 changes: 1 addition & 1 deletion crates/cli-support/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,4 @@ wasm-bindgen-anyref-xform = { path = '../anyref-xform', version = '=0.2.48' }
wasm-bindgen-shared = { path = "../shared", version = '=0.2.48' }
wasm-bindgen-threads-xform = { path = '../threads-xform', version = '=0.2.48' }
wasm-bindgen-wasm-interpreter = { path = "../wasm-interpreter", version = '=0.2.48' }
wasm-webidl-bindings = "0.1.0"
wasm-webidl-bindings = "0.1.2"
89 changes: 79 additions & 10 deletions crates/cli-support/src/js/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::descriptor::VectorKind;
use crate::intrinsic::Intrinsic;
use crate::webidl;
use crate::webidl::{AuxEnum, AuxExport, AuxExportKind, AuxImport, AuxStruct};
use crate::webidl::{AuxValue, Binding};
use crate::webidl::{JsImport, JsImportName, NonstandardWebidlSection, WasmBindgenAux};
Expand Down Expand Up @@ -284,7 +285,10 @@ impl<'a> Context<'a> {
| OutputMode::Node {
experimental_modules: true,
} => {
imports.push_str(&format!("import * as wasm from './{}_bg.wasm';\n", module_name));
imports.push_str(&format!(
"import * as wasm from './{}_bg.wasm';\n",
module_name
));
for (id, js) in sorted_iter(&self.wasm_import_definitions) {
let import = self.module.imports.get_mut(*id);
import.module = format!("./{}.js", module_name);
Expand Down Expand Up @@ -723,6 +727,15 @@ impl<'a> Context<'a> {
self.global("function getObject(idx) { return heap[idx]; }");
}

fn expose_not_defined(&mut self) {
if !self.should_write_global("not_defined") {
return;
}
self.global(
"function notDefined(what) { return () => { throw new Error(`${what} is not defined`); }; }"
);
}

fn expose_assert_num(&mut self) {
if !self.should_write_global("assert_num") {
return;
Expand Down Expand Up @@ -1826,7 +1839,8 @@ impl<'a> Context<'a> {
for (id, import) in sorted_iter(&aux.import_map) {
let variadic = aux.imports_with_variadic.contains(&id);
let catch = aux.imports_with_catch.contains(&id);
self.generate_import(*id, import, bindings, variadic, catch)
let assert_no_shim = aux.imports_with_assert_no_shim.contains(&id);
self.generate_import(*id, import, bindings, variadic, catch, assert_no_shim)
.with_context(|_| {
format!("failed to generate bindings for import `{:?}`", import,)
})?;
Expand Down Expand Up @@ -1917,7 +1931,7 @@ impl<'a> Context<'a> {
let js_doc = builder.js_doc_comments();
let docs = format_doc_comments(&export.comments, Some(js_doc));

// Once we've got all the JS then put it in the right location dependin
// Once we've got all the JS then put it in the right location depending
// on what's being exported.
match &export.kind {
AuxExportKind::Function(name) => {
Expand Down Expand Up @@ -1965,22 +1979,77 @@ impl<'a> Context<'a> {
bindings: &NonstandardWebidlSection,
variadic: bool,
catch: bool,
assert_no_shim: bool,
) -> Result<(), Error> {
let binding = &bindings.imports[&id];
let webidl = bindings
.types
.get::<ast::WebidlFunction>(binding.webidl_ty)
.unwrap();
let mut builder = binding::Builder::new(self);
builder.catch(catch)?;
let js = builder.process(&binding, &webidl, false, &None, &mut |cx, prelude, args| {
cx.invoke_import(&binding, import, bindings, args, variadic, prelude)
})?;
let js = format!("function{}", js);
let js = match import {
AuxImport::Value(AuxValue::Bare(js))
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 that this may not be quite right in terms of matching where technically any value imported is a candidate for not requiring any glue. For example the other values, getters/setters, can all be hooked up directly in theory. They today, however, all require a method invocation style and will fail the Static check below very quickly, so it effectively ends up being the same.

Perhaps though it'd be good to refactor this in a forward-facing way where we match AuxImport::Value(v) here and then have shared code between way below in here about how to turn v into a JS snippet?

if !variadic && !catch && self.import_does_not_require_glue(binding, webidl) =>
{
self.expose_not_defined();
let name = self.import_name(js)?;
format!(
"typeof {name} == 'function' ? {name} : notDefined('{name}')",
name = name,
)
}
_ => {
if assert_no_shim {
panic!(
"imported function was annotated with `#[wasm_bindgen(assert_no_shim)]` \
but we need to generate a JS shim for it:\n\n\
\timport = {:?}\n\n\
\tbinding = {:?}\n\n\
\twebidl = {:?}",
import, binding, webidl,
);
}

let mut builder = binding::Builder::new(self);
builder.catch(catch)?;
let js = builder.process(
&binding,
&webidl,
false,
&None,
&mut |cx, prelude, args| {
cx.invoke_import(&binding, import, bindings, args, variadic, prelude)
},
)?;
format!("function{}", js)
}
};
self.wasm_import_definitions.insert(id, js);
Ok(())
}

fn import_does_not_require_glue(
&self,
binding: &Binding,
webidl: &ast::WebidlFunction,
) -> bool {
if !self.config.anyref && binding.contains_anyref(self.module) {
return false;
}

let wasm_ty = self.module.types.get(binding.wasm_ty);
webidl.kind == ast::WebidlFunctionKind::Static
&& webidl::outgoing_do_not_require_glue(
&binding.outgoing,
wasm_ty.params(),
&webidl.params,
)
&& webidl::incoming_do_not_require_glue(
&binding.incoming,
&webidl.result.into_iter().collect::<Vec<_>>(),
wasm_ty.results(),
)
}

/// Generates a JS snippet appropriate for invoking `import`.
///
/// This is generating code for `binding` where `bindings` has more type
Expand Down Expand Up @@ -2060,7 +2129,7 @@ impl<'a> Context<'a> {
ast::WebidlFunctionKind::Static => {
let js = match val {
AuxValue::Bare(js) => self.import_name(js)?,
_ => bail!("invalid import set for constructor"),
_ => bail!("invalid import set for free function"),
};
Ok(format!("{}({})", js, variadic_args(&args)?))
}
Expand Down
62 changes: 62 additions & 0 deletions crates/cli-support/src/webidl/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,17 @@ pub struct Binding {
pub return_via_outptr: Option<Vec<walrus::ValType>>,
}

impl Binding {
/// Does this binding's wasm function signature have any `anyref`s?
pub fn contains_anyref(&self, module: &walrus::Module) -> bool {
let ty = module.types.get(self.wasm_ty);
ty.params()
.iter()
.chain(ty.results())
.any(|ty| *ty == walrus::ValType::Anyref)
}
}

/// A synthetic custom section which is not standardized, never will be, and
/// cannot be serialized or parsed. This is synthesized from all of the
/// compiler-emitted wasm-bindgen sections and then immediately removed to be
Expand Down Expand Up @@ -152,6 +163,7 @@ pub struct WasmBindgenAux {
/// Small bits of metadata about imports.
pub imports_with_catch: HashSet<ImportId>,
pub imports_with_variadic: HashSet<ImportId>,
pub imports_with_assert_no_shim: HashSet<ImportId>,

/// Auxiliary information to go into JS/TypeScript bindings describing the
/// exported enums from Rust.
Expand Down Expand Up @@ -782,6 +794,7 @@ impl<'a> Context<'a> {
method,
structural,
function,
assert_no_shim,
} = function;
let (import_id, _id) = match self.function_imports.get(*shim) {
Some(pair) => *pair,
Expand All @@ -800,6 +813,9 @@ impl<'a> Context<'a> {
if *catch {
self.aux.imports_with_catch.insert(import_id);
}
if *assert_no_shim {
self.aux.imports_with_assert_no_shim.insert(import_id);
}

// Perform two functions here. First we're saving off our WebIDL
// bindings signature, indicating what we think our import is going to
Expand Down Expand Up @@ -1428,3 +1444,49 @@ fn concatenate_comments(comments: &[&str]) -> String {
.collect::<Vec<_>>()
.join("\n")
}

/// Do we need to generate JS glue shims for these incoming bindings?
pub fn incoming_do_not_require_glue(
exprs: &[NonstandardIncoming],
from_webidl_tys: &[ast::WebidlTypeRef],
to_wasm_tys: &[walrus::ValType],
) -> bool {
exprs.len() == from_webidl_tys.len()
&& exprs.len() == to_wasm_tys.len()
&& exprs
.iter()
.zip(from_webidl_tys)
.zip(to_wasm_tys)
.enumerate()
.all(|(i, ((expr, from_webidl_ty), to_wasm_ty))| match expr {
NonstandardIncoming::Standard(e) => e.is_expressible_in_js_without_webidl_bindings(
*from_webidl_ty,
*to_wasm_ty,
i as u32,
),
_ => false,
})
}

/// Do we need to generate JS glue shims for these outgoing bindings?
pub fn outgoing_do_not_require_glue(
exprs: &[NonstandardOutgoing],
from_wasm_tys: &[walrus::ValType],
to_webidl_tys: &[ast::WebidlTypeRef],
) -> bool {
exprs.len() == from_wasm_tys.len()
&& exprs.len() == to_webidl_tys.len()
&& exprs
.iter()
.zip(from_wasm_tys)
.zip(to_webidl_tys)
.enumerate()
.all(|(i, ((expr, from_wasm_ty), to_webidl_ty))| match expr {
NonstandardOutgoing::Standard(e) => e.is_expressible_in_js_without_webidl_bindings(
*from_wasm_ty,
*to_webidl_ty,
i as u32,
),
_ => false,
})
}
8 changes: 6 additions & 2 deletions crates/cli-support/src/webidl/outgoing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,12 @@ impl OutgoingBuilder<'_> {
Descriptor::U16 => self.standard_as(ValType::I32, ast::WebidlScalarType::UnsignedShort),
Descriptor::I32 => self.standard_as(ValType::I32, ast::WebidlScalarType::Long),
Descriptor::U32 => self.standard_as(ValType::I32, ast::WebidlScalarType::UnsignedLong),
Descriptor::F32 => self.standard_as(ValType::F32, ast::WebidlScalarType::Float),
Descriptor::F64 => self.standard_as(ValType::F64, ast::WebidlScalarType::Double),
Descriptor::F32 => {
self.standard_as(ValType::F32, ast::WebidlScalarType::UnrestrictedFloat)
}
Descriptor::F64 => {
self.standard_as(ValType::F64, ast::WebidlScalarType::UnrestrictedDouble)
}
Descriptor::Enum { .. } => self.standard_as(ValType::I32, ast::WebidlScalarType::Long),

Descriptor::Char => {
Expand Down
5 changes: 5 additions & 0 deletions crates/macro-support/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ macro_rules! attrgen {
(typescript_custom_section, TypescriptCustomSection(Span)),
(start, Start(Span)),
(skip, Skip(Span)),

// For testing purposes only.
(assert_no_shim, AssertNoShim(Span)),
}
};
}
Expand Down Expand Up @@ -496,8 +499,10 @@ impl<'a> ConvertToAst<(BindgenAttrs, &'a ast::ImportModule)> for syn::ForeignIte
return Err(Diagnostic::span_error(*span, msg));
}
}
let assert_no_shim = opts.assert_no_shim().is_some();
let ret = ast::ImportKind::Function(ast::ImportFunction {
function: wasm,
assert_no_shim,
kind,
js_ret,
catch,
Expand Down
1 change: 1 addition & 0 deletions crates/shared/src/lib.rs
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ macro_rules! shared_api {
shim: &'a str,
catch: bool,
variadic: bool,
assert_no_shim: bool,
method: Option<MethodData<'a>>,
structural: bool,
function: Function<'a>,
Expand Down
1 change: 1 addition & 0 deletions crates/webidl/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,7 @@ impl<'src> FirstPassRecord<'src> {
variadic,
catch,
structural,
assert_no_shim: false,
shim: {
let ns = match kind {
ast::ImportFunctionKind::Normal => "",
Expand Down
1 change: 1 addition & 0 deletions tests/wasm/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ pub mod imports;
pub mod js_objects;
pub mod jscast;
pub mod math;
pub mod no_shims;
pub mod node;
pub mod option;
pub mod optional_primitives;
Expand Down
10 changes: 5 additions & 5 deletions tests/wasm/math.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@ extern "C" {
// return value is always `f64` to faithfully capture what was sent to JS
// (what we're interested in) because all JS numbers fit in `f64` anyway.
// This is testing what happens when we pass numbers to JS and what it sees.
#[wasm_bindgen(js_name = roundtrip)]
#[wasm_bindgen(assert_no_shim, js_name = roundtrip)]
fn roundtrip_i8(a: i8) -> f64;
#[wasm_bindgen(js_name = roundtrip)]
#[wasm_bindgen(assert_no_shim, js_name = roundtrip)]
fn roundtrip_i16(a: i16) -> f64;
#[wasm_bindgen(js_name = roundtrip)]
#[wasm_bindgen(assert_no_shim, js_name = roundtrip)]
fn roundtrip_i32(a: i32) -> f64;
#[wasm_bindgen(js_name = roundtrip)]
#[wasm_bindgen(assert_no_shim, js_name = roundtrip)]
fn roundtrip_u8(a: u8) -> f64;
#[wasm_bindgen(js_name = roundtrip)]
#[wasm_bindgen(assert_no_shim, js_name = roundtrip)]
fn roundtrip_u16(a: u16) -> f64;
#[wasm_bindgen(js_name = roundtrip)]
fn roundtrip_u32(a: u32) -> f64;
Expand Down
Loading