Add export_type attribute to export string enums

[RunDevelopment]

Fixes #2153

This gives users control over whether the type of string enums is exported via a new export_type attribute exclusive for string enums.

I also added documentation on why the attribute exists and how it works. I intentionally kept the description of this PR short, because the docs should explain the behavior of the attribute. If any questions arise regarding the behavior/function of the attribute during the review process, I will see this as the docs being insufficient.

[RunDevelopment]

wtf is going on here? CI fails with this message:

---- schema_hash_approval::schema_version stdout ----
thread 'schema_hash_approval::schema_version' panicked at crates/shared/src/schema_hash_approval.rs:15:5:
assertion `left == right` failed
  left: "11946883356319465460"
 right: "211103844299778814"

But the constant being assert!ed (APPROVED_SCHEMA_FILE_HASH) has the value "18290232323238297705". What is going on here?!

[RunDevelopment]

One rebase later and the nature order is restored. My bad.

[daxpedda]

So I had a different proposal a while ago. We might want to differentiate between imported and exported enums, which seems a bit strange considering JS doesn't actually have enums.

The idea is that we need to make it clear what types are actually exported into JS/TS by wasm-bindgen. Currently every struct and function marked with #[wasm_bindgen] is exported. Making special rules around enums, as is already the place with numeric enums, seems counter-productive and I would instead like to fix it at the root.

It seems to me that the case to have enums which we don't want to export is if we want to "import enums that already exist", e.g. in web-sys.

Here is my suggestion:

extern "C" {
    pub enum Foo {
        Bar = "Bar",
    }

This is an "imported" enum and will not be exported in JS/TS, but can still be correctly represented in JS/TSDoc and TS signatures as a type. This works quite well with web-sys as TS should recognize all these enums, but I don't know how this works exactly with #[wasm_bindgen(module = "...")] extern "C" { ... }.

Let me know what you think!

EDIT: I don't actually know if Rust or syn will allow us to parse something like this, which would also be important to figure out.

[RunDevelopment]

Firstly, I would like to point out that your proposed solution would repeat #4163, so it's a breaking change.

I also don't like the idea of putting them into extern blocks, since enums aren't really externally defined. C-style and string enums essentially defined, in a sense, a primitive data type. So they are as externally defined as the concept of a 64-bit floating-pointer number is externally defined, IMO.

Currently every struct and function marked with #[wasm_bindgen] is exported. Making special rules around enums, as is already the place with numeric enums, seems counter-productive and I would instead like to fix it at the root.

What special rules? Right now, only string enums are inconsistent since they aren't exported. C-style enums being exported is consistent with structs being exported.

---

How about this idea: the main usability problem here is that WBG has to decide for the user whether a string enum (or any other thing for that matter) is exported or imported. WBG currently decides with via extern blocks, and you proposed to reuse that mechanism. However, what if we allow users to just explicitly tell us whether something is exported (= should be part of the public API of the generated JS)?

Maybe something like this:

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

#[wasm_bindgen(export)] { // export section
   Foo = true,
   Bar = false,
}

// short-hand for `Baz = true` in the export section
#[wasm_bindgen(export)]
pub enum Baz { C = "c" }

This would give users very direct control over the API of their JS package.

But the main reason I want something like this is that it gives users control over third-party types. Even if a user wants to export a string enum defined by a third-party library, they currently just can't.

As I see it, the goal is to give users control over the public API of their JS packages, so why not be explicit about it instead of going through the notion of imported/exported types?

This would also allow us to make guesses. E.g. if a string enum is used by a publicly exported function, the string enum should probably be publicly exported too. If we guessed wrong and the user wanted to keep it internal, they can then make it so.

What do you think about this approach?

[daxpedda]

Firstly, I would like to point out that your proposed solution would repeat #4163, so it's a breaking change.

The idea is to let web-sys move to this new pattern as well, avoiding this issue.

I also don't like the idea of putting them into extern blocks, since enums aren't really externally defined. C-style and string enums essentially defined, in a sense, a primitive data type. So they are as externally defined as the concept of a 64-bit floating-pointer number is externally defined, IMO.

I agree, this is a compromise in lack of a better solution.

Currently every struct and function marked with #[wasm_bindgen] is exported. Making special rules around enums, as is already the place with numeric enums, seems counter-productive and I would instead like to fix it at the root.

What special rules? Right now, only string enums are inconsistent since they aren't exported. C-style enums being exported is consistent with structs being exported.

I meant that the proposed change of this PR would not make it consistent with the rest. As you point out, numeric enums are already consistent.

However, what if we allow users to just explicitly tell us whether something is exported (= should be part of the public API of the generated JS)?

Something like this would be indeed great. However while we can do this just fine with string enums, because they aren't exported, we can't do it for everything else. So I would rather move to something like this consistently in the next breaking change.

Just dropping some thoughts about this topic: Rust itself has no mechanism for native libraries on how to handle what is exported and what is not from dependencies. So I see this more as a Rust problem. In the future, the component model would address this quite well. So I would argue instead of coming up with more out-of-the-ordinary solutions for this problem, moving to the component model would be more appropriate.

[RunDevelopment]

So I would rather move to something like this consistently in the next breaking change.

Oh, then I think I miscommunicated a bit. My goal for this PR was to have a solution without a breaking change.

But you are correct that the export_type attribute would only serve as a temporary solution and be removed when we implement a proper solution in the next major-ish version.

I also thought of a different solution to this problem based on a heuristic and without an attribute. We could just assume that if a string enum is used in an exported function, the string enum is probably exported as well. This wouldn't remove the weirdness of string enums, but should correctly export (and not export) string enums in the majority of use cases.

Rust itself has no mechanism for native libraries on how to handle what is exported and what is not from dependencies.

Can't you just pub use dep; to re-export a dependency? Not that this would help us, since WBG exports are determined by which code is compiled.

[daxpedda]

So I would rather move to something like this consistently in the next breaking change.

Oh, then I think I miscommunicated a bit. My goal for this PR was to have a solution without a breaking change.

But you are correct that the export_type attribute would only serve as a temporary solution and be removed when we implement a proper solution in the next major-ish version.

I also thought of a different solution to this problem based on a heuristic and without an attribute. We could just assume that if a string enum is used in an exported function, the string enum is probably exported as well. This wouldn't remove the weirdness of string enums, but should correctly export (and not export) string enums in the majority of use cases.

I understand that you want a solution without a breaking change. My concern is that whatever solution we make should be consistent across all things: functions, structs and enums. This is why I am proposing the extern "C" { pub enum Foo { ... } } idea.

So, e.g., if you want to use a heuristic for string enums to figure out whats exported and what is not, I would like to see this applied to numeric enums, structs and functions as well. Which is probably quite hard to design without a breaking change.

Rust itself has no mechanism for native libraries on how to handle what is exported and what is not from dependencies.

Can't you just pub use dep; to re-export a dependency? Not that this would help us, since WBG exports are determined by which code is compiled.

Apologies, with native libraries I meant libraries that are not consumed via Cargo, but via static or dynamic linking, e.g. going through FFI. This is the mechanism that WBG uses after all.

So while Rust dependencies have an excellent interface, as you said, pub use dep;, FFI via extern "C" fn does not.

[RunDevelopment]

I understand that you want a solution without a breaking change. My concern is that whatever solution we make should be consistent across all things: functions, structs and enums.

I wanted to have a temporary solution that improves things until we can implement a proper (but breaking) solution. So I'm not concerned about it not being consistent, because it's only supposed to make the current situation better enough to resolve #2153, not fix it entirely.

Yes, a proper solution would be even better, but absent a new major-ish version, that's not possible. Right now, we have a lot of user code that relies upon the consistency, so we cannot fix it without breaking things.

[daxpedda]

I understand that you want a solution without a breaking change. My concern is that whatever solution we make should be consistent across all things: functions, structs and enums.

I wanted to have a temporary solution that improves things until we can implement a proper (but breaking) solution. So I'm not concerned about it not being consistent, because it's only supposed to make the current situation better enough to resolve #2153, not fix it entirely.

Yes, a proper solution would be even better, but absent a new major-ish version, that's not possible. Right now, we have a lot of user code that relies upon the consistency, so we cannot fix it without breaking things.

I understand, but this is where I am coming from. In particular, I outlined a solution in #4260 (comment) that should solve #2153 while keeping everything consistent. I'm not sure what the downsides are except that its not Rusty, which is definitely bad.

The new system you are proposing is interesting, but I'm only in favor of it if we can apply it to structs, functions and numeric enums equally. So this proposal would need some significant evolution in figuring out how to deprecate the old system without a breaking change. You would also need to figure out how to make sure this is only used by the application and not a library. But maybe that's fine because the same could be said about std::panic::set_hook. Off the top of my head there is also the issue that some libraries, dealing with spawning workers, need to export some types to function correctly, so we have to also account for types marked with e.g. "forced" that give some sort of error if users try to stop exporting them. It should also be noted that this is basically replicating the component model proposal, which is sad, but maybe that's all we can do for now.

I hope you understand where I'm coming from your proposal would require significant amount of design work and its not just solving the original issue, but would be a huge feature that touches a lot of other stuff.

Maybe we can find a different solution that we can apply to just enums, so it doesn't necessarily have to be completely consistent with functions and structs. The problem is that its just going to be tough to figure something out without breaking changes to numeric enums, which already export by default. Otherwise your export_type solution would have worked just fine.

[RunDevelopment]

I outlined a solution in #4260 (comment) that should solve #2153 while keeping everything consistent. I'm not sure what the downsides are except that its not Rusty, which is definitely bad.

The main downside is that it's a breaking change. If we implement what you are proposing, then all string enums (which currently behave like the imported enums you are proposing) will be exported. This is a breaking change as we found out with #4163. Even if we then update web-sys, all third-party libraries that we don't control will still be broken in that all their string enums will then be exported.

I hope you understand where I'm coming from your proposal would require significant amount of design work and its not just solving the original issue, but would be a huge feature that touches a lot of other stuff.

Certainly. That's also why I'm not saying that we should do it now. What I basically meant to say is that if we're already breaking things (like your proposed solution is), then we may as well fix an entire class of problems once and for all.

The problem is that its just going to be tough to figure something out without breaking changes to numeric enums, which already export by default. Otherwise your export_type solution would have worked just fine.

I don't understand what you mean. export_type is not a valid option for C-style enums. This PR only affects whether the type of string enums is exported and nothing else. Nothing changes for C-style enums.

Right now, export_type on a C-style enum will result in warning for an unused option, but I can also make it a hard error if you want.

[daxpedda]

I outlined a solution in #4260 (comment) that should solve #2153 while keeping everything consistent. I'm not sure what the downsides are except that its not Rusty, which is definitely bad.

The main downside is that it's a breaking change. If we implement what you are proposing, then all string enums (which currently behave like the imported enums you are proposing) will be exported. This is a breaking change as we found out with #4163. Even if we then update web-sys, all third-party libraries that we don't control will still be broken in that all their string enums will then be exported.

I hope you understand where I'm coming from your proposal would require significant amount of design work and its not just solving the original issue, but would be a huge feature that touches a lot of other stuff.

Certainly. That's also why I'm not saying that we should do it now. What I basically meant to say is that if we're already breaking things (like your proposed solution is), then we may as well fix an entire class of problems once and for all.

This isn't a breaking change per se, its just very undesirable. But I really this can't be avoided, we just have to find an actual "long-term" solution. This is why we had to address #4163, because we didn't anticipate it and did no design work, if we start exporting string enums that would be something we can't revert without a breaking change, so we have to get it right. Basically I don't want to repeat the mistake we did with numeric enums, which is causing us to be limited by making string enums work the same to be consistent, even if the outcome is undesirable to begin with.

Apologies for any misunderstandings, just reading through my comments in #4163, I got some things wrong. Was definitely only running on fumes that night :/

The problem is that its just going to be tough to figure something out without breaking changes to numeric enums, which already export by default. Otherwise your export_type solution would have worked just fine.

I don't understand what you mean. export_type is not a valid option for C-style enums. This PR only affects whether the type of string enums is exported and nothing else. Nothing changes for C-style enums.

Right now, export_type on a C-style enum will result in warning for an unused option, but I can also make it a hard error if you want.

I was trying to say that I am not in favor of the export_type solution because it would make string enums and numeric enums inconsistent. Numeric enums would be exported just with #[wasm_bindgen], but string enums would require #[wasm_bindgen(export_type)]. Really the kind of inconsistency we should avoid imo.

---

Going with my earlier suggestion:

Maybe we can find a different solution that we can apply to just enums, so it doesn't necessarily have to be completely consistent with functions and structs.

We could do the reverse of this PR, basically something like #4173, but also not exporting JS (which is why skip_typescript didn't work). So maybe no_export? This would keep string enums and numeric enums consistent, introduce no breaking change and can be applied to all web-sys types.

WDYT?