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 3 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.

Comment on lines +796 to +799
// change span of class, so it points to the location of the
// function causing the assert to fail
let mut class = class.clone();
class.set_span(self.rust_name.span());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether this is hygienic (or even correct). I did this so that the error message would point to the function causing the error instead of the type name on the impl block. This seems hacky, so please let me know if there are better ways of doing this.

@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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant