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

Add #[allow(clippy::*)] to all our macro-generated Rust code #1200

Closed
fitzgen opened this issue Jan 22, 2019 · 4 comments
Closed

Add #[allow(clippy::*)] to all our macro-generated Rust code #1200

fitzgen opened this issue Jan 22, 2019 · 4 comments
Labels
assigned This issue has been assigned to a contributor codegen Issues related to emitting code at the end of the wasm-bindgen pipeline good first issue This is a good issue for people who have never contributed to wasm-bindgen before help wanted We could use some help fixing this issue!

Comments

@fitzgen
Copy link
Member

fitzgen commented Jan 22, 2019

What

Using wasm-bindgen with clippy results in warnings like those reported in #1112 and like:

error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
 --> src/change_list.rs:9:5
  |
9 |     #[wasm_bindgen]
  |     ^^^^^^^^^^^^^^^
  |
  = note: #[deny(clippy::drop_ref)] on by default
  = note: argument has type &wasm_bindgen::JsValue
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#drop_ref

You can test it out by installing clippy:

rustup self update
rustup component add clippy

and then running clippy on wasm-bindgen/examples/request-animation-frame:

cd wasm-bindgen/examples/request-animation-frame/
cargo clippy

This should print out warnings like

warning: this function has too many arguments (8/7)
    --> crates/js-sys/src/lib.rs:3798:15
     |
3798 | #[wasm_bindgen]
     |               ^
     |
     = note: #[warn(clippy::too_many_arguments)] on by default
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments

We would ideally like there to be zero clippy warnings in code generated by the #[wasm-bindgen] macro. (Note that tehre are unrelated clippy warnings in wasm-bindgen's general code base, which we are less interested in. The goal is to support folks using clippy on projects that use wasm-bindgen.)

How to fix

In all the top-level items we create with the quote! { ... } macro in wasm-bindgen/crates/backend/src/codegen.rs, we need to add #[allow(clippy::*)] to the top-level item (function or static, etc...).

@fitzgen fitzgen added help wanted We could use some help fixing this issue! good first issue This is a good issue for people who have never contributed to wasm-bindgen before codegen Issues related to emitting code at the end of the wasm-bindgen pipeline labels Jan 22, 2019
@T5uku5hi
Copy link
Contributor

I’d like to pick this issue up!

@fitzgen
Copy link
Member Author

fitzgen commented Jan 24, 2019

Let me know if you have any questions!

@fitzgen fitzgen added the assigned This issue has been assigned to a contributor label Jan 24, 2019
@T5uku5hi
Copy link
Contributor

Thank you so much!

I tried to fix but I have a problem.
Please read my PR.

@fitzgen fitzgen closed this as completed Feb 12, 2019
@fitzgen
Copy link
Member Author

fitzgen commented Feb 12, 2019

Fixed in #1207 -- thanks @T5uku5hi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned This issue has been assigned to a contributor codegen Issues related to emitting code at the end of the wasm-bindgen pipeline good first issue This is a good issue for people who have never contributed to wasm-bindgen before help wanted We could use some help fixing this issue!
Projects
None yet
Development

No branches or pull requests

2 participants