Merge pull request #870 from alexcrichton/no-constructor-token

Remove the need for a `ConstructorToken`

crates/backend/src/ast.rs

@@ -34,11 +34,9 @@ pub struct Export {
     pub class: Option<Ident>,
     /// The type of `self` (either `self`, `&self`, or `&mut self`)
     pub method_self: Option<MethodSelf>,
-    /// The name of the constructor function (e.g. new).
+    /// Whether or not this export is flagged as a constructor, returning an
-    ///
+    /// instance of the `impl` type
-    /// This allows javascript to expose an `Object` interface, where calling
+    pub is_constructor: bool,
-    /// the constructor maps correctly to rust.
-    pub constructor: Option<String>,
     /// The rust function
     pub function: Function,
     /// Comments extracted from the rust source.
@@ -326,7 +324,7 @@ impl Export {
             class: self.class.as_ref().map(|s| s.to_string()),
             method,
             consumed,
-            constructor: self.constructor.clone(),
+            is_constructor: self.is_constructor,
             function: self.function.shared(),
             comments: self.comments.clone(),
         }

crates/cli-support/src/js/js2rust.rs

@@ -37,6 +37,14 @@ pub struct Js2Rust<'a, 'b: 'a> {
     /// Name of the JS shim/function that we're generating, primarily for
     /// TypeScript right now.
     js_name: String,
+
+    /// whether or not this generated function body will act like a constructor,
+    /// meaning it doesn't actually return something but rather assigns to
+    /// `this`
+    ///
+    /// The string value here is the class that this should be a constructor
+    /// for.
+    constructor: Option<String>,
 }
 
 impl<'a, 'b> Js2Rust<'a, 'b> {
@@ -51,6 +59,7 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
             arg_idx: 0,
             ret_ty: String::new(),
             ret_expr: String::new(),
+            constructor: None,
         }
     }
 
@@ -64,6 +73,11 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
         Ok(self)
     }
 
+    pub fn constructor(&mut self, class: Option<&str>) -> &mut Self {
+        self.constructor = class.map(|s| s.to_string());
+        self
+    }
+
     /// Flag this shim as a method call into Rust, so the first Rust argument
     /// passed should be `this.ptr`.
     pub fn method(&mut self, method: bool, consumed: bool) -> &mut Self {
@@ -393,6 +407,32 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
     }
 
     pub fn ret(&mut self, ty: &Descriptor) -> Result<&mut Self, Error> {
+        if let Some(name) = ty.rust_struct() {
+            match &self.constructor {
+                Some(class) if class == name => {
+                    self.ret_expr = format!("this.ptr = RET;");
+                    if self.cx.config.weak_refs {
+                        self.ret_expr.push_str(&format!("\
+                            addCleanup(this, this.ptr, free{});
+                        ", name));
+                    }
+                }
+                Some(class) => {
+                    bail!("constructor for `{}` cannot return `{}`", class, name)
+                }
+                None => {
+                    self.ret_ty = name.to_string();
+                    self.cx.require_class_wrap(name);
+                    self.ret_expr = format!("return {name}.__wrap(RET);", name = name);
+                }
+            }
+            return Ok(self);
+        }
+
+        if self.constructor.is_some() {
+            bail!("constructor functions must return a Rust structure")
+        }
+
         if let Descriptor::Unit = ty {
             self.ret_ty = "void".to_string();
             self.ret_expr = format!("return RET;");
@@ -551,7 +591,8 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
 
         if let Some(name) = ty.rust_struct() {
             self.ret_ty = name.to_string();
-            self.ret_expr = format!("return {name}.__construct(RET);", name = name);
+            self.cx.require_class_wrap(name);
+            self.ret_expr = format!("return {name}.__wrap(RET);", name = name);
             return Ok(self);
         }
 

crates/cli-support/src/js/mod.rs

@@ -54,7 +54,8 @@ pub struct ExportedClass {
     comments: String,
     contents: String,
     typescript: String,
-    constructor: Option<String>,
+    has_constructor: bool,
+    wrap_needed: bool,
     fields: Vec<ClassField>,
 }
 
@@ -532,11 +533,7 @@ impl<'a> Context<'a> {
             // `drop()` the `WeakRef` (cancel finalization) whenever it is
             // finalized.
             self.expose_cleanup_groups();
-            let mk = format!("
+            let mk = format!("addCleanup(this, this.ptr, free{});", name);
-                const cleanup_ptr = this.ptr;
-                const ref = CLEANUPS.makeRef(this, () => free{}(cleanup_ptr));
-                CLEANUPS_MAP.set(this.ptr, ref);
-            ", name);
             let free = "
                 CLEANUPS_MAP.get(ptr).drop();
                 CLEANUPS_MAP.delete(ptr);
@@ -546,73 +543,28 @@ impl<'a> Context<'a> {
             (String::new(), "")
         };
 
-        if self.config.debug || class.constructor.is_some() {
+        if self.config.debug && !class.has_constructor {
-            self.expose_constructor_token();
+            dst.push_str(
-
-            dst.push_str(&format!(
                 "
-                static __construct(ptr) {{
+                    constructor() {
-                    return new {}(new ConstructorToken(ptr));
+                        throw new Error('cannot invoke `new` directly');
-                }}
+                    }
-
-                constructor(...args) {{
-                    if (args.length === 1 && args[0] instanceof ConstructorToken) {{
-                        this.ptr = args[0].ptr;
-                        return;
-                    }}
-                ",
-                name
-            ));
-
-            if let Some(ref constructor) = class.constructor {
-                ts_dst.push_str(&format!("constructor(...args: any[]);\n"));
-
-                dst.push_str(&format!(
-                    "
-                    // This invocation of new will call this constructor with a ConstructorToken
-                    let instance = {class}.{constructor}(...args);
-                    this.ptr = instance.ptr;
-                    {mkweakref}
-                    ",
-                    class = name,
-                    constructor = constructor,
-                    mkweakref = mkweakref,
-                ));
-            } else {
-                dst.push_str(
-                    "throw new Error('you cannot invoke `new` directly without having a \
-                     method annotated a constructor');\n",
-                );
-            }
-
-            dst.push_str("}");
-        } else {
-            dst.push_str(&format!(
                 "
-                static __construct(ptr) {{
+            );
-                    return new {}(ptr);
-                }}
-
-                constructor(ptr) {{
-                    this.ptr = ptr;
-                    {}
-                }}
-                ",
-                name,
-                mkweakref,
-            ));
         }
 
+        let mut wrap_needed = class.wrap_needed;
         let new_name = shared::new_function(&name);
         if self.wasm_import_needed(&new_name) {
             self.expose_add_heap_object();
+            wrap_needed = true;
 
             self.export(
                 &new_name,
                 &format!(
                     "
                     function(ptr) {{
-                        return addHeapObject({}.__construct(ptr));
+                        return addHeapObject({}.__wrap(ptr));
                     }}
                     ",
                     name
@@ -621,6 +573,21 @@ impl<'a> Context<'a> {
             );
         }
 
+        if wrap_needed {
+            dst.push_str(&format!(
+                "
+                static __wrap(ptr) {{
+                    const obj = Object.create({}.prototype);
+                    obj.ptr = ptr;
+                    {}
+                    return obj;
+                }}
+                ",
+                name,
+                mkweakref.replace("this", "obj"),
+            ));
+        }
+
         for field in class.fields.iter() {
             let wasm_getter = shared::struct_field_get(name, &field.name);
             let wasm_setter = shared::struct_field_set(name, &field.name);
@@ -1165,22 +1132,6 @@ impl<'a> Context<'a> {
         );
     }
 
-    fn expose_constructor_token(&mut self) {
-        if !self.exposed_globals.insert("ConstructorToken") {
-            return;
-        }
-
-        self.global(
-            "
-            class ConstructorToken {
-                constructor(ptr) {
-                    this.ptr = ptr;
-                }
-            }
-            ",
-        );
-    }
-
     fn expose_get_string_from_wasm(&mut self) {
         if !self.exposed_globals.insert("get_string_from_wasm") {
             return;
@@ -1717,6 +1668,11 @@ impl<'a> Context<'a> {
             "
                 const CLEANUPS = new WeakRefGroup(x => x.holdings());
                 const CLEANUPS_MAP = new Map();
+
+                function addCleanup(obj, ptr, free) {
+                    const ref = CLEANUPS.makeRef(obj, () => free(ptr));
+                    CLEANUPS_MAP.set(ptr, ref);
+                }
             "
         );
     }
@@ -1785,6 +1741,13 @@ impl<'a> Context<'a> {
         self.memory_init = Some(mem.limits().clone());
         "memory"
     }
+
+    fn require_class_wrap(&mut self, class: &str) {
+        self.exported_classes
+            .entry(class.to_string())
+            .or_insert_with(ExportedClass::default)
+            .wrap_needed = true;
+    }
 }
 
 impl<'a, 'b> SubContext<'a, 'b> {
@@ -1857,8 +1820,14 @@ impl<'a, 'b> SubContext<'a, 'b> {
             Some(d) => d,
         };
 
-        let (js, ts, js_doc) = Js2Rust::new(&export.function.name, self.cx)
+        let function_name = if export.is_constructor {
+            "constructor"
+        } else {
+            &export.function.name
+        };
+        let (js, ts, js_doc) = Js2Rust::new(function_name, self.cx)
             .method(export.method, export.consumed)
+            .constructor(if export.is_constructor { Some(class_name) } else { None })
             .process(descriptor.unwrap_function())?
             .finish("", &format!("wasm.{}", wasm_name));
 
@@ -1870,25 +1839,19 @@ impl<'a, 'b> SubContext<'a, 'b> {
         class
             .contents
             .push_str(&format_doc_comments(&export.comments, Some(js_doc)));
-        if !export.method {
+
+        if export.is_constructor {
+            if class.has_constructor {
+                bail!("found duplicate constructor `{}`",
+                      export.function.name);
+            }
+            class.has_constructor = true;
+        } else if !export.method {
             class.contents.push_str("static ");
             class.typescript.push_str("static ");
         }
 
-        let constructors: Vec<String> = self
+        class.contents.push_str(function_name);
-            .program
-            .exports
-            .iter()
-            .filter(|x| x.class == Some(class_name.to_string()))
-            .filter_map(|x| x.constructor.clone())
-            .collect();
-
-        class.constructor = match constructors.len() {
-            0 => None,
-            1 => Some(constructors[0].clone()),
-            x @ _ => bail!("there must be only one constructor, not {}", x),
-        };
-        class.contents.push_str(&export.function.name);
         class.contents.push_str(&js);
         class.contents.push_str("\n");
         class.typescript.push_str(&ts);

crates/cli-support/src/js/rust2js.rs

@@ -232,7 +232,8 @@ impl<'a, 'b> Rust2Js<'a, 'b> {
             if arg.is_by_ref() {
                 bail!("cannot invoke JS functions with custom ref types yet")
             }
-            let assign = format!("let c{0} = {1}.__construct({0});", abi, class);
+            self.cx.require_class_wrap(class);
+            let assign = format!("let c{0} = {1}.__wrap({0});", abi, class);
             self.prelude(&assign);
             self.js_arguments.push(format!("c{}", abi));
             return Ok(());

crates/cli/src/bin/wasm-bindgen-test-runner/server.rs

@@ -29,7 +29,7 @@ pub fn spawn(
             // native ESM support for wasm modules.
             await wasm.booted;
 
-            const cx = Context.new();
+            const cx = new Context();
             window.console_log_redirect = __wbgtest_console_log;
             window.console_error_redirect = __wbgtest_console_error;
 

crates/macro-support/src/parser.rs

@@ -733,7 +733,7 @@ impl<'a> MacroParse<(Option<BindgenAttrs>, &'a mut TokenStream)> for syn::Item {
                 program.exports.push(ast::Export {
                     class: None,
                     method_self: None,
-                    constructor: None,
+                    is_constructor: false,
                     comments,
                     rust_name: f.ident.clone(),
                     function: f.convert(opts)?,
@@ -856,12 +856,6 @@ impl<'a, 'b> MacroParse<()> for (&'a Ident, &'b mut syn::ImplItem) {
         let opts = BindgenAttrs::find(&mut method.attrs)?;
         let comments = extract_doc_comments(&method.attrs);
         let is_constructor = opts.constructor();
-        let constructor = if is_constructor {
-            Some(method.sig.ident.to_string())
-        } else {
-            None
-        };
-
         let (function, method_self) = function_from_decl(
             &method.sig.ident,
             &opts,
@@ -875,7 +869,7 @@ impl<'a, 'b> MacroParse<()> for (&'a Ident, &'b mut syn::ImplItem) {
         program.exports.push(ast::Export {
             class: Some(class.clone()),
             method_self,
-            constructor,
+            is_constructor,
             function,
             comments,
             rust_name: method.sig.ident.clone(),

crates/shared/src/lib.rs

@@ -97,7 +97,7 @@ pub struct Export {
     pub class: Option<String>,
     pub method: bool,
     pub consumed: bool,
-    pub constructor: Option<String>,
+    pub is_constructor: bool,
     pub function: Function,
     pub comments: Vec<String>,
 }

tests/wasm/classes.js

@@ -2,7 +2,7 @@ const wasm = require('wasm-bindgen-test.js');
 const assert = require('assert');
 
 exports.js_simple = () => {
-    const r = wasm.ClassesSimple.new();
+    const r = new wasm.ClassesSimple();
     assert.strictEqual(r.add(0), 0);
     assert.strictEqual(r.add(1), 1);
     assert.strictEqual(r.add(1), 2);
@@ -70,7 +70,8 @@ exports.js_constructors = () => {
     assert.strictEqual(foo.get_number(), 1);
     foo.free();
 
-    const foo2 = wasm.ConstructorsFoo.new(2);
+    assert.strictEqual(wasm.ConstructorsBar.new, undefined);
+    const foo2 = new wasm.ConstructorsFoo(2);
     assert.strictEqual(foo2.get_number(), 2);
     foo2.free();
 
@@ -78,7 +79,8 @@ exports.js_constructors = () => {
     assert.strictEqual(bar.get_sum(), 7);
     bar.free();
 
-    const bar2 = wasm.ConstructorsBar.other_name(5, 6);
+    assert.strictEqual(wasm.ConstructorsBar.other_name, undefined);
+    const bar2 = new wasm.ConstructorsBar(5, 6);
     assert.strictEqual(bar2.get_sum(), 11);
     bar2.free();
 
@@ -121,12 +123,12 @@ exports.js_readonly_fields = () => {
 };
 
 exports.js_double_consume = () => {
-    const r = wasm.DoubleConsume.new();
+    const r = new wasm.DoubleConsume();
     assert.throws(() => r.consume(r), /Attempt to use a moved value/);
 };
 
 
 exports.js_js_rename = () => {
-    wasm.JsRename.new().bar();
+    (new wasm.JsRename()).bar();
     wasm.classes_foo();
 };