Skip to content

webidl: Turn the [Throws] extended attributes into Result<T, JsValue> #457

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

Merged
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
23 changes: 22 additions & 1 deletion crates/backend/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,23 @@ pub fn raw_ident(name: &str) -> Ident {
/// Create a path type from the given segments. For example an iterator yielding
/// the idents `[foo, bar, baz]` will result in the path type `foo::bar::baz`.
pub fn simple_path_ty<I>(segments: I) -> syn::Type
where
I: IntoIterator<Item = Ident>,
{
path_ty(false, segments)
}

/// Create a global path type from the given segments. For example an iterator
/// yielding the idents `[foo, bar, baz]` will result in the path type
/// `::foo::bar::baz`.
pub fn leading_colon_path_ty<I>(segments: I) -> syn::Type
where
I: IntoIterator<Item = Ident>,
{
path_ty(true, segments)
}

fn path_ty<I>(leading_colon: bool, segments: I) -> syn::Type
where
I: IntoIterator<Item = Ident>,
{
Expand All @@ -49,7 +66,11 @@ where
syn::TypePath {
qself: None,
path: syn::Path {
leading_colon: None,
leading_colon: if leading_colon {
Some(Default::default())
} else {
None
},
segments: syn::punctuated::Punctuated::from_iter(segments),
},
}.into()
Expand Down
77 changes: 65 additions & 12 deletions crates/webidl/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ fn compile_ast(mut ast: backend::ast::Program) -> String {
let mut defined = BTreeSet::from_iter(
vec![
"str", "char", "bool", "JsValue", "u8", "i8", "u16", "i16", "u32", "i32", "u64", "i64",
"usize", "isize", "f32", "f64",
"usize", "isize", "f32", "f64", "Result",
].into_iter()
.map(|id| proc_macro2::Ident::new(id, proc_macro2::Span::call_site())),
);
Expand Down Expand Up @@ -210,19 +210,38 @@ impl<'a> WebidlParse<&'a webidl::ast::NonPartialInterface> for webidl::ast::Exte
) -> Result<()> {
let mut add_constructor = |arguments: &[webidl::ast::Argument], class: &str| {
let self_ty = ident_ty(rust_ident(&interface.name));

let kind = backend::ast::ImportFunctionKind::Method {
class: class.to_string(),
ty: self_ty.clone(),
kind: backend::ast::MethodKind::Constructor,
};

let structural = false;

// Constructors aren't annotated with `[Throws]` extended attributes
// (how could they be, since they themselves are extended
// attributes?) so we must conservatively assume that they can
// always throw.
//
// From https://heycam.github.io/webidl/#Constructor (emphasis
// mine):
//
// > The prose definition of a constructor must either return an IDL
// > value of a type corresponding to the interface the
// > `[Constructor]` extended attribute appears on, **or throw an
// > exception**.
let throws = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a little unfortunate :(

I wonder though if we can add configuration options to the generation to handle this? I'd expect that the majority of webidl constructors don't actually throw (or we're not interested in what they throw). We could perhaps explicitly whitelist some constructors as throwing but all others would be infallible?

Copy link
Member Author

@fitzgen fitzgen Jul 11, 2018

Choose a reason for hiding this comment

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

We could certainly add our own [WbgNoThrowConstructor] extended attribute if we get to a place where we want to explicitly whitelist some constructors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps yeah, although I was thinking moreso the build script rather than modifying the webidl files themselves, but either way's fine!

Copy link
Member

@ohanar ohanar Jul 11, 2018

Choose a reason for hiding this comment

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

It would be nice if WebIDL had a NoThrowsConstructor or something like that.

I think for now, we should probably just leave this as is, and maybe come back to some whitelist thing -- maybe make an issue about it?

Edit: Seems I was beat to the punch.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll file a follow up.

Copy link
Member Author

Choose a reason for hiding this comment

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


create_function(
"new",
arguments
.iter()
.map(|arg| (&*arg.name, &*arg.type_, arg.variadic)),
Some(self_ty),
kind,
false,
structural,
throws,
).map(|function| {
program.imports.push(backend::ast::Import {
module: None,
Expand All @@ -236,7 +255,8 @@ impl<'a> WebidlParse<&'a webidl::ast::NonPartialInterface> for webidl::ast::Exte
match self {
webidl::ast::ExtendedAttribute::ArgumentList(
webidl::ast::ArgumentListExtendedAttribute { arguments, name },
) if name == "Constructor" =>
)
if name == "Constructor" =>
{
add_constructor(arguments, &interface.name);
}
Expand All @@ -251,7 +271,8 @@ impl<'a> WebidlParse<&'a webidl::ast::NonPartialInterface> for webidl::ast::Exte
rhs_arguments,
rhs_name,
},
) if lhs_name == "NamedConstructor" =>
)
if lhs_name == "NamedConstructor" =>
{
add_constructor(rhs_arguments, rhs_name);
}
Expand Down Expand Up @@ -322,14 +343,27 @@ impl<'a> WebidlParse<&'a str> for webidl::ast::RegularAttribute {
}

let is_structural = util::is_structural(&self.extended_attributes);
let throws = util::throws(&self.extended_attributes);

create_getter(&self.name, &self.type_, self_name, false, is_structural)
.map(wrap_import_function)
create_getter(
&self.name,
&self.type_,
self_name,
false,
is_structural,
throws,
).map(wrap_import_function)
.map(|import| program.imports.push(import));

if !self.read_only {
create_setter(&self.name, &self.type_, self_name, false, is_structural)
.map(wrap_import_function)
create_setter(
&self.name,
&self.type_,
self_name,
false,
is_structural,
throws,
).map(wrap_import_function)
.map(|import| program.imports.push(import));
}

Expand All @@ -344,14 +378,27 @@ impl<'a> WebidlParse<&'a str> for webidl::ast::StaticAttribute {
}

let is_structural = util::is_structural(&self.extended_attributes);
let throws = util::throws(&self.extended_attributes);

create_getter(&self.name, &self.type_, self_name, true, is_structural)
.map(wrap_import_function)
create_getter(
&self.name,
&self.type_,
self_name,
true,
is_structural,
throws,
).map(wrap_import_function)
.map(|import| program.imports.push(import));

if !self.read_only {
create_setter(&self.name, &self.type_, self_name, true, is_structural)
.map(wrap_import_function)
create_setter(
&self.name,
&self.type_,
self_name,
true,
is_structural,
throws,
).map(wrap_import_function)
.map(|import| program.imports.push(import));
}

Expand All @@ -365,12 +412,15 @@ impl<'a> WebidlParse<&'a str> for webidl::ast::RegularOperation {
return Ok(());
}

let throws = util::throws(&self.extended_attributes);

create_basic_method(
&self.arguments,
self.name.as_ref(),
&self.return_type,
self_name,
false,
throws,
).map(wrap_import_function)
.map(|import| program.imports.push(import));

Expand All @@ -384,12 +434,15 @@ impl<'a> WebidlParse<&'a str> for webidl::ast::StaticOperation {
return Ok(());
}

let throws = util::throws(&self.extended_attributes);

create_basic_method(
&self.arguments,
self.name.as_ref(),
&self.return_type,
self_name,
true,
throws,
).map(wrap_import_function)
.map(|import| program.imports.push(import));

Expand Down
57 changes: 52 additions & 5 deletions crates/webidl/src/util.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::iter;
use std::iter::{self, FromIterator};

use backend;
use backend::util::{ident_ty, raw_ident, rust_ident, simple_path_ty};
use backend::util::{ident_ty, leading_colon_path_ty, raw_ident, rust_ident, simple_path_ty};
use heck::SnakeCase;
use proc_macro2::Ident;
use syn;
Expand Down Expand Up @@ -146,12 +146,40 @@ where
Some(res)
}

fn unit_ty() -> syn::Type {
syn::Type::Tuple(syn::TypeTuple {
paren_token: Default::default(),
elems: syn::punctuated::Punctuated::new(),
})
}

fn result_ty(t: syn::Type) -> syn::Type {
let js_value = leading_colon_path_ty(vec![rust_ident("wasm_bindgen"), rust_ident("JsValue")]);

let arguments = syn::PathArguments::AngleBracketed(syn::AngleBracketedGenericArguments {
colon2_token: None,
lt_token: Default::default(),
args: FromIterator::from_iter(vec![
syn::GenericArgument::Type(t),
syn::GenericArgument::Type(js_value),
]),
gt_token: Default::default(),
});

let ident = raw_ident("Result");
let seg = syn::PathSegment { ident, arguments };
let path: syn::Path = seg.into();
let ty = syn::TypePath { qself: None, path };
ty.into()
}

pub fn create_function<'a, I>(
name: &str,
arguments: I,
ret: Option<syn::Type>,
mut ret: Option<syn::Type>,
kind: backend::ast::ImportFunctionKind,
structural: bool,
catch: bool,
) -> Option<backend::ast::ImportFunction>
where
I: Iterator<Item = (&'a str, &'a webidl::ast::Type, bool)>,
Expand All @@ -163,6 +191,10 @@ where

let js_ret = ret.clone();

if catch {
ret = Some(ret.map_or_else(|| result_ty(unit_ty()), |ret| result_ty(ret)))
}

let shim = {
let ns = match kind {
backend::ast::ImportFunctionKind::Normal => "",
Expand All @@ -184,7 +216,7 @@ where
},
rust_name,
js_ret,
catch: false,
catch,
structural,
kind,
shim,
Expand All @@ -197,6 +229,7 @@ pub fn create_basic_method(
return_type: &webidl::ast::ReturnType,
self_name: &str,
is_static: bool,
catch: bool,
) -> Option<backend::ast::ImportFunction> {
let name = match name {
None => {
Expand Down Expand Up @@ -235,6 +268,7 @@ pub fn create_basic_method(
ret,
kind,
false,
catch,
)
}

Expand All @@ -244,6 +278,7 @@ pub fn create_getter(
self_name: &str,
is_static: bool,
is_structural: bool,
catch: bool,
) -> Option<backend::ast::ImportFunction> {
let ret = match webidl_ty_to_syn_ty(ty, TypePosition::Return) {
None => {
Expand All @@ -262,7 +297,7 @@ pub fn create_getter(
}),
};

create_function(name, iter::empty(), ret, kind, is_structural)
create_function(name, iter::empty(), ret, kind, is_structural, catch)
}

pub fn create_setter(
Expand All @@ -271,6 +306,7 @@ pub fn create_setter(
self_name: &str,
is_static: bool,
is_structural: bool,
catch: bool,
) -> Option<backend::ast::ImportFunction> {
let kind = backend::ast::ImportFunctionKind::Method {
class: self_name.to_string(),
Expand All @@ -287,6 +323,7 @@ pub fn create_setter(
None,
kind,
is_structural,
catch,
)
}

Expand Down Expand Up @@ -315,3 +352,13 @@ pub fn is_structural(attrs: &[Box<ExtendedAttribute>]) -> bool {
}
})
}

pub fn throws(attrs: &[Box<ExtendedAttribute>]) -> bool {
attrs.iter().any(|attr| {
if let ExtendedAttribute::NoArguments(webidl::ast::Other::Identifier(ref name)) = **attr {
name == "Throws"
} else {
false
}
})
}
1 change: 1 addition & 0 deletions crates/webidl/tests/all/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ extern crate wasm_bindgen_test_project_builder as project_builder;
use project_builder::project;

mod simple;
mod throws;
Loading