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

Merged

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
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Amazing improvement, great work!

Missing a changelog entry.

Cargo.toml Outdated Show resolved Hide resolved
crates/backend/src/codegen.rs Outdated Show resolved Hide resolved
crates/macro/ui-tests/main-debug.stderr Outdated Show resolved Hide resolved
Comment on lines +6 to +22

warning: unreachable expression
--> ui-tests/main-infallible.rs:4:1
|
4 | #[wasm_bindgen(main)]
| ^^^^^^^^^^^^^^^^^^^^^
| |
| unreachable expression
| any code following this expression is unreachable
|
note: this expression has type `Infallible`, which is uninhabited
--> ui-tests/main-infallible.rs:4:1
|
4 | #[wasm_bindgen(main)]
| ^^^^^^^^^^^^^^^^^^^^^
= note: `#[warn(unreachable_code)]` on by default
= note: this warning originates in the attribute macro `wasm_bindgen` (in Nightly builds, run with -Z macro-backtrace for more info)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fun. That's a bug. We should avoid emitting this warning somehow.
Not related to this PR.

src/lib.rs Outdated Show resolved Hide resolved
src/marker.rs Outdated Show resolved Hide resolved
src/marker.rs Outdated Show resolved Hide resolved
src/marker.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

I just remembered: crates/msrv lib and resolver need to be fixed:

default = ["std"]

Should instead be:

default = [
  "wasm-bindgen/default",
  "js-sys/default",
  "wasm-bindgen-futures/default",
  "web-sys/default",
  "wasm-bindgen-test/default",
]

crates/backend/src/codegen.rs Outdated Show resolved Hide resolved
@RunDevelopment
Copy link
Contributor Author

I just remembered: crates/msrv lib and resolver need to be fixed:

Please do that in a separate PR. This has nothing to do with the changes in this PR.

@daxpedda
Copy link
Collaborator

daxpedda commented Dec 8, 2024

I just remembered: crates/msrv lib and resolver need to be fixed:

Please do that in a separate PR. This has nothing to do with the changes in this PR.

default = ["std"] worked because the only relevant default crate feature was std. This PR adds a new default crate feature with new code paths. That has to be included in the tests.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Missing a changelog entry and a couple of nits, otherwise LGTM!

Great work!

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@RunDevelopment
Copy link
Contributor Author

Okay, together with #4345, I think I addressed all review relevant comments now.

Cargo.toml Show resolved Hide resolved
@daxpedda daxpedda merged commit 83a2ff4 into rustwasm:main Dec 9, 2024
67 checks passed
@RunDevelopment
Copy link
Contributor Author

Thank you for the help @daxpedda!

@RunDevelopment RunDevelopment deleted the error-enum-getter-setter-constructor branch December 9, 2024 00:29
daxpedda pushed a commit to daxpedda/wasm-bindgen that referenced this pull request Dec 19, 2024
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