Remove class wrap for constructors in Rust exports (#3562)

* Remove class wrap for constructors in Rust exports After #1594 constructors of Rust exported structs started using class wrapping when generating JS shims. Wrapping erases prototype information from the object instance in JS and as a result it is not possible to override methods (via inheritance) of the generated class. Additionally, some checks to ensure constructors always return an instance of `Self` were lost. This PR is rebased from #3561, it passes the constructor information from the `Builder` into the instruction translation function which is then used to modify the generated bindings. The `return` statement is also removed instead the value is directly attached to the instance. Signed-off-by: Oliver T <[email protected]> * Fix typo Co-authored-by: Liam Murphy <[email protected]> * Disallow returning JS primitives from constructors Signed-off-by: Oliver T <[email protected]> * Added missing String in match Signed-off-by: Oliver T <[email protected]> * Handle nested descriptors Signed-off-by: Oliver T <[email protected]> --------- Signed-off-by: Oliver T <[email protected]> Co-authored-by: Liam Murphy <[email protected]>
rustwasm · Aug 25, 2023 · e7cfba5 · e7cfba5
1 parent 1b136e3
commit e7cfba5
Show file tree

Hide file tree

Showing 13 changed files with 308 additions and 7 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -55,6 +55,11 @@
   bindgen placeholder.
   [#3233](https://github.com/rustwasm/wasm-bindgen/pull/3233)
 
+* Changed constructor implementation in generated JS bindings, it is now
+  possible to override methods from generated JS classes using inheritance.
+  When exported constructors return `Self`.
+  [#3562](https://github.com/rustwasm/wasm-bindgen/pull/3562)
+
 ### Fixed
 
 * Fixed bindings and comments for `Atomics.wait`.
@@ -69,6 +74,9 @@
 * Use fully qualified paths in the `wasm_bindgen_test` macro.
   [#3549](https://github.com/rustwasm/wasm-bindgen/pull/3549)
 
+* Fixed bug allowing JS primitives to be returned from exported constructors.
+  [#3562](https://github.com/rustwasm/wasm-bindgen/pull/3562)
+
 ## [0.2.87](https://github.com/rustwasm/wasm-bindgen/compare/0.2.86...0.2.87)
 
 Released 2023-06-12.

diff --git a/crates/cli-support/src/js/binding.rs b/crates/cli-support/src/js/binding.rs
@@ -166,7 +166,12 @@ impl<'a, 'b> Builder<'a, 'b> {
         // more JIT-friendly. The generated code should be equivalent to the
         // wasm interface types stack machine, however.
         for instr in instructions {
-            instruction(&mut js, &instr.instr, &mut self.log_error)?;
+            instruction(
+                &mut js,
+                &instr.instr,
+                &mut self.log_error,
+                &self.constructor,
+            )?;
         }
 
         assert_eq!(js.stack.len(), adapter.results.len());
@@ -537,7 +542,12 @@ impl<'a, 'b> JsBuilder<'a, 'b> {
     }
 }
 
-fn instruction(js: &mut JsBuilder, instr: &Instruction, log_error: &mut bool) -> Result<(), Error> {
+fn instruction(
+    js: &mut JsBuilder,
+    instr: &Instruction,
+    log_error: &mut bool,
+    constructor: &Option<String>,
+) -> Result<(), Error> {
     match instr {
         Instruction::ArgGet(n) => {
             let arg = js.arg(*n).to_string();
@@ -987,18 +997,27 @@ fn instruction(js: &mut JsBuilder, instr: &Instruction, log_error: &mut bool) ->
         }
 
         Instruction::RustFromI32 { class } => {
-            js.cx.require_class_wrap(class);
             let val = js.pop();
-            js.push(format!("{}.__wrap({})", class, val));
+            match constructor {
+                Some(name) if name == class => {
+                    js.prelude(&format!("this.__wbg_ptr = {} >>> 0;", val));
+                    js.push(String::from("this"));
+                }
+                Some(_) | None => {
+                    js.cx.require_class_wrap(class);
+                    js.push(format!("{}.__wrap({})", class, val));
+                }
+            }
         }
 
         Instruction::OptionRustFromI32 { class } => {
-            js.cx.require_class_wrap(class);
+            assert!(constructor.is_none());
             let val = js.pop();
+            js.cx.require_class_wrap(class);
             js.push(format!(
                 "{0} === 0 ? undefined : {1}.__wrap({0})",
                 val, class,
-            ))
+            ));
         }
 
         Instruction::CachedStringLoad {

diff --git a/crates/cli-support/src/wit/mod.rs b/crates/cli-support/src/wit/mod.rs
@@ -484,7 +484,10 @@ impl<'a> Context<'a> {
             Some(class) => {
                 let class = class.to_string();
                 match export.method_kind {
-                    decode::MethodKind::Constructor => AuxExportKind::Constructor(class),
+                    decode::MethodKind::Constructor => {
+                        verify_constructor_return(&class, &descriptor.ret)?;
+                        AuxExportKind::Constructor(class)
+                    }
                     decode::MethodKind::Operation(op) => {
                         if !op.is_static {
                             // Make the first argument be the index of the receiver.
@@ -1424,6 +1427,36 @@ impl<'a> Context<'a> {
     }
 }
 
+/// Verifies exported constructor return value is not a JS primitive type
+fn verify_constructor_return(class: &str, ret: &Descriptor) -> Result<(), Error> {
+    match ret {
+        Descriptor::I8
+        | Descriptor::U8
+        | Descriptor::ClampedU8
+        | Descriptor::I16
+        | Descriptor::U16
+        | Descriptor::I32
+        | Descriptor::U32
+        | Descriptor::F32
+        | Descriptor::F64
+        | Descriptor::I64
+        | Descriptor::U64
+        | Descriptor::Boolean
+        | Descriptor::Char
+        | Descriptor::CachedString
+        | Descriptor::String
+        | Descriptor::Option(_)
+        | Descriptor::Enum { .. }
+        | Descriptor::Unit => {
+            bail!("The constructor for class `{}` tries to return a JS primitive type, which would cause the return value to be ignored. Use a builder instead (remove the `constructor` attribute).", class);
+        }
+        Descriptor::Result(ref d) | Descriptor::Ref(ref d) | Descriptor::RefMut(ref d) => {
+            verify_constructor_return(class, d)
+        }
+        _ => Ok(()),
+    }
+}
+
 /// Extract all of the `Program`s encoded in our custom section.
 ///
 /// `program_storage` is used to squirrel away the raw bytes of the custom

diff --git a/crates/cli/tests/reference/builder.d.ts b/crates/cli/tests/reference/builder.d.ts
@@ -0,0 +1,11 @@
+/* tslint:disable */
+/* eslint-disable */
+/**
+*/
+export class ClassBuilder {
+  free(): void;
+/**
+* @returns {ClassBuilder}
+*/
+  static builder(): ClassBuilder;
+}
diff --git a/crates/cli/tests/reference/builder.js b/crates/cli/tests/reference/builder.js
@@ -0,0 +1,61 @@
+let wasm;
+export function __wbg_set_wasm(val) {
+    wasm = val;
+}
+
+
+const lTextDecoder = typeof TextDecoder === 'undefined' ? (0, module.require)('util').TextDecoder : TextDecoder;
+
+let cachedTextDecoder = new lTextDecoder('utf-8', { ignoreBOM: true, fatal: true });
+
+cachedTextDecoder.decode();
+
+let cachedUint8Memory0 = null;
+
+function getUint8Memory0() {
+    if (cachedUint8Memory0 === null || cachedUint8Memory0.byteLength === 0) {
+        cachedUint8Memory0 = new Uint8Array(wasm.memory.buffer);
+    }
+    return cachedUint8Memory0;
+}
+
+function getStringFromWasm0(ptr, len) {
+    ptr = ptr >>> 0;
+    return cachedTextDecoder.decode(getUint8Memory0().subarray(ptr, ptr + len));
+}
+/**
+*/
+export class ClassBuilder {
+
+    static __wrap(ptr) {
+        ptr = ptr >>> 0;
+        const obj = Object.create(ClassBuilder.prototype);
+        obj.__wbg_ptr = ptr;
+
+        return obj;
+    }
+
+    __destroy_into_raw() {
+        const ptr = this.__wbg_ptr;
+        this.__wbg_ptr = 0;
+
+        return ptr;
+    }
+
+    free() {
+        const ptr = this.__destroy_into_raw();
+        wasm.__wbg_classbuilder_free(ptr);
+    }
+    /**
+    * @returns {ClassBuilder}
+    */
+    static builder() {
+        const ret = wasm.classbuilder_builder();
+        return ClassBuilder.__wrap(ret);
+    }
+}
+
+export function __wbindgen_throw(arg0, arg1) {
+    throw new Error(getStringFromWasm0(arg0, arg1));
+};
+
diff --git a/crates/cli/tests/reference/builder.rs b/crates/cli/tests/reference/builder.rs
@@ -0,0 +1,11 @@
+use wasm_bindgen::prelude::*;
+
+#[wasm_bindgen]
+pub struct ClassBuilder(());
+
+#[wasm_bindgen]
+impl ClassBuilder {
+    pub fn builder() -> ClassBuilder {
+        ClassBuilder(())
+    }
+}
diff --git a/crates/cli/tests/reference/builder.wat b/crates/cli/tests/reference/builder.wat
@@ -0,0 +1,10 @@
+(module
+  (type (;0;) (func (result i32)))
+  (type (;1;) (func (param i32)))
+  (func $__wbg_classbuilder_free (;0;) (type 1) (param i32))
+  (func $classbuilder_builder (;1;) (type 0) (result i32))
+  (memory (;0;) 17)
+  (export "memory" (memory 0))
+  (export "__wbg_classbuilder_free" (func $__wbg_classbuilder_free))
+  (export "classbuilder_builder" (func $classbuilder_builder))
+)
diff --git a/crates/cli/tests/reference/constructor.d.ts b/crates/cli/tests/reference/constructor.d.ts
@@ -0,0 +1,10 @@
+/* tslint:disable */
+/* eslint-disable */
+/**
+*/
+export class ClassConstructor {
+  free(): void;
+/**
+*/
+  constructor();
+}
diff --git a/crates/cli/tests/reference/constructor.js b/crates/cli/tests/reference/constructor.js
@@ -0,0 +1,53 @@
+let wasm;
+export function __wbg_set_wasm(val) {
+    wasm = val;
+}
+
+
+const lTextDecoder = typeof TextDecoder === 'undefined' ? (0, module.require)('util').TextDecoder : TextDecoder;
+
+let cachedTextDecoder = new lTextDecoder('utf-8', { ignoreBOM: true, fatal: true });
+
+cachedTextDecoder.decode();
+
+let cachedUint8Memory0 = null;
+
+function getUint8Memory0() {
+    if (cachedUint8Memory0 === null || cachedUint8Memory0.byteLength === 0) {
+        cachedUint8Memory0 = new Uint8Array(wasm.memory.buffer);
+    }
+    return cachedUint8Memory0;
+}
+
+function getStringFromWasm0(ptr, len) {
+    ptr = ptr >>> 0;
+    return cachedTextDecoder.decode(getUint8Memory0().subarray(ptr, ptr + len));
+}
+/**
+*/
+export class ClassConstructor {
+
+    __destroy_into_raw() {
+        const ptr = this.__wbg_ptr;
+        this.__wbg_ptr = 0;
+
+        return ptr;
+    }
+
+    free() {
+        const ptr = this.__destroy_into_raw();
+        wasm.__wbg_classconstructor_free(ptr);
+    }
+    /**
+    */
+    constructor() {
+        const ret = wasm.classconstructor_new();
+        this.__wbg_ptr = ret >>> 0;
+        return this;
+    }
+}
+
+export function __wbindgen_throw(arg0, arg1) {
+    throw new Error(getStringFromWasm0(arg0, arg1));
+};
+
diff --git a/crates/cli/tests/reference/constructor.rs b/crates/cli/tests/reference/constructor.rs
@@ -0,0 +1,13 @@
+use wasm_bindgen::prelude::*;
+
+#[wasm_bindgen]
+pub struct ClassConstructor(());
+
+#[wasm_bindgen]
+impl ClassConstructor {
+
+    #[wasm_bindgen(constructor)]
+    pub fn new() -> ClassConstructor {
+        ClassConstructor(())
+    }
+}
diff --git a/crates/cli/tests/reference/constructor.wat b/crates/cli/tests/reference/constructor.wat
@@ -0,0 +1,10 @@
+(module
+  (type (;0;) (func (result i32)))
+  (type (;1;) (func (param i32)))
+  (func $__wbg_classconstructor_free (;0;) (type 1) (param i32))
+  (func $classconstructor_new (;1;) (type 0) (result i32))
+  (memory (;0;) 17)
+  (export "memory" (memory 0))
+  (export "__wbg_classconstructor_free" (func $__wbg_classconstructor_free))
+  (export "classconstructor_new" (func $classconstructor_new))
+)
diff --git a/crates/cli/tests/wasm-bindgen/main.rs b/crates/cli/tests/wasm-bindgen/main.rs
@@ -431,3 +431,27 @@ fn function_table_preserved() {
         .wasm_bindgen("");
     cmd.assert().success();
 }
+
+#[test]
+fn constructor_cannot_return_option_struct() {
+    let (mut cmd, _out_dir) = Project::new("constructor_cannot_return_option_struct")
+        .file(
+            "src/lib.rs",
+            r#"
+                use wasm_bindgen::prelude::*;
+
+                #[wasm_bindgen]
+                pub struct Foo(());
+                
+                #[wasm_bindgen]
+                impl Foo {
+                    #[wasm_bindgen(constructor)]
+                    pub fn new() -> Option<Foo> {
+                        Some(Foo(()))
+                    }
+                }
+            "#,
+        )
+        .wasm_bindgen("--target web");
+    cmd.assert().failure();
+}
diff --git a/guide/src/reference/attributes/on-rust-exports/constructor.md b/guide/src/reference/attributes/on-rust-exports/constructor.md
@@ -32,3 +32,41 @@ import { Foo } from './my_module';
 const f = new Foo();
 console.log(f.get_contents());
 ```
+
+## Caveats
+
+Starting from v0.2.48 there is a bug in `wasm-bindgen` which breaks inheritance of exported Rust structs from JavaScript side (see [#3213](https://github.com/rustwasm/wasm-bindgen/issues/3213)). If you want to inherit from a Rust struct such as:
+
+```rust
+use wasm_bindgen::prelude::*;
+
+#[wasm_bindgen]
+pub struct Parent {
+    msg: String,
+}
+
+#[wasm_bindgen]
+impl Parent {
+    #[wasm_bindgen(constructor)]
+    fn new() -> Self {
+        Parent {
+            msg: String::from("Hello from Parent!"),
+        }
+    }
+}
+```
+
+You will need to reset the prototype of `this` back to the `Child` class prototype after calling the `Parent`'s constructor via `super`.
+
+```js
+import { Parent } from './my_module';
+
+class Child extends Parent {
+    constructor() {
+        super();
+        Object.setPrototypeOf(this, Child.prototype);
+    }
+}
+```
+
+This is no longer required as of v0.2.88.