From 534cceafc8c702357bdae542fce25e374a7505d8 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 21 Sep 2018 17:29:50 -0700 Subject: [PATCH 1/2] Improve error message for `infer_setter_property` If the setter doesn't start with `set_*` then we currently panic, but panicking is bad! Instead let's thread through structured errors to make sure they make their way to the top --- crates/backend/src/ast.rs | 53 +++++++++++++-------- crates/macro-support/src/parser.rs | 42 ++++++++-------- crates/macro/ui-tests/invalid-setter.rs | 19 ++++++++ crates/macro/ui-tests/invalid-setter.stderr | 14 ++++++ 4 files changed, 90 insertions(+), 38 deletions(-) create mode 100644 crates/macro/ui-tests/invalid-setter.rs create mode 100644 crates/macro/ui-tests/invalid-setter.stderr diff --git a/crates/backend/src/ast.rs b/crates/backend/src/ast.rs index 406fb0a3c..6787c7583 100644 --- a/crates/backend/src/ast.rs +++ b/crates/backend/src/ast.rs @@ -167,6 +167,7 @@ pub struct ImportEnum { #[derive(Clone)] pub struct Function { pub name: String, + pub name_span: Span, pub arguments: Vec, pub ret: Option, pub rust_attrs: Vec, @@ -261,13 +262,20 @@ pub struct DictionaryField { impl Program { pub(crate) fn shared(&self) -> Result { + let mut errors = Vec::new(); + let mut imports = Vec::new(); + for import in self.imports.iter() { + match import.shared() { + Ok(i) => imports.push(i), + Err(e) => errors.push(e), + } + } + Diagnostic::from_vec(errors)?; Ok(shared::Program { exports: self.exports.iter().map(|a| a.shared()).collect(), structs: self.structs.iter().map(|a| a.shared()).collect(), enums: self.enums.iter().map(|a| a.shared()).collect(), - imports: self.imports.iter() - .map(|a| a.shared()) - .collect::>()?, + imports, version: shared::version(), schema_version: shared::SCHEMA_VERSION.to_string(), }) @@ -348,7 +356,7 @@ impl Import { Ok(shared::Import { module: self.module.clone(), js_namespace: self.js_namespace.as_ref().map(|s| s.to_string()), - kind: self.kind.shared(), + kind: self.kind.shared()?, }) } } @@ -364,13 +372,13 @@ impl ImportKind { } } - fn shared(&self) -> shared::ImportKind { - match *self { - ImportKind::Function(ref f) => shared::ImportKind::Function(f.shared()), + fn shared(&self) -> Result { + Ok(match *self { + ImportKind::Function(ref f) => shared::ImportKind::Function(f.shared()?), ImportKind::Static(ref f) => shared::ImportKind::Static(f.shared()), ImportKind::Type(ref f) => shared::ImportKind::Type(f.shared()), ImportKind::Enum(ref f) => shared::ImportKind::Enum(f.shared()), - } + }) } } @@ -383,16 +391,20 @@ impl ImportFunction { /// If the rust object has a `fn set_xxx(&mut self, MyType)` style method, get the name /// for a setter in javascript (in this case `xxx`, so you can write `obj.xxx = val`) - fn infer_setter_property(&self) -> String { + fn infer_setter_property(&self) -> Result { let name = self.function.name.to_string(); if !name.starts_with("set_") { - panic!("error: setters must start with `set_`, found: {}", name); + bail_span!( + syn::token::Pub(self.function.name_span), + "setters must start with `set_`, found: {}", + name, + ); } - name[4..].to_string() + Ok(name[4..].to_string()) } - fn shared(&self) -> shared::ImportFunction { - let shared_operation = |operation: &Operation| { + fn shared(&self) -> Result { + let shared_operation = |operation: &Operation| -> Result<_, Diagnostic> { let is_static = operation.is_static; let kind = match &operation.kind { OperationKind::Regular => shared::OperationKind::Regular, @@ -405,14 +417,17 @@ impl ImportFunction { OperationKind::Setter(s) => { let s = s.as_ref().map(|s| s.to_string()); shared::OperationKind::Setter( - s.unwrap_or_else(|| self.infer_setter_property()), - ) + match s { + Some(s) => s, + None => self.infer_setter_property()?, + } + ) } OperationKind::IndexingGetter => shared::OperationKind::IndexingGetter, OperationKind::IndexingSetter => shared::OperationKind::IndexingSetter, OperationKind::IndexingDeleter => shared::OperationKind::IndexingDeleter, }; - shared::Operation { is_static, kind } + Ok(shared::Operation { is_static, kind }) }; let method = match self.kind { @@ -424,7 +439,7 @@ impl ImportFunction { let kind = match kind { MethodKind::Constructor => shared::MethodKind::Constructor, MethodKind::Operation(op) => { - shared::MethodKind::Operation(shared_operation(op)) + shared::MethodKind::Operation(shared_operation(op)?) } }; Some(shared::MethodData { @@ -435,14 +450,14 @@ impl ImportFunction { ImportFunctionKind::Normal => None, }; - shared::ImportFunction { + Ok(shared::ImportFunction { shim: self.shim.to_string(), catch: self.catch, variadic: self.variadic, method, structural: self.structural, function: self.function.shared(), - } + }) } } diff --git a/crates/macro-support/src/parser.rs b/crates/macro-support/src/parser.rs index 76bee0335..eafc6de17 100644 --- a/crates/macro-support/src/parser.rs +++ b/crates/macro-support/src/parser.rs @@ -158,11 +158,11 @@ impl BindgenAttrs { } /// Get the first js_name attribute - fn js_name(&self) -> Option<&str> { + fn js_name(&self) -> Option<(&str, Span)> { self.attrs .iter() .filter_map(|a| match a { - BindgenAttr::JsName(s) => Some(&s[..]), + BindgenAttr::JsName(s, span) => Some((&s[..], *span)), _ => None, }).next() } @@ -221,7 +221,7 @@ pub enum BindgenAttr { IndexingDeleter, Structural, Readonly, - JsName(String), + JsName(String, Span), JsClass(String), Extends(Ident), Variadic, @@ -294,11 +294,14 @@ impl Parse for BindgenAttr { } if attr == "js_name" { input.parse::()?; - let val = match input.parse::() { - Ok(str) => str.value(), - Err(_) => input.parse::()?.0.to_string(), + let (val, span) = match input.parse::() { + Ok(str) => (str.value(), str.span()), + Err(_) => { + let ident = input.parse::()?.0; + (ident.to_string(), ident.span()) + } }; - return Ok(BindgenAttr::JsName(val)) + return Ok(BindgenAttr::JsName(val, span)) } Err(original.error("unknown attribute")) @@ -387,11 +390,9 @@ impl<'a> ConvertToAst<(BindgenAttrs, &'a Option)> for syn::ForeignItemFn self, (opts, module): (BindgenAttrs, &'a Option), ) -> Result { - let default_name = self.ident.to_string(); - let js_name = opts.js_name().unwrap_or(&default_name); - let wasm = function_from_decl( - js_name, + &self.ident, + &opts, self.decl.clone(), self.attrs.clone(), self.vis.clone(), @@ -516,7 +517,7 @@ impl<'a> ConvertToAst<(BindgenAttrs, &'a Option)> for syn::ForeignItemFn let data = (ns, &self.ident, module); format!( "__wbg_{}_{}", - js_name + wasm.name .chars() .filter(|c| c.is_ascii_alphanumeric()) .collect::(), @@ -544,6 +545,7 @@ impl ConvertToAst for syn::ForeignItemType { assert_not_variadic(&attrs, &self)?; let js_name = attrs .js_name() + .map(|s| s.0) .map_or_else(|| self.ident.to_string(), |s| s.to_string()); let shim = format!("__wbg_instanceof_{}_{}", self.ident, ShortHash(&self.ident)); Ok(ast::ImportKind::Type(ast::ImportType { @@ -569,7 +571,7 @@ impl<'a> ConvertToAst<(BindgenAttrs, &'a Option)> for syn::ForeignItemSt } assert_not_variadic(&opts, &self)?; let default_name = self.ident.to_string(); - let js_name = opts.js_name().unwrap_or(&default_name); + let js_name = opts.js_name().map(|p| p.0).unwrap_or(&default_name); let shim = format!( "__wbg_static_accessor_{}_{}", self.ident, @@ -604,15 +606,14 @@ impl ConvertToAst for syn::ItemFn { } assert_not_variadic(&attrs, &self)?; - let default_name = self.ident.to_string(); - let name = attrs.js_name().unwrap_or(&default_name); - Ok(function_from_decl(name, self.decl, self.attrs, self.vis, false, None)?.0) + Ok(function_from_decl(&self.ident, &attrs, self.decl, self.attrs, self.vis, false, None)?.0) } } /// Construct a function (and gets the self type if appropriate) for our AST from a syn function. fn function_from_decl( - name: &str, + decl_name: &syn::Ident, + opts: &BindgenAttrs, decl: Box, attrs: Vec, vis: syn::Visibility, @@ -683,9 +684,11 @@ fn function_from_decl( syn::ReturnType::Type(_, ty) => Some(replace_self(*ty)), }; + let js_name = opts.js_name(); Ok(( ast::Function { - name: name.to_string(), + name: js_name.map(|s| s.0.to_string()).unwrap_or(decl_name.to_string()), + name_span: js_name.map(|s| s.1).unwrap_or(decl_name.span()), arguments, ret, rust_vis: vis, @@ -859,7 +862,8 @@ impl<'a, 'b> MacroParse<()> for (&'a Ident, &'b mut syn::ImplItem) { }; let (function, method_self) = function_from_decl( - opts.js_name().unwrap_or(&method.sig.ident.to_string()), + &method.sig.ident, + &opts, Box::new(method.sig.decl.clone()), method.attrs.clone(), method.vis.clone(), diff --git a/crates/macro/ui-tests/invalid-setter.rs b/crates/macro/ui-tests/invalid-setter.rs new file mode 100644 index 000000000..dbf6d26c0 --- /dev/null +++ b/crates/macro/ui-tests/invalid-setter.rs @@ -0,0 +1,19 @@ +extern crate wasm_bindgen; + +use wasm_bindgen::prelude::*; + +#[wasm_bindgen] +extern "C" { + type A; + + #[wasm_bindgen(setter, method)] + fn a(this: &A, b: i32); + + #[wasm_bindgen(setter = x, method)] + fn b(this: &A, b: i32); + + #[wasm_bindgen(setter, method, js_name = x)] + fn c(this: &A, b: i32); +} + +fn main() {} diff --git a/crates/macro/ui-tests/invalid-setter.stderr b/crates/macro/ui-tests/invalid-setter.stderr new file mode 100644 index 000000000..b4e0d2f16 --- /dev/null +++ b/crates/macro/ui-tests/invalid-setter.stderr @@ -0,0 +1,14 @@ +error: setters must start with `set_`, found: a + --> $DIR/invalid-setter.rs:10:8 + | +10 | fn a(this: &A, b: i32); + | ^ + +error: setters must start with `set_`, found: x + --> $DIR/invalid-setter.rs:15:46 + | +15 | #[wasm_bindgen(setter, method, js_name = x)] + | ^ + +error: aborting due to 2 previous errors + From 75f005be23eca8b38d88ed56dc6dee52c6f174c7 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 21 Sep 2018 17:34:41 -0700 Subject: [PATCH 2/2] Support `#[wasm_bindgen(setter, js_name = ...)]` Previously we'd require the explicit `js_name` to *also* start with `set_`, but when explicitly specified it shouldn't be mangled at all! Closes #584 --- crates/backend/src/ast.rs | 9 +++++++++ crates/macro-support/src/parser.rs | 1 + crates/macro/ui-tests/invalid-setter.stderr | 8 +------- crates/webidl/src/util.rs | 4 +++- tests/wasm/import_class.rs | 6 ++++++ 5 files changed, 20 insertions(+), 8 deletions(-) diff --git a/crates/backend/src/ast.rs b/crates/backend/src/ast.rs index 6787c7583..51eabd563 100644 --- a/crates/backend/src/ast.rs +++ b/crates/backend/src/ast.rs @@ -168,6 +168,7 @@ pub struct ImportEnum { pub struct Function { pub name: String, pub name_span: Span, + pub renamed_via_js_name: bool, pub arguments: Vec, pub ret: Option, pub rust_attrs: Vec, @@ -393,6 +394,14 @@ impl ImportFunction { /// for a setter in javascript (in this case `xxx`, so you can write `obj.xxx = val`) fn infer_setter_property(&self) -> Result { let name = self.function.name.to_string(); + + // if `#[wasm_bindgen(js_name = "...")]` is used then that explicitly + // because it was hand-written anyway. + if self.function.renamed_via_js_name { + return Ok(name) + } + + // Otherwise we infer names based on the Rust function name. if !name.starts_with("set_") { bail_span!( syn::token::Pub(self.function.name_span), diff --git a/crates/macro-support/src/parser.rs b/crates/macro-support/src/parser.rs index eafc6de17..9ed487ec3 100644 --- a/crates/macro-support/src/parser.rs +++ b/crates/macro-support/src/parser.rs @@ -689,6 +689,7 @@ fn function_from_decl( ast::Function { name: js_name.map(|s| s.0.to_string()).unwrap_or(decl_name.to_string()), name_span: js_name.map(|s| s.1).unwrap_or(decl_name.span()), + renamed_via_js_name: js_name.is_some(), arguments, ret, rust_vis: vis, diff --git a/crates/macro/ui-tests/invalid-setter.stderr b/crates/macro/ui-tests/invalid-setter.stderr index b4e0d2f16..164224899 100644 --- a/crates/macro/ui-tests/invalid-setter.stderr +++ b/crates/macro/ui-tests/invalid-setter.stderr @@ -4,11 +4,5 @@ error: setters must start with `set_`, found: a 10 | fn a(this: &A, b: i32); | ^ -error: setters must start with `set_`, found: x - --> $DIR/invalid-setter.rs:15:46 - | -15 | #[wasm_bindgen(setter, method, js_name = x)] - | ^ - -error: aborting due to 2 previous errors +error: aborting due to previous error diff --git a/crates/webidl/src/util.rs b/crates/webidl/src/util.rs index 7ed04a2d0..055d3fc2d 100644 --- a/crates/webidl/src/util.rs +++ b/crates/webidl/src/util.rs @@ -4,7 +4,7 @@ use std::ptr; use backend; use backend::util::{ident_ty, leading_colon_path_ty, raw_ident, rust_ident}; use heck::{CamelCase, ShoutySnakeCase, SnakeCase}; -use proc_macro2::Ident; +use proc_macro2::{Ident, Span}; use syn; use weedle; use weedle::attribute::{ExtendedAttributeList, ExtendedAttribute}; @@ -304,6 +304,8 @@ impl<'src> FirstPassRecord<'src> { Some(backend::ast::ImportFunction { function: backend::ast::Function { name: js_name.to_string(), + name_span: Span::call_site(), + renamed_via_js_name: false, arguments, ret: ret.clone(), rust_attrs: vec![], diff --git a/tests/wasm/import_class.rs b/tests/wasm/import_class.rs index 64c97d142..d0ae1f21e 100644 --- a/tests/wasm/import_class.rs +++ b/tests/wasm/import_class.rs @@ -53,8 +53,12 @@ extern { fn new() -> RenameProperties; #[wasm_bindgen(getter = a, method)] fn test(this: &RenameProperties) -> i32; + #[wasm_bindgen(getter, method, js_name = a)] + fn test2(this: &RenameProperties) -> i32; #[wasm_bindgen(setter = a, method)] fn another(this: &RenameProperties, a: i32); + #[wasm_bindgen(setter, method, js_name = a)] + fn another2(this: &RenameProperties, a: i32); /// dox pub type AssertImportDenyDocsWorks; @@ -154,6 +158,8 @@ fn rename_setter_getter() { assert_eq!(a.test(), 1); a.another(2); assert_eq!(a.test(), 2); + a.another2(3); + assert_eq!(a.test2(), 3); } /// dox