Merge pull request #871 from alexcrichton/better-setters

Support `#[wasm_bindgen(setter, js_name = ...)]`

crates/backend/src/ast.rs

@@ -167,6 +167,8 @@ pub struct ImportEnum {
 #[derive(Clone)]
 pub struct Function {
     pub name: String,
+    pub name_span: Span,
+    pub renamed_via_js_name: bool,
     pub arguments: Vec<syn::ArgCaptured>,
     pub ret: Option<syn::Type>,
     pub rust_attrs: Vec<syn::Attribute>,
@@ -261,13 +263,20 @@ pub struct DictionaryField {
 
 impl Program {
     pub(crate) fn shared(&self) -> Result<shared::Program, Diagnostic> {
+        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()
+            imports,
-                .map(|a| a.shared())
-                .collect::<Result<_, Diagnostic>>()?,
             version: shared::version(),
             schema_version: shared::SCHEMA_VERSION.to_string(),
         })
@@ -348,7 +357,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 +373,13 @@ impl ImportKind {
         }
     }
 
-    fn shared(&self) -> shared::ImportKind {
+    fn shared(&self) -> Result<shared::ImportKind, Diagnostic> {
-        match *self {
+        Ok(match *self {
-            ImportKind::Function(ref f) => shared::ImportKind::Function(f.shared()),
+            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 +392,28 @@ 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<String, Diagnostic> {
         let name = self.function.name.to_string();
-        if !name.starts_with("set_") {
+
-            panic!("error: setters must start with `set_`, found: {}", name);
+        // 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)
         }
-        name[4..].to_string()
+
+        // Otherwise we infer names based on the Rust function name.
+        if !name.starts_with("set_") {
+            bail_span!(
+                syn::token::Pub(self.function.name_span),
+                "setters must start with `set_`, found: {}",
+                name,
+            );
+        }
+        Ok(name[4..].to_string())
     }
 
-    fn shared(&self) -> shared::ImportFunction {
+    fn shared(&self) -> Result<shared::ImportFunction, Diagnostic> {
-        let shared_operation = |operation: &Operation| {
+        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 +426,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 +448,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 +459,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(),
-        }
+        })
     }
 }
 

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::<Token![=]>()?;
-            let val = match input.parse::<syn::LitStr>() {
+            let (val, span) = match input.parse::<syn::LitStr>() {
-                Ok(str) => str.value(),
+                Ok(str) => (str.value(), str.span()),
-                Err(_) => input.parse::<AnyIdent>()?.0.to_string(),
+                Err(_) => {
+                    let ident = input.parse::<AnyIdent>()?.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<String>)> for syn::ForeignItemFn
         self,
         (opts, module): (BindgenAttrs, &'a Option<String>),
     ) -> Result<Self::Target, Diagnostic> {
-        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<String>)> for syn::ForeignItemFn
             let data = (ns, &self.ident, module);
             format!(
                 "__wbg_{}_{}",
-                js_name
+                wasm.name
                     .chars()
                     .filter(|c| c.is_ascii_alphanumeric())
                     .collect::<String>(),
@@ -544,6 +545,7 @@ impl ConvertToAst<BindgenAttrs> 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<String>)> 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<BindgenAttrs> for syn::ItemFn {
         }
         assert_not_variadic(&attrs, &self)?;
 
-        let default_name = self.ident.to_string();
+        Ok(function_from_decl(&self.ident, &attrs, self.decl, self.attrs, self.vis, false, None)?.0)
-        let name = attrs.js_name().unwrap_or(&default_name);
-        Ok(function_from_decl(name, 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<syn::FnDecl>,
     attrs: Vec<syn::Attribute>,
     vis: syn::Visibility,
@@ -683,9 +684,12 @@ 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()),
+            renamed_via_js_name: js_name.is_some(),
             arguments,
             ret,
             rust_vis: vis,
@@ -859,7 +863,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(),

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() {}

crates/macro/ui-tests/invalid-setter.stderr

@@ -0,0 +1,8 @@
+error: setters must start with `set_`, found: a
+  --> $DIR/invalid-setter.rs:10:8
+   |
+10 |     fn a(this: &A, b: i32);
+   |        ^
+
+error: aborting due to previous error
+

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![],

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