Add #[allow(clippy::*)] to the top-level items in codegen.rs #1207

T5uku5hi · 2019-01-25T06:06:59Z

This PR it to fix issue #1200.

I added #[allow(clippy::*)] to the top-level items in codegen.rs, but I have a problem 😣
Running clippy after fixing, warnings remain print out.
I think I misunderstand how to fix.
Please tell me the correct fixing.

fitzgen · 2019-01-25T17:17:51Z

Hi @T5uku5hi, it looks like you've introduced some syntax errors in the macro-expanded code somewhere: https://travis-ci.com/rustwasm/wasm-bindgen/jobs/172934004#L585

Unfortunately, there isn't a super great way to debug syntax errors within macro-expanded code right now. The two best ways to debug this that I know of is to comment out half of your changes, test to see if the syntax error is there or not. Keep doing this and use a binary search to try and find the code that introduced the syntax errors.

Running clippy after fixing, warnings remain print out.

You're still seeing clippy warnings with this patch, and not the syntax errors that CI has? If so, can you share the output?

T5uku5hi · 2019-01-26T03:08:24Z

@fitzgen
Thank you for your comment.
I fixed the code too much at once. I'm really sorry 😣

You're still seeing clippy warnings with this patch, and not the syntax errors that CI has? If so, can you share the output?

I got the same syntax error as the CI output result.
Here is the output.
errors_commit_e19306c.txt

I would like to consult you about next steps.
Here is the output result before my fixing.
errors_before_add_[#[allow(clippy)]].txt

At first, I'll focus on fixing to remove this warning according to your advice.

warning: this function has too many arguments (8/7)
    --> crates/js-sys/src/lib.rs:3832:15
     |
3832 | #[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

Is this next step correct?

fitzgen · 2019-01-28T17:13:07Z

If you want to try and pinpoint a single lint warning, https://github.com/dtolnay/cargo-expand might help. But I'm not 100% sure, since debugging proc-macro output is a bit suboptimal currently.

I suggest adding one #[allow(clippy::*)] at a time, and verifying that the example project builds after each one. Eventually all the #[allow(...)]s will be in place and you can push the code to this PR :)

T5uku5hi · 2019-01-31T15:26:32Z

@fitzgen thank you so much for your advice.
I'll try it 😊

T5uku5hi · 2019-02-03T14:05:58Z

@fitzgen
I tried to add #[allow(clippy::*)] to lines as much as possible.
And then, I found error messages changed when I added #[allow(clippy::*)] to 902 line.
　
Before
errors_before_add_allow_clippy.txt
After
errors_after_add_allow_clippy.txt
　
I thought it seems a big step in fixing this issue, but I don't know what to do next 😣
Please advice me.

fitzgen · 2019-02-06T09:08:18Z

Are these errors from when you run cargo clippy in the example project?

limira · 2019-02-06T14:29:20Z

Are these errors from when you run cargo clippy in the example project?

I think the answer is 'yes'. Here is an excerpt from errors_after_add_allow_clippy.txt

request-animation-frame $ cargo clippy
   Compiling wasm-bindgen-backend v0.2.33 (/Users/misakimakino/Works/clones/fork-wasm-bindgen/wasm-bindgen/crates/backend)
   Compiling wasm-bindgen-macro-support v0.2.33 (/Users/misakimakino/Works/clones/fork-wasm-bindgen/wasm-bindgen/crates/macro-support)
   Compiling wasm-bindgen-webidl v0.2.27 (/Users/misakimakino/Works/clones/fork-wasm-bindgen/wasm-bindgen/crates/webidl)
   Compiling wasm-bindgen-macro v0.2.33 (/Users/misakimakino/Works/clones/fork-wasm-bindgen/wasm-bindgen/crates/macro)
   Compiling web-sys v0.3.10 (/Users/misakimakino/Works/clones/fork-wasm-bindgen/wasm-bindgen/crates/web-sys)
    Checking wasm-bindgen v0.2.33 (/Users/misakimakino/Works/clones/fork-wasm-bindgen/wasm-bindgen)

T5uku5hi · 2019-02-06T22:52:45Z

@fitzgen
Yes.
When I run cargo clippy in request-animation-frame dir, these errors occurred.

fitzgen · 2019-02-08T09:42:34Z

crates/backend/src/codegen.rs

@@ -899,6 +899,7 @@ impl TryToTokens for ast::ImportFunction {
            abi_arguments.push(quote! { #exn_data_ptr: *mut u32 });
            convert_ret = quote! { Ok(#convert_ret) };
            exceptional_ret = quote! {
+                #[allow(clippy::*)]


Ah I see -- we should only add #[allow(clippy::*)] to things that are top-level items in the expanded code, not anything that is at the top of a quote! macro invocation, since it might take multiple quotes to build up a top-level item that we are emitting.

A good rule of thumb is that we should add it on each fn we define.

Does that make sense?

Yes!! I understand you said.
I tried to add #[allow(clippy::*)] to things that are top-level items at first,
but this code occurred another error like this.

error: expected one of `(`, `)`, `,`, or `=`, found `::` --> crates/backend/src/codegen.rs:15:15 | 15 | #[allow(clippy::*)] | ^^ expected one of `(`, `)`, `,`, or `=` here error: unexpected token: `)` --> crates/backend/src/codegen.rs:15:18 | 15 | #[allow(clippy::*)] | ^ unexpected token after this error: aborting due to 2 previous errors

　
I found that I should use #[allow(clippy::drop_ref)] instead of #[allow(clippy::*)] when I read codes written by others 🧐💡
Finally, I fixed it.

@T5uku5hi, did you try #![allow(clippy::all)]?

@limira
Oh! I didn't try all instead of *.
I tried it just now and I well did it!

I'll add #[allow(clippy::all)] in all the top-level items.
Thank you for your kindness 😊

This is my bad, sorry!

For some reason I thought the syntax was allow(clippy::*) and it is actually allow(clippy::all) as @limira points out. Sorry about that!

T5uku5hi · 2019-02-10T11:25:42Z

@fitzgen
I added #[allow(clippy::all)] to the top-level items in codegen.rs.

fitzgen

Wonderful! Thanks for sticking with this, and sorry again for the mixup!

add #[allow(clippy::*)] in the top-level items

e19306c

alexcrichton requested a review from fitzgen January 25, 2019 15:04

alexcrichton assigned fitzgen Jan 25, 2019

T5uku5hi added 4 commits February 3, 2019 13:49

fixed conflict

2230548

comment out the all #[allow(clippy::*)] lines

d3a5234

add #[allow(clippy::*)] to 901 line

51f7756

remove unnecessary changes

fb8e158

fitzgen reviewed Feb 8, 2019

View reviewed changes

T5uku5hi added 3 commits February 9, 2019 03:13

Merge remote-tracking branch 'upstream/master' into add-allow-clippy

1b76410

add #[allow(clippy::drop_ref)]

9167893

add #[allow(clippy::all)] in top-level items.

5c0da88

fitzgen approved these changes Feb 10, 2019

View reviewed changes

fitzgen merged commit 499ae12 into rustwasm:master Feb 10, 2019

fitzgen added the TWiRaWA Nominate this PR for inclusion in the next issue of This Week in Rust and WebAssembly label Feb 12, 2019

fitzgen mentioned this pull request Feb 12, 2019

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add #[allow(clippy::*)] to the top-level items in codegen.rs #1207

Add #[allow(clippy::*)] to the top-level items in codegen.rs #1207

T5uku5hi commented Jan 25, 2019

fitzgen commented Jan 25, 2019

T5uku5hi commented Jan 26, 2019

fitzgen commented Jan 28, 2019

T5uku5hi commented Jan 31, 2019

T5uku5hi commented Feb 3, 2019

fitzgen commented Feb 6, 2019

limira commented Feb 6, 2019

T5uku5hi commented Feb 6, 2019 •

edited

Loading

fitzgen Feb 8, 2019

T5uku5hi Feb 8, 2019

limira Feb 9, 2019

T5uku5hi Feb 9, 2019

fitzgen Feb 10, 2019

T5uku5hi commented Feb 10, 2019

fitzgen left a comment

Add #[allow(clippy::*)] to the top-level items in codegen.rs #1207

Add #[allow(clippy::*)] to the top-level items in codegen.rs #1207

Conversation

T5uku5hi commented Jan 25, 2019

fitzgen commented Jan 25, 2019

T5uku5hi commented Jan 26, 2019

fitzgen commented Jan 28, 2019

T5uku5hi commented Jan 31, 2019

T5uku5hi commented Feb 3, 2019

fitzgen commented Feb 6, 2019

limira commented Feb 6, 2019

T5uku5hi commented Feb 6, 2019 • edited Loading

fitzgen Feb 8, 2019

Choose a reason for hiding this comment

T5uku5hi Feb 8, 2019

Choose a reason for hiding this comment

limira Feb 9, 2019

Choose a reason for hiding this comment

T5uku5hi Feb 9, 2019

Choose a reason for hiding this comment

fitzgen Feb 10, 2019

Choose a reason for hiding this comment

T5uku5hi commented Feb 10, 2019

fitzgen left a comment

Choose a reason for hiding this comment

T5uku5hi commented Feb 6, 2019 •

edited

Loading