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.

Sign up for GitHub

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

Wasm interface types and multi-value #1805

Merged
merged 2 commits into from
Oct 18, 2019
Merged

Wasm interface types and multi-value #1805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
311ae19
Wasm interface types should imply multi-value
fitzgen Oct 3, 2019
25dd84c
Get rid of the wasm interface types return pointer hacks
fitzgen Oct 3, 2019
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
2 changes: 1 addition & 1 deletion crates/cli-support/src/lib.rs
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ impl Bindgen {
weak_refs: env::var("WASM_BINDGEN_WEAKREF").is_ok(),
threads: threads_config(),
anyref: anyref || wasm_interface_types,
multi_value,
multi_value: multi_value || wasm_interface_types,
wasm_interface_types,
encode_into: EncodeInto::Test,
}
Expand Down
55 changes: 18 additions & 37 deletions crates/cli-support/src/webidl/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -954,7 +954,10 @@ impl<'a> Context<'a> {
match op.kind {
decode::OperationKind::Regular => {
if op.is_static {
Ok((AuxImport::ValueWithThis(class, function.name.to_string()), false))
Ok((
AuxImport::ValueWithThis(class, function.name.to_string()),
false,
))
} else if structural {
Ok((
AuxImport::StructuralMethod(function.name.to_string()),
Expand Down Expand Up @@ -1361,48 +1364,26 @@ impl<'a> Context<'a> {
.bindings
.get(bind.binding)
.ok_or_else(|| format_err!("bad binding id"))?;
let mut return_via_outptr = None;
let (wasm_ty, webidl_ty, incoming, outgoing) = match binding {
ast::FunctionBinding::Export(e) => {
// This `match` is weird, see the comment at the top of
// `standard.rs` for what it's doing.
let outgoing = match e.result.bindings.get(0) {
Some(ast::OutgoingBindingExpression::As(a)) if a.idx == u32::max_value() => {
return_via_outptr = Some(vec![walrus::ValType::I32, walrus::ValType::I32]);
&e.result.bindings[1..]
}
_ => &e.result.bindings[..],
};
(
e.wasm_ty,
e.webidl_ty,
e.params.bindings.as_slice(),
outgoing,
)
}
ast::FunctionBinding::Import(e) => {
// This `match` is weird, see the comment at the top of
// `standard.rs` for what it's doing.
let incoming = match e.result.bindings.get(0) {
Some(ast::IncomingBindingExpression::Get(g)) if g.idx == u32::max_value() => {
return_via_outptr = Some(vec![walrus::ValType::I32, walrus::ValType::I32]);
&e.result.bindings[1..]
}
_ => &e.result.bindings[..],
};
(
e.wasm_ty,
e.webidl_ty,
incoming,
e.params.bindings.as_slice(),
)
}
ast::FunctionBinding::Export(e) => (
e.wasm_ty,
e.webidl_ty,
e.params.bindings.as_slice(),
&e.result.bindings[..],
),
ast::FunctionBinding::Import(e) => (
e.wasm_ty,
e.webidl_ty,
&e.result.bindings[..],
e.params.bindings.as_slice(),
),
};
let webidl_ty = standard::copy_ty(&mut self.bindings.types, webidl_ty, &std.types);
let webidl_ty = match webidl_ty {
ast::WebidlTypeRef::Id(id) => <ast::WebidlFunction as ast::WebidlTypeId>::wrap(id),
_ => bail!("invalid webidl type listed"),
};

Ok(Binding {
wasm_ty,
webidl_ty,
Expand All @@ -1416,7 +1397,7 @@ impl<'a> Context<'a> {
.cloned()
.map(NonstandardOutgoing::Standard)
.collect(),
return_via_outptr,
return_via_outptr: None,
})
}

Expand Down
62 changes: 4 additions & 58 deletions crates/cli-support/src/webidl/standard.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,39 +16,6 @@
//! generating any JS glue. Any JS glue currently generated is also invalid if
//! the module contains the wasm bindings section and it's actually respected.
// NB: Returning strings is weird
//
// This module has what is currently a pretty gross hack for dealing with
// returning strings. One of the banner features of WebIDL bindings is not
// requiring any language-specific glue to use wasm files and you get all sorts
// of types like integers and strings by default. Note that strings are a huge
// thing here.
//
// Dealing with *incoming* strings is easy enough in that the binding expression
// has an allocator function to call and it's filled in and passed as two
// pointers. Outgoing strings are a little harder, however, for two reasons:
//
// * One is that we need to return two values, which requires multi-value
// * Another is that someone's got to free the string at some point
//
// Rust/wasm-bindgen don't support multi-value, and WebIDL bindings as literally
// spec'd right this red hot second don't support freeing strings that we pass
// out. These both have obvious fixes (supporting multi-value and extending the
// bindings spec to support freeing) but we also want something working right
// this red-hot second.
//
// To get things working we employ a terrible hack where the first bindign
// expression of the result a function may indicate "use a thing that's way off
// in the ether" and that's actually a sentinel for "oh ignore everything else
// and the string is returned through an out-ptr as the first argument". This
// manifests in all sorts of special hacks all over the place to handle this,
// and it's a real bummer.
//
// This is in general just an explainer for the current state of things and
// also a preemptive apology for writing the code to handle this in so many
// places. I, like you, look forward to actually fixing this for real as the
// spec continues to evolve and we implement more in wasm-bindgen.

use crate::descriptor::VectorKind;
use crate::webidl::{AuxExportKind, AuxImport, AuxValue, JsImport, JsImportName};
use crate::webidl::{NonstandardIncoming, NonstandardOutgoing};
Expand Down Expand Up @@ -240,26 +207,15 @@ pub fn add_section(
name
)
})?;
let mut result = extract_outgoing(&binding.outgoing).with_context(|_| {
let result = extract_outgoing(&binding.outgoing).with_context(|_| {
format!(
"failed to map return value for export `{}` to standard \
binding expressions",
name
)
})?;

// see comment at top of this module about returning strings for what
// this is doing and why it's weird
if binding.return_via_outptr.is_some() {
result.insert(
0,
ast::OutgoingBindingExpressionAs {
idx: u32::max_value(),
ty: ast::WebidlScalarType::Long.into(),
}
.into(),
);
}
assert!(binding.return_via_outptr.is_none());
let binding = section.bindings.insert(ast::ExportBinding {
wasm_ty: binding.wasm_ty,
webidl_ty: copy_ty(
Expand Down Expand Up @@ -293,24 +249,14 @@ pub fn add_section(
module_name, name,
)
})?;
let mut result = extract_incoming(&binding.incoming).with_context(|_| {
let result = extract_incoming(&binding.incoming).with_context(|_| {
format!(
"failed to map return value of import `{}::{}` to standard \
binding expressions",
module_name, name,
)
})?;
// see comment at top of this module about returning strings for what
// this is doing and why it's weird
if binding.return_via_outptr.is_some() {
result.insert(
0,
ast::IncomingBindingExpressionGet {
idx: u32::max_value(),
}
.into(),
);
}
assert!(binding.return_via_outptr.is_none());
let binding = section.bindings.insert(ast::ImportBinding {
wasm_ty: binding.wasm_ty,
webidl_ty: copy_ty(
Expand Down