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

Improved errors for invalid self arguments #4276

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

Conversation

RunDevelopment
Copy link
Contributor

@RunDevelopment RunDevelopment commented Nov 17, 2024

While writing tests, I forgot to add #[wasm_bindgen] on an impl block and the proc macro panicked with the message "arguments cannot be self". Since this was a panic, I tried to debug the proc macro for like 10 minutes before I noticed the missing #[wasm_bindgen] in my test.

This PR improves the error messages for self arguments in invalid positions. Instead of panics, users now get proper errors with helpful error messages.

Before:

error: custom attribute panicked
  --> ui-tests/invalid-self.rs:10:5
   |
10 |     #[wasm_bindgen(js_name = bar)]
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: message: arguments cannot be `self`

After:

error: the `self` argument is only allowed for functions in `impl` blocks.

       If the function is already in an `impl` block, did you perhaps forget to add `#[wasm_bindgen]` to the `impl` block?
  --> ui-tests/invalid-self.rs:11:17
   |
11 |     pub fn foo(&self) {}
   |                 ^^^^

For imported/extern functions with a self argument, I added a similar error message that suggests using this instead of self.

These new error messages should (1) make these issues easier to fix for our users and (2) make it clear that it's an error with user code and not the macro.


To implement the better error messages, I initially added a new parameter to function_from_decl to hold the function position (free, extern, impl), but realized that this single parameter could represent allow_self: bool, self_ty: Option<&Ident>, and is_from_impl: bool all in a single parameter. So I did that. In total, I added 1 new parameter and removed 3 old ones from function_from_decl. This makes the diff a bit larger, but I couldn't resist when it cleaned up the API of function_from_decl so nicely.

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