From 84d6c8584392b736801427c789504800bc638cd2 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Wed, 7 Sep 2022 00:06:54 +0100 Subject: [PATCH 1/6] Trigger warnings for unused wasm-bindgen attributes This attempts to do something similar to #3070, but without potentially dangerous fallout from strict-mode failing on all the existing code out there. Instead of forcing a compiler error like strict-mode does, this PR will internally generate unused variables with spans pointing to unused attributes, so that users get a relatively meaningful warning. Here's how the result looks like on example from #2874: ``` warning: unused variable: `typescript_type` --> tests\headless\snippets.rs:67:28 | 67 | #[wasm_bindgen(getter, typescript_type = "Thing[]")] | ^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_typescript_type` | = note: `#[warn(unused_variables)]` on by default ``` This is not 100% perfect - until Rust has a built-in `compile_warning!`, nothing is - but is a better status quo than the current one and can help users find problematic attributes without actually breaking their builds. Fixes #3038. --- crates/macro-support/src/lib.rs | 4 +- crates/macro-support/src/parser.rs | 68 +++++++++++++++++++----------- 2 files changed, 46 insertions(+), 26 deletions(-) diff --git a/crates/macro-support/src/lib.rs b/crates/macro-support/src/lib.rs index 9996849596a..b7a35ae1bed 100644 --- a/crates/macro-support/src/lib.rs +++ b/crates/macro-support/src/lib.rs @@ -35,7 +35,7 @@ pub fn expand(attr: TokenStream, input: TokenStream) -> Result true, _ => false, diff --git a/crates/macro-support/src/parser.rs b/crates/macro-support/src/parser.rs index a3148fdb184..d40eaae97b1 100644 --- a/crates/macro-support/src/parser.rs +++ b/crates/macro-support/src/parser.rs @@ -41,6 +41,8 @@ const JS_KEYWORDS: [&str; 20] = [ struct AttributeParseState { parsed: Cell, checks: Cell, + #[cfg(not(feature = "strict-macro"))] + unused_attrs: std::cell::RefCell>, } /// Parsed attributes from a `#[wasm_bindgen(..)]`. @@ -94,35 +96,40 @@ macro_rules! methods { ($(($name:ident, $variant:ident($($contents:tt)*)),)*) => { $(methods!(@method $name, $variant($($contents)*));)* - #[cfg(feature = "strict-macro")] fn check_used(self) -> Result<(), Diagnostic> { // Account for the fact this method was called ATTRS.with(|state| state.checks.set(state.checks.get() + 1)); - let mut errors = Vec::new(); - for (used, attr) in self.attrs.iter() { - if used.get() { - continue - } - // The check below causes rustc to crash on powerpc64 platforms - // with an LLVM error. To avoid this, we instead use #[cfg()] - // and duplicate the function below. See #58516 for details. - /*if !cfg!(feature = "strict-macro") { - continue - }*/ - let span = match attr { - $(BindgenAttr::$variant(span, ..) => span,)* - }; - errors.push(Diagnostic::span_error(*span, "unused #[wasm_bindgen] attribute")); + let unused = + self.attrs + .iter() + .filter_map(|(used, attr)| if used.get() { None } else { Some(attr) }) + .map(|attr| { + match attr { + $(BindgenAttr::$variant(span, ..) => { + #[cfg(feature = "strict-macro")] + { + Diagnostic::span_error(*span, "unused #[wasm_bindgen] attribute") + } + + #[cfg(not(feature = "strict-macro"))] + { + Ident::new(stringify!($name), *span) + } + },)* + } + }); + + #[cfg(feature = "strict-macro")] + { + Diagnostic::from_vec(unused.collect()) } - Diagnostic::from_vec(errors) - } - #[cfg(not(feature = "strict-macro"))] - fn check_used(self) -> Result<(), Diagnostic> { - // Account for the fact this method was called - ATTRS.with(|state| state.checks.set(state.checks.get() + 1)); - Ok(()) + #[cfg(not(feature = "strict-macro"))] + { + ATTRS.with(|state| state.unused_attrs.borrow_mut().extend(unused)); + Ok(()) + } } }; @@ -1617,12 +1624,25 @@ pub fn reset_attrs_used() { ATTRS.with(|state| { state.parsed.set(0); state.checks.set(0); + #[cfg(not(feature = "strict-macro"))] + state.unused_attrs.borrow_mut().clear(); }) } -pub fn assert_all_attrs_checked() { +pub fn check_unused_attrs(tokens: &mut TokenStream) { ATTRS.with(|state| { assert_eq!(state.parsed.get(), state.checks.get()); + #[cfg(not(feature = "strict-macro"))] + { + let unused = &*state.unused_attrs.borrow(); + tokens.extend(quote::quote! { + // Anonymous scope to prevent name clashes. + const _: () = { + #(let #unused: ();)* + }; + }); + } + let _ = tokens; }) } From 91334cc9a6ce2d5bb538a891ff01f1723895b3a3 Mon Sep 17 00:00:00 2001 From: Lukas Lihotzki Date: Wed, 7 Sep 2022 18:25:10 +0200 Subject: [PATCH 2/6] Guide users who used the suggested (invalid) fix (#1) Co-authored-by: Ingvar Stepanyan --- crates/macro-support/src/parser.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/macro-support/src/parser.rs b/crates/macro-support/src/parser.rs index d40eaae97b1..e9cfcbfa82d 100644 --- a/crates/macro-support/src/parser.rs +++ b/crates/macro-support/src/parser.rs @@ -360,7 +360,11 @@ impl Parse for BindgenAttr { attrgen!(parsers); - return Err(original.error("unknown attribute")); + return Err(original.error(if attr_string.starts_with("_") { + "unknown attribute: it's safe to remove unused attributes entirely." + } else { + "unknown attribute" + })); } } From d43124d492924c3040eeb49d1375ed7dec55bff5 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Wed, 7 Sep 2022 17:42:53 +0100 Subject: [PATCH 3/6] Deprecate strict-macro feature; update tests --- crates/macro-support/src/lib.rs | 10 ++ crates/macro-support/src/parser.rs | 98 +++++++------------ crates/macro/Cargo.toml | 2 +- crates/macro/ui-tests/unused-attributes.rs | 25 ++++- .../macro/ui-tests/unused-attributes.stderr | 42 ++++++-- 5 files changed, 104 insertions(+), 73 deletions(-) diff --git a/crates/macro-support/src/lib.rs b/crates/macro-support/src/lib.rs index b7a35ae1bed..5d808fde9b8 100644 --- a/crates/macro-support/src/lib.rs +++ b/crates/macro-support/src/lib.rs @@ -21,6 +21,16 @@ use syn::parse::{Parse, ParseStream, Result as SynResult}; mod parser; +#[cfg(feature = "strict-macro")] +const _: () = { + #[deprecated( + note = "strict-macro feature is deprecated and will be removed in a future version" + )] + const WASM_BINDGEN_STRICT_MACRO: () = {}; + + WASM_BINDGEN_STRICT_MACRO +}; + /// Takes the parsed input from a `#[wasm_bindgen]` macro and returns the generated bindings pub fn expand(attr: TokenStream, input: TokenStream) -> Result { parser::reset_attrs_used(); diff --git a/crates/macro-support/src/parser.rs b/crates/macro-support/src/parser.rs index e9cfcbfa82d..eb639353dc4 100644 --- a/crates/macro-support/src/parser.rs +++ b/crates/macro-support/src/parser.rs @@ -1,4 +1,4 @@ -use std::cell::Cell; +use std::cell::{Cell, RefCell}; use std::char; use std::str::Chars; @@ -41,8 +41,7 @@ const JS_KEYWORDS: [&str; 20] = [ struct AttributeParseState { parsed: Cell, checks: Cell, - #[cfg(not(feature = "strict-macro"))] - unused_attrs: std::cell::RefCell>, + unused_attrs: RefCell>, } /// Parsed attributes from a `#[wasm_bindgen(..)]`. @@ -96,40 +95,24 @@ macro_rules! methods { ($(($name:ident, $variant:ident($($contents:tt)*)),)*) => { $(methods!(@method $name, $variant($($contents)*));)* - fn check_used(self) -> Result<(), Diagnostic> { + fn check_used(self) { // Account for the fact this method was called - ATTRS.with(|state| state.checks.set(state.checks.get() + 1)); + ATTRS.with(|state| { + state.checks.set(state.checks.get() + 1); - let unused = - self.attrs - .iter() - .filter_map(|(used, attr)| if used.get() { None } else { Some(attr) }) - .map(|attr| { - match attr { - $(BindgenAttr::$variant(span, ..) => { - #[cfg(feature = "strict-macro")] - { - Diagnostic::span_error(*span, "unused #[wasm_bindgen] attribute") - } - - #[cfg(not(feature = "strict-macro"))] - { + state.unused_attrs.borrow_mut().extend( + self.attrs + .iter() + .filter_map(|(used, attr)| if used.get() { None } else { Some(attr) }) + .map(|attr| { + match attr { + $(BindgenAttr::$variant(span, ..) => { Ident::new(stringify!($name), *span) - } - },)* - } - }); - - #[cfg(feature = "strict-macro")] - { - Diagnostic::from_vec(unused.collect()) - } - - #[cfg(not(feature = "strict-macro"))] - { - ATTRS.with(|state| state.unused_attrs.borrow_mut().extend(unused)); - Ok(()) - } + })* + } + }) + ); + }); } }; @@ -225,7 +208,7 @@ impl BindgenAttrs { } let mut attrs: BindgenAttrs = syn::parse2(group.stream())?; ret.attrs.extend(attrs.attrs.drain(..)); - attrs.check_used()?; + attrs.check_used(); } } @@ -422,7 +405,7 @@ impl<'a> ConvertToAst for &'a mut syn::ItemStruct { let attrs = BindgenAttrs::find(&mut field.attrs)?; if attrs.skip().is_some() { - attrs.check_used()?; + attrs.check_used(); continue; } @@ -447,11 +430,11 @@ impl<'a> ConvertToAst for &'a mut syn::ItemStruct { generate_typescript: attrs.skip_typescript().is_none(), getter_with_clone: getter_with_clone || attrs.getter_with_clone().is_some(), }); - attrs.check_used()?; + attrs.check_used(); } let generate_typescript = attrs.skip_typescript().is_none(); let comments: Vec = extract_doc_comments(&self.attrs); - attrs.check_used()?; + attrs.check_used(); Ok(ast::Struct { rust_name: self.ident.clone(), js_name, @@ -673,7 +656,7 @@ impl<'a> ConvertToAst<(BindgenAttrs, &'a Option)> for syn::Fo shim: Ident::new(&shim, Span::call_site()), doc_comment, }); - opts.check_used()?; + opts.check_used(); Ok(ret) } @@ -706,7 +689,7 @@ impl ConvertToAst for syn::ForeignItemType { _ => {} } } - attrs.check_used()?; + attrs.check_used(); Ok(ast::ImportKind::Type(ast::ImportType { vis: self.vis, attrs: self.attrs, @@ -745,7 +728,7 @@ impl<'a> ConvertToAst<(BindgenAttrs, &'a Option)> for syn::Fo self.ident, ShortHash((&js_name, module, &self.ident)), ); - opts.check_used()?; + opts.check_used(); Ok(ast::ImportKind::Static(ast::ImportStatic { ty: *self.ty, vis: self.vis, @@ -781,7 +764,7 @@ impl ConvertToAst for syn::ItemFn { None, false, )?; - attrs.check_used()?; + attrs.check_used(); Ok(ret.0) } } @@ -1060,7 +1043,7 @@ impl<'a> MacroParse for &'a mut syn::ItemImpl { } } Diagnostic::from_vec(errors)?; - opts.check_used()?; + opts.check_used(); Ok(()) } } @@ -1171,7 +1154,7 @@ impl<'a, 'b> MacroParse<(&'a Ident, &'a str)> for &'b mut syn::ImplItemMethod { rust_name: self.sig.ident.clone(), start: false, }); - opts.check_used()?; + opts.check_used(); Ok(()) } } @@ -1240,7 +1223,7 @@ impl<'a> MacroParse<(&'a mut TokenStream, BindgenAttrs)> for syn::ItemEnum { attrs: _, lit: syn::Lit::Str(_), }) => { - opts.check_used()?; + opts.check_used(); return import_enum(self, program); } _ => {} @@ -1250,7 +1233,7 @@ impl<'a> MacroParse<(&'a mut TokenStream, BindgenAttrs)> for syn::ItemEnum { .js_name() .map(|s| s.0) .map_or_else(|| self.ident.to_string(), |s| s.to_string()); - opts.check_used()?; + opts.check_used(); let has_discriminant = self.variants[0].discriminant.is_some(); @@ -1364,7 +1347,7 @@ impl MacroParse for syn::ItemConst { } } - opts.check_used()?; + opts.check_used(); Ok(()) } @@ -1412,7 +1395,7 @@ impl MacroParse for syn::ItemForeignMod { } } Diagnostic::from_vec(errors)?; - opts.check_used()?; + opts.check_used(); Ok(()) } } @@ -1628,7 +1611,6 @@ pub fn reset_attrs_used() { ATTRS.with(|state| { state.parsed.set(0); state.checks.set(0); - #[cfg(not(feature = "strict-macro"))] state.unused_attrs.borrow_mut().clear(); }) } @@ -1636,17 +1618,13 @@ pub fn reset_attrs_used() { pub fn check_unused_attrs(tokens: &mut TokenStream) { ATTRS.with(|state| { assert_eq!(state.parsed.get(), state.checks.get()); - #[cfg(not(feature = "strict-macro"))] - { - let unused = &*state.unused_attrs.borrow(); - tokens.extend(quote::quote! { - // Anonymous scope to prevent name clashes. - const _: () = { - #(let #unused: ();)* - }; - }); - } - let _ = tokens; + let unused_attrs = &*state.unused_attrs.borrow(); + tokens.extend(quote::quote! { + // Anonymous scope to prevent name clashes. + const _: () = { + #(let #unused_attrs: ();)* + }; + }); }) } diff --git a/crates/macro/Cargo.toml b/crates/macro/Cargo.toml index cfe8f0f0138..671d94fc9b2 100644 --- a/crates/macro/Cargo.toml +++ b/crates/macro/Cargo.toml @@ -25,5 +25,5 @@ quote = "1.0" [dev-dependencies] trybuild = "1.0" -wasm-bindgen = { path = "../..", version = "0.2.82", features = ['strict-macro'] } +wasm-bindgen = { path = "../..", version = "0.2.82" } wasm-bindgen-futures = { path = "../futures", version = "0.4.32" } diff --git a/crates/macro/ui-tests/unused-attributes.rs b/crates/macro/ui-tests/unused-attributes.rs index de7962e4646..c829ec346b4 100644 --- a/crates/macro/ui-tests/unused-attributes.rs +++ b/crates/macro/ui-tests/unused-attributes.rs @@ -1,12 +1,31 @@ +#![deny(unused_variables)] + use wasm_bindgen::prelude::*; -struct A; +struct A {} #[wasm_bindgen] impl A { #[wasm_bindgen(method)] - #[wasm_bindgen(method)] - pub fn foo() { + pub fn foo() {} +} + +#[wasm_bindgen] +pub struct MyStruct { + hello: String, +} + +#[wasm_bindgen(getter, typescript_custom_section)] +pub const FOO: &'static str = "FOO"; + +#[wasm_bindgen(readonly)] +pub fn bar() {} + +#[wasm_bindgen(getter_with_clone)] +impl MyStruct { + #[wasm_bindgen(getter, typescript_type = "Thing[]")] + pub fn hello(&self) -> String { + self.hello.clone() } } diff --git a/crates/macro/ui-tests/unused-attributes.stderr b/crates/macro/ui-tests/unused-attributes.stderr index 12f2c01f31e..a1cc025d081 100644 --- a/crates/macro/ui-tests/unused-attributes.stderr +++ b/crates/macro/ui-tests/unused-attributes.stderr @@ -1,11 +1,35 @@ -error: unused #[wasm_bindgen] attribute - --> $DIR/unused-attributes.rs:7:20 +error: unused variable: `method` + --> ui-tests/unused-attributes.rs:9:20 | -7 | #[wasm_bindgen(method)] - | ^^^^^^ - -error: unused #[wasm_bindgen] attribute - --> $DIR/unused-attributes.rs:8:20 +9 | #[wasm_bindgen(method)] + | ^^^^^^ help: if this is intentional, prefix it with an underscore: `_method` + | +note: the lint level is defined here + --> ui-tests/unused-attributes.rs:1:9 | -8 | #[wasm_bindgen(method)] - | ^^^^^^ +1 | #![deny(unused_variables)] + | ^^^^^^^^^^^^^^^^ + +error: unused variable: `getter` + --> ui-tests/unused-attributes.rs:18:16 + | +18 | #[wasm_bindgen(getter, typescript_custom_section)] + | ^^^^^^ help: if this is intentional, prefix it with an underscore: `_getter` + +error: unused variable: `readonly` + --> ui-tests/unused-attributes.rs:21:16 + | +21 | #[wasm_bindgen(readonly)] + | ^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_readonly` + +error: unused variable: `getter_with_clone` + --> ui-tests/unused-attributes.rs:24:16 + | +24 | #[wasm_bindgen(getter_with_clone)] + | ^^^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_getter_with_clone` + +error: unused variable: `typescript_type` + --> ui-tests/unused-attributes.rs:26:28 + | +26 | #[wasm_bindgen(getter, typescript_type = "Thing[]")] + | ^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_typescript_type` From a6d1d9e47b81845e7d5c02b90a4687e625921298 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Wed, 7 Sep 2022 17:45:56 +0100 Subject: [PATCH 4/6] Skip anonymous scope if there are no unused attrs --- crates/macro-support/src/parser.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/crates/macro-support/src/parser.rs b/crates/macro-support/src/parser.rs index eb639353dc4..c3004d78cca 100644 --- a/crates/macro-support/src/parser.rs +++ b/crates/macro-support/src/parser.rs @@ -1619,12 +1619,14 @@ pub fn check_unused_attrs(tokens: &mut TokenStream) { ATTRS.with(|state| { assert_eq!(state.parsed.get(), state.checks.get()); let unused_attrs = &*state.unused_attrs.borrow(); - tokens.extend(quote::quote! { - // Anonymous scope to prevent name clashes. - const _: () = { - #(let #unused_attrs: ();)* - }; - }); + if !unused_attrs.is_empty() { + tokens.extend(quote::quote! { + // Anonymous scope to prevent name clashes. + const _: () = { + #(let #unused_attrs: ();)* + }; + }); + } }) } From b8d612c14fb24c0fcaad79e145408b72e1c2b9ee Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Wed, 7 Sep 2022 17:58:06 +0100 Subject: [PATCH 5/6] Fix unused-attr check for reserved attribute names --- crates/macro-support/src/parser.rs | 2 +- crates/macro/ui-tests/unused-attributes.rs | 2 +- crates/macro/ui-tests/unused-attributes.stderr | 8 +++++++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/crates/macro-support/src/parser.rs b/crates/macro-support/src/parser.rs index c3004d78cca..d05f139f62e 100644 --- a/crates/macro-support/src/parser.rs +++ b/crates/macro-support/src/parser.rs @@ -107,7 +107,7 @@ macro_rules! methods { .map(|attr| { match attr { $(BindgenAttr::$variant(span, ..) => { - Ident::new(stringify!($name), *span) + syn::parse_quote_spanned!(*span => $name) })* } }) diff --git a/crates/macro/ui-tests/unused-attributes.rs b/crates/macro/ui-tests/unused-attributes.rs index c829ec346b4..f517dcc4b55 100644 --- a/crates/macro/ui-tests/unused-attributes.rs +++ b/crates/macro/ui-tests/unused-attributes.rs @@ -21,7 +21,7 @@ pub const FOO: &'static str = "FOO"; #[wasm_bindgen(readonly)] pub fn bar() {} -#[wasm_bindgen(getter_with_clone)] +#[wasm_bindgen(getter_with_clone, final)] impl MyStruct { #[wasm_bindgen(getter, typescript_type = "Thing[]")] pub fn hello(&self) -> String { diff --git a/crates/macro/ui-tests/unused-attributes.stderr b/crates/macro/ui-tests/unused-attributes.stderr index a1cc025d081..c4ba32f0817 100644 --- a/crates/macro/ui-tests/unused-attributes.stderr +++ b/crates/macro/ui-tests/unused-attributes.stderr @@ -25,9 +25,15 @@ error: unused variable: `readonly` error: unused variable: `getter_with_clone` --> ui-tests/unused-attributes.rs:24:16 | -24 | #[wasm_bindgen(getter_with_clone)] +24 | #[wasm_bindgen(getter_with_clone, final)] | ^^^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_getter_with_clone` +error: unused variable: `final` + --> ui-tests/unused-attributes.rs:24:35 + | +24 | #[wasm_bindgen(getter_with_clone, final)] + | ^^^^^ help: if this is intentional, prefix it with an underscore: `_final` + error: unused variable: `typescript_type` --> ui-tests/unused-attributes.rs:26:28 | From 73e82c1ab87482685deac374a20e38d5d7969611 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Wed, 7 Sep 2022 21:49:13 +0100 Subject: [PATCH 6/6] Remove defunct deprecation warning --- crates/macro-support/src/lib.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/crates/macro-support/src/lib.rs b/crates/macro-support/src/lib.rs index 5d808fde9b8..b7a35ae1bed 100644 --- a/crates/macro-support/src/lib.rs +++ b/crates/macro-support/src/lib.rs @@ -21,16 +21,6 @@ use syn::parse::{Parse, ParseStream, Result as SynResult}; mod parser; -#[cfg(feature = "strict-macro")] -const _: () = { - #[deprecated( - note = "strict-macro feature is deprecated and will be removed in a future version" - )] - const WASM_BINDGEN_STRICT_MACRO: () = {}; - - WASM_BINDGEN_STRICT_MACRO -}; - /// Takes the parsed input from a `#[wasm_bindgen]` macro and returns the generated bindings pub fn expand(attr: TokenStream, input: TokenStream) -> Result { parser::reset_attrs_used();