Add support for static enum methods via TS namespaces #4258
Conversation
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 If we want users to use a separate 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 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. |
We should definitely not do this here, instead lets resolve #4260 first. |
Uhm, please reconsider. Okay, so here are some details on how string enum methods """work""" in JS. Firstly, string enums don't implement Detailed example (it's a lot of code but necessary)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) {}
}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);
}
}(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:
So to actually use a class generated for a string enum, the user would have to either overwrite the constructor or manually initialize the 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 Then in the next major version, we can make it hard error and tell people to use a separate What do you think?
On the TS side, it's pretty hacky. It's not that getting getters+setters to work on a 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.
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. |
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 I think it would also be nice if we emit a warning for every function in an
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.
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? |
|
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?
Sounds good 👍 As for not exporting those functions and emitting warning, I think this should be a separate PR.
I kind of agree, but the problem is that static getters and setters already work today for string enums.
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. |
I don't mind either way as well, but I would go with fixing string enums first.
Strange, I couldn't confirm this. What did you do to reproduce this exactly? |
Codeuse 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) {}
}/* tslint:disable */
/* eslint-disable */
type Foo = "a" | "b";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. |
Right, I forgot to mark the methods 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 I think this is what you meant from the start, I was just never considering the explicit I think we resolved all the remaining points, let me know if something is still standing. |
|
So to summarize, the plan for now is to:
|
|
Yes, except for:
Which we should do in a follow-up PR, first figuring out if this is something we want to support in the first place. |
|
Okay:
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 Codeuse 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)
}
}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? |
There was a problem hiding this comment.
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.
This should probably be a follow-up PR though.
What do you think?
Yes, lets do it, but I guess we can't do it for string enums until we resolve #4260, because they can't be instantiated.
EDIT: I guess they can, but only through return values from other functions and not directly.
| /// I will be ignored by the generated JS bindings. | ||
| pub fn is_lossless(self) -> bool { | ||
| matches!(self, ImageFormat::PNG | ImageFormat::GIF) | ||
| } |
There was a problem hiding this comment.
This should instead be outputted as-is by the proc-macro and not generate any extern "C" fn blocks.
Alternatively, instead of going through that work, you can go ahead and implement instance methods as you have suggested.
crates/cli-support/src/js/mod.rs
Outdated
| let msg = format!( | ||
| "The enum `{}` cannot support the instance method `{}`. \ | ||
| No binding will be generated for this method. \ | ||
| Consider moving the method in an `impl` block with the `#[wasm_bindgen]` attribute to avoid exporting it, \ | ||
| or making it a static method by replacing `self` with `value: Self`.", | ||
| ns.name, | ||
| name | ||
| ); | ||
| println!("WARNING: {}", msg); |
There was a problem hiding this comment.
The warning has to happen in the proc-macro, not in the CLI.
Warnings in proc-macros are a bit tough, so its not a requirement for the PR.
crates/cli-support/src/js/mod.rs
Outdated
| bail!( | ||
| "constructor is not supported on `{}`. Enums do not support exported constructors.", | ||
| class | ||
| ); |
There was a problem hiding this comment.
This has to happen in the proc-macro instead. In which case this becomes unreachable!().
A UI test would be great as well.
There was a problem hiding this comment.
That will be difficult. We don't know whether a type is a string or an enum until the whole program is parsed.
There was a problem hiding this comment.
Yeah, that's bad ... I guess that explains why this was implemented how it was until now.
I guess the best we can do now is emit a warning in wasm-bindgen, but erroring out is not good. Not all library authors actually run wasm-bindgen-cli in their CI, they just cargo build --target wasm32-unknown-unknown and call it a day.
There was a problem hiding this comment.
Just remembered that this can actually be done via the ClassMarker (or w/e its called, on my phone rn).
There was a problem hiding this comment.
See my comment in #4258 (comment), this is definitely possible.
crates/cli-support/src/js/mod.rs
Outdated
| bail!( | ||
| "the getter `{}` is not supported on `{}`. Enums only support static methods on them.", | ||
| name, | ||
| class | ||
| ); |
crates/cli-support/src/js/mod.rs
Outdated
| bail!( | ||
| "the setter `{}` is not supported on `{}`. Enums only support static methods on them.", | ||
| name, | ||
| class | ||
| ); |
|
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 So that's what needs to be done next, I guess. |
|
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 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++ 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. |
I'm not sure what you mean here exactly. Considering that only
Why would that not be backwards compatible? In any case, this is how its currently done with e.g. not allowing |
I might have to look at that again, but what I meant is that the ABI is a But I haven't actually looked at the Rust code gen, so I might be wrong that it's a pointer.
What I meant was that we could have internal marker traits (e.g. 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. |
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 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.
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 #4258 (comment). We have historically made technically breaking changes in |
You probably didn't miss anything. I just saw that the TS type was as generated as But I again, I'll have to check the Rust code gen to be sure.
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. |
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:
Now generates the following JS and type definitions:
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. IfObject.freezewas 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:
Exported methods with a JS class that points to an enum or string enum will now be exported via a namespace.
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 bestnamespaces can do is toexport const/letlike 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?
Explicitly type what type of expressions can be exported in
Context::export. This is done via the newExportJsenum, which supports classes, functions, expressions, and namespaces. Instead of making theContext::exportdetect 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.