Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make getters, setters, and constructors compiler errors for enums #4278

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

RunDevelopment
Copy link
Contributor

This PR implements a compile-time check to verify that enums have no methods with #[wasm_bindgen(getter)], #[wasm_bindgen(setter)], and #[wasm_bindgen(constructor)]. This was previously accepted and resulted in incorrect JS code gen.

The check is implemented like I outlined in this comment. There are 3 new internal marker traits (SupportsConstructor, SupportsInstanceProperty, SupportsStaticProperty) that are implemented by structs with #[wasm_bindgen] and not enums. Exported function then generate a const _: () that checks whether the Self type (if any) implements those traits. I used diagnostic::on_unimplemented to make the trait errors easily understandable for users.

Example error for using #[wasm_bindgen(constructor)] on an enum called RustEnum:

error[E0277]: JavaScript constructors are not supported for `RustEnum`
  --> ui-tests/unsupported-options.rs:56:12
   |
56 |     pub fn new() -> Self {
   |            ^^^ this function cannot be the constructor of `RustEnum`
   |
   = help: the trait `SupportsConstructor` is not implemented for `RustEnum`
   = note: `#[wasm_bindgen(constructor)]` is generally only supported for `struct`s with `#[wasm_bindgen]` and cannot be used for `enum`s.
   = note: Consider removing the `constructor` option and using a regular static method instead.
   = help: the trait `SupportsConstructor` is implemented for `RustStruct`
note: required by a bound in `__wasm_bindgen_generated_RustEnum_new::_::assert_supports_constructor`
  --> ui-tests/unsupported-options.rs:49:1
   |
49 | #[wasm_bindgen]
   | ^^^^^^^^^^^^^^^ required by this bound in `assert_supports_constructor`
   = note: this error originates in the attribute macro `wasm_bindgen` (in Nightly builds, run with -Z macro-backtrace for more info)

As we can see, the error message is clear and even offers a suggestion to fix the error.


This PR is virtually a minimal implementation of a technique we could use a lot more. E.g. it can be used to verify that the Self type actually has a #[wasm_bindgen] macro (the test file I added includes an example where exactly this case does not cause an error). However, I kept it minimal so we can concentrate on the "how" this PR does things.

As for the how: I initially wanted to add just a single trait (something like IsStruct) and then let constructors/getters/setters/etc. assert that they are on a struct. This did work, but resulted in error messages that were a lot worse. By using traits that describe the capabilities of the type and not what the type is, error messages can be a lot more specific. Plus, adding/removing/changing capabilities seems easier at scale.

@RunDevelopment
Copy link
Contributor Author

So that's unfortunate. #[diagnostic::on_unimplemented] was only stabilized in Rust 1.78.0. So we can't use with our MSRV.

So either we give up on good error messages, or we conditionally enable the attribute only for Rust versions that support it like zerocopy does (see their build.rs).

@daxpedda daxpedda added the waiting for author Waiting for author to respond label Nov 28, 2024
@daxpedda daxpedda marked this pull request as draft November 28, 2024 22:42
@daxpedda
Copy link
Collaborator

We established over chat that we want to use rustversion behind a crate feature enabled by default.

@RunDevelopment
Copy link
Contributor Author

Alright, I added a new disagnostic feature to gate the rustversion dependency, and enable the diagnostic::on_unimplemented attributes only when supported and wanted.

(Btw, rustversion itself is a joy to work with. It's so easy to use, I love it.)

Another change is that I raised the rust version we use to run UI tests in CI from 1.76 to 1.78. This is necessary for the CI to actually test the custom trait impl error messages. However, this also caused a few other UI tests to slightly change their output, so I had to update their output as well. Hence this PR touches a few unrelated test files.

@RunDevelopment RunDevelopment marked this pull request as ready for review November 29, 2024 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author Waiting for author to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants