Skip to content
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

Remove class wrap for constructors in Rust exports #3562

Merged
merged 6 commits into from
Aug 25, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,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`.
Expand All @@ -66,6 +71,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.
Expand Down
31 changes: 25 additions & 6 deletions crates/cli-support/src/js/binding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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 {
Expand Down
25 changes: 24 additions & 1 deletion crates/cli-support/src/wit/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,30 @@ impl<'a> Context<'a> {
Some(class) => {
let class = class.to_string();
match export.method_kind {
decode::MethodKind::Constructor => AuxExportKind::Constructor(class),
decode::MethodKind::Constructor => {
let msg = "Trying to return JS primitive from constructor, return value will be ignored. Use a builder instead (remove `constructor` attribute).";
snOm3ad marked this conversation as resolved.
Show resolved Hide resolved
match descriptor.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::Unit => bail!(msg),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should also include Enum, since (exported) enums are represented as integers in JS.

Refs and RefMuts to any of these should probably also be included, since they'll still map to primitives while behind a reference, although it doesn't matter a whole lot since you can't return references right now anyway.

(Imported / string enums are handled purely at the macro level, so we don't need to worry about them here.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this check to it's own function so that it is cleaner to check nested descriptors. Like you mentioned Ref and RefMut are not relevant for now, but it occurred to me that Result still is.

_ => {}
}
AuxExportKind::Constructor(class)
}
decode::MethodKind::Operation(op) => {
if !op.is_static {
// Make the first argument be the index of the receiver.
Expand Down
11 changes: 11 additions & 0 deletions crates/cli/tests/reference/builder.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/* tslint:disable */
/* eslint-disable */
/**
*/
export class ClassBuilder {
free(): void;
/**
* @returns {ClassBuilder}
*/
static builder(): ClassBuilder;
}
61 changes: 61 additions & 0 deletions crates/cli/tests/reference/builder.js
Original file line number Diff line number Diff line change
@@ -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));
};

11 changes: 11 additions & 0 deletions crates/cli/tests/reference/builder.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
use wasm_bindgen::prelude::*;

#[wasm_bindgen]
pub struct ClassBuilder(());

#[wasm_bindgen]
impl ClassBuilder {
pub fn builder() -> ClassBuilder {
ClassBuilder(())
}
}
10 changes: 10 additions & 0 deletions crates/cli/tests/reference/builder.wat
Original file line number Diff line number Diff line change
@@ -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))
)
10 changes: 10 additions & 0 deletions crates/cli/tests/reference/constructor.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/* tslint:disable */
/* eslint-disable */
/**
*/
export class ClassConstructor {
free(): void;
/**
*/
constructor();
}
53 changes: 53 additions & 0 deletions crates/cli/tests/reference/constructor.js
Original file line number Diff line number Diff line change
@@ -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));
};

13 changes: 13 additions & 0 deletions crates/cli/tests/reference/constructor.rs
Original file line number Diff line number Diff line change
@@ -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(())
}
}
10 changes: 10 additions & 0 deletions crates/cli/tests/reference/constructor.wat
Original file line number Diff line number Diff line change
@@ -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))
)
24 changes: 24 additions & 0 deletions crates/cli/tests/wasm-bindgen/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
38 changes: 38 additions & 0 deletions guide/src/reference/attributes/on-rust-exports/constructor.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.