Add support for static enum methods via TS namespaces

[RunDevelopment]

Fixes #1715 Resolves #2045

This PR adds support for static methods on enums (string and C-style). It uses the approach I outlined here using TS namespaces for typing.

Example:

#[wasm_bindgen]
pub enum ImageFormat { PNG, JPEG, GIF }

#[wasm_bindgen]
impl ImageFormat {
    pub fn from_str(s: &str) -> ImageFormat { todo!() }
}

Now generates the following JS and type definitions:

/**
 * @enum {0 | 1 | 2}
 */
export const ImageFormat = {
    PNG: 0, "0": "PNG",
    JPEG: 1, "1": "JPEG",
    GIF: 2, "2": "GIF",
};

(function(ImageFormat) {
    /**
     * @param {string} s
     * @returns {ImageFormat}
     */
    ImageFormat.from_str = function from_str(s) { /* ... */ };
})(ImageFormat);

export enum ImageFormat {
  PNG = 0,
  JPEG = 1,
  GIF = 2,
}
export namespace ImageFormat {
  export function from_str(s: string): ImageFormat;
}

Note that C-style enums no longer use Object.freeze. This is required since the functionality of namespaces lies in adding properties to the namespaced object. If Object.freeze was kept, the namespace would result in a runtime error when the JS file is run. This resolves #2045.

Another thing to note is how string enums are handled. String enums don't have an exported object the namespace could latch onto, so an empty object is automatically exported.

---

Changes:

1. Exported methods with a JS class that points to an enum or string enum will now be exported via a namespace.

2. Error if users try to add instance methods/getters/setters or static getters/setters.

   This is technically a breaking change, because string enums technically supported all this before. Technically, because them supporting it was a bug. Both string enums and C-style enums had the bug where they tried to generate a class for methods and getters/setters. For C-style enums, with resulted in a runtime error in JS, because both the enum and class were exported under the same name. However, string enums don't export anything, so the class didn't cause any problems. While buggy (there were no type definitions for the generated class) and unintended, this somewhat worked. Users could successfully define and use static methods for string enums.

   Why is this a breaking change? The namespaces I implemented don't support static getters and setter. While it is trivial to implement static getter/setters in JS via Object.defineProperty, the problem is typing them in TS. TS only support getters/setters on classes. The best namespaces can do is to export const/let like this.

   So the question is: are we okay with breaking user code relying on a bug, or should I add support for static getters/setters on namespaces?

3. Explicitly type what type of expressions can be exported in Context::export. This is done via the new ExportJs enum, which supports classes, functions, expressions, and namespaces. Instead of making the Context::export detect what is being exported by analyzing the string content of the JS expression, the ones generating the JS expression are now required to follow a certain format. This makes the code for exporting a lot more explicit and the API a lot less stringly typed.

[daxpedda]

2. Error if users try to add instance methods/getters/setters or static getters/setters.

   This is technically a breaking change, because string enums technically supported all this before. Technically, because them supporting it was a bug. Both string enums and C-style enums had the bug where they tried to generate a class for methods and getters/setters. For C-style enums, with resulted in a runtime error in JS, because both the enum and class were exported under the same name. However, string enums don't export anything, so the class didn't cause any problems. While buggy (there were no type definitions for the generated class) and unintended, this somewhat worked. Users could successfully define and use static methods for string enums.

   Why is this a breaking change? The namespaces I implemented don't support static getters and setter. While it is trivial to implement static getter/setters in JS via Object.defineProperty, the problem is typing them in TS. TS only support getters/setters on classes. The best namespaces can do is to export const/let like this.

   So the question is: are we okay with breaking user code relying on a bug, or should I add support for static getters/setters on namespaces?

I don't think this was necessary unintended by users. After all these methods are used internally by Rust as well, so I think we shouldn't disallow them. We might want to require users to do this via a separate impl block which doesn't have the #[wasm_bindgen] attribute, but indeed, this is a breaking change.

If we want users to use a separate impl block, we can use warnings instead of errors. The upside is that its clear to users what is and what isn't exported. So AFAICS this is the best route.

Adding support for static getters and setters will still not solve the issue with non-static methods, where we still need to decide if we want users to have a separate impl block or not.

I'm unable to make a decision on implementing static getters and setters, I'm having a hard time getting a feel of how "okay this is according to TS". If you believe this doesn't look too strange, it would sounds like a good feature to me to have.

[daxpedda]

Another thing to note is how string enums are handled. String enums don't have an exported object the namespace could latch onto, so an empty object is automatically exported.

We should definitely not do this here, instead lets resolve #4260 first.

[RunDevelopment]

I don't think this was necessary unintended by users. After all these methods are used internally by Rust as well, so I think we shouldn't disallow them.

Uhm, please reconsider. Okay, so here are some details on how string enum methods """work""" in JS. Firstly, string enums don't implement RefFromWasmAbi, so you can only consume self in methods/getters/setters. Secondly, please look at the JS code gen of a string enum with a constructor and instance method:

Detailed example (it's a lot of code but necessary)

```rs use wasm_bindgen::prelude::*; #[wasm_bindgen] pub enum StringEnum { A = "a", B = "b", } #[wasm_bindgen] impl StringEnum { #[wasm_bindgen(constructor)] pub fn new() -> Self { Self::A } pub fn consume(self) {} } ``` ```js let wasm; export function __wbg_set_wasm(val) { wasm = val; } const __wbindgen_enum_StringEnum = ["a", "b"]; const StringEnumFinalization = (typeof FinalizationRegistry === 'undefined') ? { register: () => {}, unregister: () => {} } : new FinalizationRegistry(ptr => wasm.__wbg_stringenum_free(ptr >>> 0, 1)); export class StringEnum { __destroy_into_raw() { const ptr = this.__wbg_ptr; this.__wbg_ptr = 0; StringEnumFinalization.unregister(this); return ptr; } free() { const ptr = this.__destroy_into_raw(); wasm.__wbg_stringenum_free(ptr, 0); } constructor() { const ret = wasm.stringenum_new(); return __wbindgen_enum_StringEnum[ret]; } consume() { const ptr = this.__destroy_into_raw(); wasm.stringenum_consume(ptr); } } ``` ```wat (module $reference_test.wasm (type (;0;) (func (result i32))) (type (;1;) (func (param i32))) (func $stringenum_new (;0;) (type 0) (result i32)) (func $stringenum_consume (;1;) (type 1) (param i32)) (memory (;0;) 17) (export "memory" (memory 0)) (export "stringenum_new" (func $stringenum_new)) (export "stringenum_consume" (func $stringenum_consume)) (@custom "target_features" (after code) "\04+\0amultivalue+\0fmutable-globals+\0freference-types+\08sign-ext") ) ``` Please feast your eyes upon this beauty. A few things to note: 1. The constructor doesn't work. It doesn't initialize the `__wbg_ptr` field, meaning that class instances created by it are already in a freed/destroyed state. It instead just returns the string value of `StringEnum::A`. This means that there is no way to create valid instances of this class. \ (This is minor, but the constructor also doesn't register instances in the generated `FinalizationRegistry`.) 3. The `consume` method doesn't work. `this.__destroy_into_raw()` always returns `undefined`, which is obviously an invalid pointer. Also, I'm not sure whether `wasm.stringenum_consume` even wants a pointer, since the WASM ABI of string enums is an i32/u32 that identifies the variant. 4. The `free` method and finalizer will always fail, because `wasm.__wbg_stringenum_free` doesn't exist. So to actually use a class generated for a string enum, the user would have to either overwrite the constructor or manually initialize the `__wbg_ptr` field later. Either way, they would have to write the logic for allocating and returning a pointer to a Rust `StringEnum` type. And once they've done all that work to create a valid instance, they get to use it at most once (if it works at all. As I said, I'm not sure about the ABI).

So yeah, string enum methods are generated in a completely dysfunctional state. I highly highly doubt that anyone is using them on the JS side. This is why I can only see string enum methods as a bug.

That all said, I agree that some users might have accidentally included instance methods in an impl block with #[wasm_bindgen] and thought "if it compiles, it works!" So how about this: instead of a hard error, we just ignore instance methods/getters/setters and constructors for string enums and don't generate a JS class for it for now? This would be backwards compatible, since no one is using the generated JS class anyway. This does mean that we'll have a few unnecessarily exported functions in the wasm binary, but that's shouldn't cause any problems, right?

Then in the next major version, we can make it hard error and tell people to use a separate impl block.

What do you think?

I'm unable to make a decision on implementing static getters and setters, I'm having a hard time getting a feel of how "okay this is according to TS". If you believe this doesn't look too strange, it would sounds like a good feature to me to have.

On the TS side, it's pretty hacky. It's not that getting getters+setters to work on a namespace is difficult to implement in JS, it's just that getters/setters can only be typed with classes and interfaces. So using export const and export let is pretty hacky. I also don't see TS adding a way to properly type this any time soon.

But I still think we should support them. They are just too useful and natural to not support. Even if the type definitions may be a bit hacky, I think it's still worth it.

Or put another way, I don't like as the one that implements the feature, but I want it as someone that will use the feature.

Another thing to note is how string enums are handled. String enums don't have an exported object the namespace could latch onto, so an empty object is automatically exported.

We should definitely not do this here, instead lets resolve #4260 first.

Sorry, but I don't see why this is a problem? We necessarily need to export a "physical" JS object to create a namespace.

I also don't see how this relates to #4260. #4260 is about the TS type of string enums (not) being exported. This PR only exports a JS object and doesn't change the type or whether it's exported at all.

[daxpedda]

I don't think this was necessary unintended by users. After all these methods are used internally by Rust as well, so I think we shouldn't disallow them.

Uhm, please reconsider. Okay, so here are some details on how string enum methods """work""" in JS. Firstly, string enums don't implement RefFromWasmAbi, so you can only consume self in methods/getters/setters. Secondly, please look at the JS code gen of a string enum with a constructor and instance method: Detailed example (it's a lot of code but necessary)

So yeah, string enum methods are generated in a completely dysfunctional state. I highly highly doubt that anyone is using them on the JS side. This is why I can only see string enum methods as a bug.

That all said, I agree that some users might have accidentally included instance methods in an impl block with #[wasm_bindgen] and thought "if it compiles, it works!" So how about this: instead of a hard error, we just ignore instance methods/getters/setters and constructors for string enums and don't generate a JS class for it for now? This would be backwards compatible, since no one is using the generated JS class anyway. This does mean that we'll have a few unnecessarily exported functions in the wasm binary, but that's shouldn't cause any problems, right?

Then in the next major version, we can make it hard error and tell people to use a separate impl block.

What do you think?

I was saying that regular methods can be used in Rust as well, this is why we shouldn't disallow it. I didn't know that e.g. constructors were allowed in first place and that regular methods generated any JS code, which probably cause the misunderstanding here.

What we can do is make #[wasm_bindgen(constructor)] error, because it is simply not supported. Anything else should be allowed, but not generate any JS code as you were already suggesting. As an additional fix we should also not export these functions in the Wasm binary.

I think it would also be nice if we emit a warning for every function in an #[wasm_bindgen] impl block that isn't being exported. But if this is too hard I don't want to make this a requirement for this PR.

I'm unable to make a decision on implementing static getters and setters, I'm having a hard time getting a feel of how "okay this is according to TS". If you believe this doesn't look too strange, it would sounds like a good feature to me to have.

On the TS side, it's pretty hacky. It's not that getting getters+setters to work on a namespace is difficult to implement in JS, it's just that getters/setters can only be typed with classes and interfaces. So using export const and export let is pretty hacky. I also don't see TS adding a way to properly type this any time soon.

But I still think we should support them. They are just too useful and natural to not support. Even if the type definitions may be a bit hacky, I think it's still worth it.

Or put another way, I don't like as the one that implements the feature, but I want it as someone that will use the feature.

You make it sound like its not very TSy, so my gut feeling is that we should not implement this. Lets move this discussion to a follow-up PR.

Another thing to note is how string enums are handled. String enums don't have an exported object the namespace could latch onto, so an empty object is automatically exported.

We should definitely not do this here, instead lets resolve #4260 first.

Sorry, but I don't see why this is a problem? We necessarily need to export a "physical" JS object to create a namespace.

I also don't see how this relates to #4260. #4260 is about the TS type of string enums (not) being exported. This PR only exports a JS object and doesn't change the type or whether it's exported at all.

My thinking here is that we should not expose static methods of a type we don't export. This would apply to TS as well. You make it sound obvious that there is no relation at all, am I missing something?

[RunDevelopment]

Since string enums seem to cause of few problems, maybe I should change the PR to only affect C-style enums? Basically, enable namespaces for C-style enums, fix up the situation around string enums, and then enable namespaces for string enums.

Or we fix up things around string enums beforehand and block this PR until then.

I don't mind either way. What do you think?

What we can do is make #[wasm_bindgen(constructor)] error, because it is simply not supported. Anything else should be allowed, but not generate any JS code as you were already suggesting. As an additional fix we should also not export these functions in the Wasm binary.

Sounds good 👍

As for not exporting those functions and emitting warning, I think this should be a separate PR.

You make it sound like its not very TSy, so my gut feeling is that we should not implement this. Lets move this discussion to a follow-up PR.

I kind of agree, but the problem is that static getters and setters already work today for string enums.

My thinking here is that we should not expose static methods of a type we don't export. This would apply to TS as well. You make it sound obvious that there is no relation at all, am I missing something?

Ah, that's what you meant. Yeah, I was thinking of namespaces as mostly independent of the type they add things too, but you're right that this is a bit confusing.

[daxpedda]

Since string enums seem to cause of few problems, maybe I should change the PR to only affect C-style enums? Basically, enable namespaces for C-style enums, fix up the situation around string enums, and then enable namespaces for string enums.

Or we fix up things around string enums beforehand and block this PR until then.

I don't mind either way. What do you think?

I don't mind either way as well, but I would go with fixing string enums first.

You make it sound like its not very TSy, so my gut feeling is that we should not implement this. Lets move this discussion to a follow-up PR.

I kind of agree, but the problem is that static getters and setters already work today for string enums.

Strange, I couldn't confirm this. What did you do to reproduce this exactly?

[RunDevelopment]

Strange, I couldn't confirm this. What did you do to reproduce this exactly?

Code

```rs use wasm_bindgen::prelude::*; #[wasm_bindgen] pub enum Foo { A = "a", B = "b", } #[wasm_bindgen] impl Foo { pub fn parse(s: &str) -> Foo { todo!() } #[wasm_bindgen(getter)] pub fn favorite() -> Foo { Foo::A } #[wasm_bindgen(setter)] pub fn set_favorite(value: Foo) {} } ``` ```ts /* tslint:disable */ /* eslint-disable */ type Foo = "a" | "b"; ``` ```js const __wbindgen_enum_Foo = ["a", "b"]; const FooFinalization = (typeof FinalizationRegistry === 'undefined') ? { register: () => {}, unregister: () => {} } : new FinalizationRegistry(ptr => wasm.__wbg_foo_free(ptr >>> 0, 1)); export class Foo { __destroy_into_raw() { const ptr = this.__wbg_ptr; this.__wbg_ptr = 0; FooFinalization.unregister(this); return ptr; } free() { const ptr = this.__destroy_into_raw(); wasm.__wbg_foo_free(ptr, 0); } /** * @param {string} s * @returns {Foo} */ static parse(s) { const ptr0 = passStringToWasm0(s, wasm.__wbindgen_malloc, wasm.__wbindgen_realloc); const len0 = WASM_VECTOR_LEN; const ret = wasm.foo_parse(ptr0, len0); return __wbindgen_enum_Foo[ret]; } /** * @returns {Foo} */ static get favorite() { const ret = wasm.foo_favorite(); return __wbindgen_enum_Foo[ret]; } /** * @param {Foo} value */ static set favorite(value) { wasm.foo_set_favorite((__wbindgen_enum_Foo.indexOf(value) + 1 || 3) - 1); } } ```

There are no types generated, but the JS code gen actually works. Static methods, getters, and setters are functional.

[daxpedda]

Strange, I couldn't confirm this. What did you do to reproduce this exactly?

Code

There are no types generated, but the JS code gen actually works. Static methods, getters, and setters are functional.

Right, I forgot to mark the methods pub ...

As you already explained, numeric enums failed at runtime and string enums are strange because the enum itself wasn't exported in the first place. So I'm fine with making it an error when a user uses #[wasm_bindgen(getter/setter)]. But without the explicit attribute it should just stop generating any JS/TS.

I think this is what you meant from the start, I was just never considering the explicit #[wasm_bindgen(getter/setter)] and was only thinking about the implicit getter/setters by a #[wasm_binden] impl { ... } block.

---

I think we resolved all the remaining points, let me know if something is still standing.

[RunDevelopment]

So to summarize, the plan for now is to:

1. Disallow constructor methods and instance getter/setter methods for all enums (C-style and string).
2. Do not generate code for instance methods. (Maybe emit a warning nudging the user to use separate impl blocks.)
3. Add support for static getters/setters for all enums.

[daxpedda]

Yes, except for:

3. Add support for static getters/setters for all enums.

Which we should do in a follow-up PR, first figuring out if this is something we want to support in the first place.

[RunDevelopment]

Okay:

• I disallowed constructors, getters, and setters entirely for enums.
• Static methods will be exported like before.
• Instance method will be ignored when generating JS bindings, but are still included in the WASM binary. There's also warning that I println!ed. Not pretty.

---

I also just thought that we could maybe support instance methods by automatically making them static methods. My thought was that an instance method has the same ABI as a static method where the first argument is arg: Self. So we could do this:

Code

```rs use wasm_bindgen::prelude::*; #[wasm_bindgen] #[derive(Copy, Clone)] pub enum ImageFormat { PNG, JPEG, GIF, } #[wasm_bindgen] impl ImageFormat { pub fn from_str(s: &str) -> ImageFormat { match s { "PNG" => ImageFormat::PNG, "JPEG" => ImageFormat::JPEG, "GIF" => ImageFormat::GIF, _ => panic!("unknown image format: {}", s), } } pub fn is_lossless(self) -> bool { matches!(self, ImageFormat::PNG | ImageFormat::GIF) } } ``` ```ts export enum ImageFormat { PNG = 0, JPEG = 1, GIF = 2, } export namespace ImageFormat { export function from_str(s: string): ImageFormat; export function is_lossless(self: ImageFormat): boolean; } ```

This should probably be a follow-up PR though.

What do you think?

[RunDevelopment]

Okay, to emit proper warnings and later error, I need to analyze the whole parsed program. There is no other way around it.

I also found out that methods most likely take a pointer as self and not the enum value. So the easierst way to convert these methods into static methods is in a post-process step after the whole process is parsed and before code gen. In other words, same as when warnings are emitted.

So that's what needs to be done next, I guess.

[RunDevelopment]

So I just realized that analysis that requires the whole program is impossible to do inside the proc_macro. A macro can only see the thing it annotates and nothing outside of it. So e.g. for:

#[wasm_bindgen]
pub enum Foo { A, B, C }

#[wasm_bindgen]
impl Foo {
  #[wasm_bindgen(constructor)]
  pub fn new(): Self { Foo::A }
}

there is no way for the attribute annotating the impl to see the definition of Foo. So it has no way of knowing whether Foo is an enum or a struct.

So there is no way for the proc macro to emit warning or no export enum instance methods, since we can't even detect them.

The best we could do would be to do some magic with traits to cause compiler errors (basically, like C++ static_asserts), but that wouldn't be backwards compatible.

---

The only thing that we can do right now, is to ignore everything invalid on the CLI side. The wasm binary will have a unused imports, but there is nothing we can do about it without breaking stuff.

[daxpedda]

I also found out that methods most likely take a pointer as self and not the enum value. So the easierst way to convert these methods into static methods is in a post-process step after the whole process is parsed and before code gen. In other words, same as when warnings are emitted.

I'm not sure what you mean here exactly. Considering that only self methods are allowed, we could and should pass the value and not a reference to it.

#[wasm_bindgen] on enums also prevents enums from implementing Drop, so this shouldn't be a concern as well.

The best we could do would be to do some magic with traits to cause compiler errors (basically, like C++ static_asserts), but that wouldn't be backwards compatible.

Why would that not be backwards compatible? In any case, this is how its currently done with e.g. not allowing &self methods on enums.

[RunDevelopment]

I'm not sure what you mean here exactly. Considering that only self methods are allowed, we could and should pass the value and not a reference to it.

I might have to look at that again, but what I meant is that the ABI is a i32 and that the generated TS type is number. The ABI of exported Rust structs for self parameters is a pointer to the Rust struct instance in wasm memory. So self, &self, and &mut self all have the same ABI for exported Rust structs.

But I haven't actually looked at the Rust code gen, so I might be wrong that it's a pointer.

The best we could do would be to do some magic with traits to cause compiler errors (basically, like C++ static_asserts), but that wouldn't be backwards compatible.

Why would that not be backwards compatible? In any case, this is how its currently done with e.g. not allowing &self methods on enums.

What I meant was that we could have internal marker traits (e.g. CanHaveConstructor, CanHaveMethods) that #[wasm_bindgen] automatically implements for structs but not for enums. Then the code gen for e.g. #[wasm_bindgen(constructor)] can include a compile-time check for Self: CanHaveConstructor.

While this would enforce correctness, it's also a breaking change. All enums currently compile with constructors and instance methods, it's just that CLI fails for C-style enums.

[daxpedda]

I'm not sure what you mean here exactly. Considering that only self methods are allowed, we could and should pass the value and not a reference to it.

I might have to look at that again, but what I meant is that the ABI is a i32 and that the generated TS type is number. The ABI of exported Rust structs for self parameters is a pointer to the Rust struct instance in wasm memory. So self, &self, and &mut self all have the same ABI for exported Rust structs.

But I haven't actually looked at the Rust code gen, so I might be wrong that it's a pointer.

You are describing how it works for structs, but it shouldn't work that way for enums, because enums can be instantiated in JS/TS without FFI'ing into Rust, they can't really be pointers. String enums were also optimized to just be a u32 by #3915 and conversion is done in JS.

That said, I have no idea how enum methods work right now and it might be buggy. Please let me know if I missed something.

The best we could do would be to do some magic with traits to cause compiler errors (basically, like C++ static_asserts), but that wouldn't be backwards compatible.

Why would that not be backwards compatible? In any case, this is how its currently done with e.g. not allowing &self methods on enums.

What I meant was that we could have internal marker traits (e.g. CanHaveConstructor, CanHaveMethods) that #[wasm_bindgen] automatically implements for structs but not for enums. Then the code gen for e.g. #[wasm_bindgen(constructor)] can include a compile-time check for Self: CanHaveConstructor.

While this would enforce correctness, it's also a breaking change. All enums currently compile with constructors and instance methods, it's just that CLI fails for C-style enums.

If the CLI fails, I would also say its a breaking change, even if maybe not technically by Rust standards. In any case, we have already decided that this is an acceptable breaking change in https://github.com/rustwasm/wasm-bindgen/pull/4258#issuecomment-2469860735. We have historically made technically breaking changes in wasm-bindgen for things that are actually broken. Which is currently the case for #[wasm_bindgen(constructor/getter/setter)]. Please correct if I'm wrong.

[RunDevelopment]

That said, I have no idea how enum methods work right now and it might be buggy. Please let me know if I missed something.

You probably didn't miss anything. I just saw that the TS type was as generated as number instead of the enum type. So I assumed that the underlying ABI was NonNull<EnumType> (which has the WASM ABI of i32).

But I again, I'll have to check the Rust code gen to be sure.

If the CLI fails, I would also say its a breaking change, even if maybe not technically by Rust standards. In any case, we have already decided that this is an acceptable breaking change in https://github.com/rustwasm/wasm-bindgen/pull/4258#issuecomment-2469860735.

Oh, okay. I thought you only agreed in reference to making it breaking for CLI users.

And just to clear, my proposed solution would result in a compiler error for all users. E.g. the following code:

#[wasm_bindgen]
pub enum Foo { A = "a", B = "b" }

#[wasm_bindgen]
impl Foo {
    #[wasm_bindgen(constructor)]
    pub fn new() -> Self { Foo::A }
}

would result in generated code like this:

pub enum Foo { A = 0, B = 1 }
impl wasm_bindgen::marker::CanHaveStaticMethod for Foo {}
// wasm_bindgen::marker::CanHaveConstrctor is not implemented

impl Foo {
    pub fn new() -> Foo {
        // compile-time assert
        const _: () = {
            fn check<T: wasm_bindgen::marker::CanHaveConstrctor>() {}
            check::<Foo>(); // will fail, because the trait is not implemented
        };

        // the usual code gen
        #[no_mangel]
        fn __wbg_export_foo_constructor() -> Foo::Abi { /* ... */ }
        /* ... */
    }
}

This would result in a compiler error for whoever is compiling the WASM binary.

I'll probably make that a separate PR to keep this PR focused on namespaces.