wasm-bindgen icon indicating copy to clipboard operation
wasm-bindgen copied to clipboard

Implementing String Enums

Open JohnGinger opened this issue 3 years ago • 12 comments

Motivation

At the moment number enums are supported. e.g.

#[wasm_bindgen]
pub enum Enum {
    A = 10,
    B = 20,
}

I would like to support string enums as well e.g.

#[wasm_bindgen]
pub enum Enum {
    A = "A",
    B = "B",
}

Proposed Solution

I would be happy to implement this, is it something that a PR would be accepted for?

JohnGinger avatar Sep 02 '22 10:09 JohnGinger

String enums are already supported; see https://rustwasm.github.io/wasm-bindgen/reference/types/imported-js-types.html.

(I'm not sure why they're in that section; surely they should be under 'Exported Rust Types'?)

Liamolucko avatar Sep 02 '22 11:09 Liamolucko

They are in the docs, but if I try to use them they don't output any typescript types.

If you have a mixed enum, e.g.

#[wasm_bindgen]
#[derive(Copy, Clone, Debug)]
pub enum StringEnum {
    Foo = 1,
    Bar = "bar",
    Qux = "qux",
}

You get the error 'enums with #[wasm_bindgen] may only have number literal values' - which is from here. https://github.com/rustwasm/wasm-bindgen/blob/main/crates/macro-support/src/parser.rs#L1288

I was assuming the docs were wrong - but if there is a way to make this work that would be great.

JohnGinger avatar Sep 02 '22 11:09 JohnGinger

I've put a minimal reproducible example at https://github.com/JohnGinger/string-enum-example. The exported types are only

export enum NumberEnum {
  Foo,
  Bar,
  Qux,
}

JohnGinger avatar Sep 02 '22 11:09 JohnGinger

They are in the docs, but if I try to use them they don't output any typescript types.

That's a bug, the types should be emitted.

If you have a mixed enum, e.g.

#[wasm_bindgen]
#[derive(Copy, Clone, Debug)]
pub enum StringEnum {
    Foo = 1,
    Bar = "bar",
    Qux = "qux",
}

You get the error 'enums with #[wasm_bindgen] may only have number literal values' - which is from here. https://github.com/rustwasm/wasm-bindgen/blob/main/crates/macro-support/src/parser.rs#L1288

That's a bad error message - it should be saying something like 'enums must have either all string or all int payloads'.

In fact, it does give a better error message is you swap the order of the variants around - this:

#[wasm_bindgen]
#[derive(Copy, Clone, Debug)]
pub enum StringEnum {
    Bar = "bar",
    Qux = "qux",
    Foo = 1,
}

gives this error message:

error: enums with #[wasm_bindgen] cannot mix string and non-string values
 --> src/lib.rs:8:11
  |
8 |     Foo = 1,
  |           ^

So, it should be fixed to always give that message.

Liamolucko avatar Sep 02 '22 11:09 Liamolucko

That's a bug, the types should be emitted.

Okay, it's slightly more complicated than I first thought - unlike regular enums, string enums are allowed to be private, and string enums defined deep in the dependency tree shouldn't be getting top-level type declarations. So, some logic will be needed to only add the type declarations if they're exported from the crate root.

Liamolucko avatar Sep 02 '22 11:09 Liamolucko

Thanks for being so responsive. I'm happy to help fix this, any pointers as to where I should start?

JohnGinger avatar Sep 02 '22 13:09 JohnGinger

String enums are parsed here: https://github.com/rustwasm/wasm-bindgen/blob/f75a3f8986c56d752b687cebe47b53232f403ba7/crates/macro-support/src/parser.rs#L1168-L1212

Their AST is converted to the type passed from the macro to the CLI here: https://github.com/rustwasm/wasm-bindgen/blob/f75a3f8986c56d752b687cebe47b53232f403ba7/crates/backend/src/encode.rs#L315-L317

That type is defined here (currently empty): https://github.com/rustwasm/wasm-bindgen/blob/f75a3f8986c56d752b687cebe47b53232f403ba7/crates/shared/src/lib.rs#L91

ImportKind::Enum is matched against and ignored here: https://github.com/rustwasm/wasm-bindgen/blob/f75a3f8986c56d752b687cebe47b53232f403ba7/crates/cli-support/src/wit/mod.rs#L503

I think that's most of the places you'd have to change, so hopefully that's helpful!

Also, I already wrote a fix for the bad error message in 6ed8f5b, if you want to start from there.

Liamolucko avatar Sep 02 '22 21:09 Liamolucko

Oh, and you'll want to put the list of string enums somewhere in this struct to pass to the JS generation stage: https://github.com/rustwasm/wasm-bindgen/blob/f75a3f8986c56d752b687cebe47b53232f403ba7/crates/cli-support/src/wit/nonstandard.rs#L8-L62

And then actually generate the JS in here: https://github.com/rustwasm/wasm-bindgen/blob/f75a3f8986c56d752b687cebe47b53232f403ba7/crates/cli-support/src/js/mod.rs#L2384-L2415

Liamolucko avatar Sep 02 '22 22:09 Liamolucko

I would like to work on this bug ~ thanks ~

Solo-steven avatar Sep 20 '23 13:09 Solo-steven

Go for it, sorry I haven't got round to it

JohnGinger avatar Sep 20 '23 14:09 JohnGinger

I tried to at least improve the situation by changing the type from any to string but couldn't even find the logic

SIGSTACKFAULT avatar Nov 30 '23 03:11 SIGSTACKFAULT

I tried to at least improve the situation by changing the type from any to string but couldn't even find the logic

The type is any because string enums currently work by converting the enums to/from JsValues before passing them to/from JS, and their WasmDescribe impls describe them as such, which the CLI translates to any. The code that generates their WasmDescribe impls is here:

https://github.com/rustwasm/wasm-bindgen/blob/def914721caf434c0f14fa9eb5df07d07d5113d4/crates/backend/src/codegen.rs#L1084-L1088

It's possible to keep them as JsValues but change the TypeScript type by describing them as NAMED_EXTERNREFs instead - I did the exact same thing in #3554 for Vec<String>s:

https://github.com/rustwasm/wasm-bindgen/blob/def914721caf434c0f14fa9eb5df07d07d5113d4/src/convert/slices.rs#L227-L240

It's a bit weird that they're passed as JsValues in the first place, though. Really they should just be passed to/from JS as &strs/Strings directly, which would also change the TypeScript type to string. So, that would be the preferable way of changing the TypeScript type to string and I don't think should be too difficult to implement; but changing the WasmDescribe impl would still be an improvement!

Liamolucko avatar Nov 30 '23 06:11 Liamolucko