Improved support for string enums

[RunDevelopment]

Another string enum PR.

fixes #2153
fixes #3057

Changes:

1. Fixed Typescript definition not generated for string enums #2153. Typescript types are now generated for string enums.
2. String enums now support the skip_typescript and js_name options, just like C-style enums.
3. Bindings now generate a non-exported array for the string enum variants and reference the array when converting to and from WASM.
4. Improved error messages. This together with 1. fixes Implementing String Enums #3057.

Everything (except skip_typescript) is tested in crates/cli/tests/reference/enums.rs.

1. Typescript types

String enums now generate TS types. Example:

/// The name of a color.
#[wasm_bindgen]
#[derive(PartialEq, Debug)]
pub enum ColorName {
    Green = "green",
    Yellow = "yellow",
    Red = "red",
}

/**
 * The name of a color.
 */
export type ColorName = "green" | "yellow" | "red";

I used a union type of string literal to represent the fact that there is no exported object on the JS side.

A const enum also would have been possible. In fact, this would have been the better option in a way, because it would allow users to use string enums just like regular enums, e.g. ColorName.Green. However, const enums do not allow using string literals directly (e.g. const _: ColorName = "green" does not type check), and because const enums are not supported by some bundlers.

2. js_name and skip_typescript

My goal was to make string enums and C-style enums are similar as possible, so I added support for these 2 attributes while I was at it. (In general, the code handling the implementation of string enums and C-style enums is now quite similar. Maybe it could be more unified in the future, but maybe that goes too far.)

skip_typescript is important to support, since users might have worked around the lack of string enum types with typescript_custom_section before (e.g. I did that in my project). While most of these custom type defs can be removed now, some users may wish to keep their types, and this option allows them to do so.

js_name is here for consistency and because it's useful.

3. Bindings

Instead of creating an array with all string enum variants for each conversion, I changed the bindings to create one global array. So instead of ["a", "b", "c"][index] for each conversion, it now generates _StringEnum_values[index].

I used _{name}_values as the name of the constant, because I wanted to avoid name collisions. I wasn't sure what the best way of doing this is, so I just made up a naming scheme that I feel is unlikely to be used by WASM bindgen users.

4. Improved error messages

As pointed out in #3057, the error messages for mixed string-and-C-style enums were not ideal.

I fixed this by changing how string enums are detected. Now, when an enum contains one variant with a string discriminate, it's assumed to be a string enum. This makes it easier to give relevant error messages.

I also changed a few enum error messages that make it sound like only C-style enums are supported.

[daxpedda]

I used _{name}_values as the name of the constant, because I wanted to avoid name collisions. I wasn't sure what the best way of doing this is, so I just made up a naming scheme that I feel is unlikely to be used by WASM bindgen users.

Just prefix it with __wbindgen and a namespace. E.g. __wbindgen_enum_*.

---

Thank you for looking into this!

[RunDevelopment]

Just prefix it with __wbindgen and a namespace. E.g. __wbindgen_enum_*.

Thank you for the suggestion. I used your __wbindgen_enum_*.

---

After reading through the comments in #3057, I noticed that I didn't correctly generate the types for non-public string enums.

Aside from that, I also fixed the error messages mentioned in #3057 to resolve the issue.

[RunDevelopment]

Btw, seems like there's a race condition in raytrace_parallel (this is unrelated to this PR). Here's the output:

running 2 tests
test raytrace_parallel ... FAILED
test wasm_audio_worklet ... ok

failures:

error: test failed, to rerun pass `-p example-tests --test shell`
---- raytrace_parallel stdout ----
thread 'raytrace_parallel' panicked at /home/runner/work/wasm-bindgen/wasm-bindgen/crates/example-tests/src/lib.rs:467:25:
called `Result::unwrap()` on an `Err` value: ()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    raytrace_parallel

[daxpedda]

Missing a changelog entry, otherwise LGTM!
Thank you for doing this, great work!

Its pretty weird, that C-style enums are exported in JS with a frozen object but string enums aren't at all. Not sure about the history here, but intuitively I would also not export C-style enums as well. Maybe something to consider for the next breaking change.

(raytrace_parallel and a couple of other tests in the job spuriously fail regularly, feel free to ignore it, I can just restart these jobs manually until they succeed)

[RunDevelopment]

Missing a changelog entry

Will add.

Its pretty weird, that C-style enums are exported in JS with a frozen object but string enums aren't at all.

The object is just how TypeScript does enums. Here's an example where you can see the JS output of tsc for a simple enum. The only difference to WASM bindgen is that TS doesn't freeze the object.

Not sure about the history here, but intuitively I would also not export C-style enums as well. Maybe something to consider for the next breaking change.

If you're suggesting to remove the exported frozen object for C-style, then please don't. That would make the bindings for C-style enums completely useless. I mean, try using the following function on the JS side:

#[wasm_bindgen]
pub fn foo(a: SomeEnum) {}

#[wasm_bindgen]
pub enum SomeEnum {
    A = 123,
    B = 624,
    C = 24891,
}

I don't want to write foo(624) in JS. I want to be able to write foo(SomeEnum.B) and that can only be achieved with an object for SomeEnum at runtime.

The only reason why string enums are useful without a frozen object is that JS/TS devs like using string literals instead of TS enums in certain places. Here's an example of this in NodeJS' crypto API. This can create pretty nice strongly-typed APIs like hash.digest("base64").

That all said, C-style enums and string enums are still handled inconsistent as you noted. C-style enums are proper TS enums, but string enums are type only. I think one way to resolve this would be to make both TS enums by default and support type-only string enums via an option (e.g. #[wasm_bindgen(type_only)]). This would also open up the path for mixed C-style and string enums. E.g.

#[wasm_bindgen]
pub enum Foo {
  A = 1,
  B = 3,
  C = "foo",
}

could become

// .d.ts
export enum Foo {
  A = 1,
  B = 3,
  C = "foo",
};

// .js
export const Foo = Object.freeze({
  A: 1, "1": "A",
  B: 3, "3": "B",
  C: "foo", "foo": "C", 
});

On the ABI side, mix enums would still use u32, just like C-style enums and string enums.

Well, it's a breaking change for string enums, so whatever.

feel free to ignore it, I can just restart these jobs manually until they succeed

No worries. I can just push an empty commit to trigger a re-run.

[daxpedda]

Its pretty weird, that C-style enums are exported in JS with a frozen object but string enums aren't at all.

The object is just how TypeScript does enums. Here's an example where you can see the JS output of tsc for a simple enum. The only difference to WASM bindgen is that TS doesn't freeze the object.

Not sure about the history here, but intuitively I would also not export C-style enums as well. Maybe something to consider for the next breaking change.

If you're suggesting to remove the exported frozen object for C-style, then please don't.

Just to clarify: as a maintainer of wasm-bindgen I don't make judgement calls on JS/TS exports because I don't have any expertise in that area. Please bear with me while I come to very naive conclusions while learning about it.

In any case, I abstain from doing any changes in this area without some significant input from others who know what they are talking about!

---

The reason why I'm suggesting removing the JS export is because enums inherently do not possess a JS equivalent. Generating JS bindings from WebIDL also has no conversion, which is something I would rather follow then doing some custom conversion.

I don't want to write foo(624) in JS. I want to be able to write foo(SomeEnum.B) and that can only be achieved with an object for SomeEnum at runtime.

That's very understandable, but I think this should be explicitly opted into instead of doing our own custom conversion implicitly.

[daxpedda]

Thank you!

[RunDevelopment]

Please bear with me while I come to very naive conclusions while learning about it.

No worries. I just wanted to explain why the current state, despite being inconsistent, is useful.