Make getters, setters, and constructors compiler errors for enums#4278
Conversation
|
So that's unfortunate. 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 |
|
We established over chat that we want to use |
|
Alright, I added a new (Btw, 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. |
daxpedda
left a comment
There was a problem hiding this comment.
Amazing improvement, great work!
Missing a changelog entry.
|
|
||
| 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) |
There was a problem hiding this comment.
Fun. That's a bug. We should avoid emitting this warning somehow.
Not related to this PR.
daxpedda
left a comment
There was a problem hiding this comment.
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",
]
Please do that in a separate PR. This has nothing to do with the changes in this PR. |
|
daxpedda
left a comment
There was a problem hiding this comment.
Missing a changelog entry and a couple of nits, otherwise LGTM!
Great work!
Co-authored-by: daxpedda <daxpedda@gmail.com>
|
Okay, together with #4345, I think I addressed all review relevant comments now. |
|
Thank you for the help @daxpedda! |
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 bystructs with#[wasm_bindgen]and notenums. Exported function then generate aconst _: ()that checks whether theSelftype (if any) implements those traits. I useddiagnostic::on_unimplementedto make the trait errors easily understandable for users.Example error for using
#[wasm_bindgen(constructor)]on an enum calledRustEnum: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
Selftype 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.