From 595b04b24aeadd467bbea01c75193d63c3ab6db5 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Thu, 8 Sep 2022 15:48:26 +0100 Subject: [PATCH] Trigger warnings for unused wasm-bindgen attributes (#3073) * 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. * Guide users who used the suggested (invalid) fix (#1) Co-authored-by: Ingvar Stepanyan * Deprecate strict-macro feature; update tests * Skip anonymous scope if there are no unused attrs * Fix unused-attr check for reserved attribute names * Remove defunct deprecation warning Co-authored-by: Lukas Lihotzki --- crates/macro-support/src/lib.rs | 4 +- crates/macro-support/src/parser.rs | 92 ++++++++++--------- crates/macro/Cargo.toml | 2 +- crates/macro/ui-tests/unused-attributes.rs | 25 ++++- .../macro/ui-tests/unused-attributes.stderr | 48 ++++++++-- 5 files changed, 112 insertions(+), 59 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..d05f139f62e 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,6 +41,7 @@ const JS_KEYWORDS: [&str; 20] = [ struct AttributeParseState { parsed: Cell, checks: Cell, + unused_attrs: RefCell>, } /// Parsed attributes from a `#[wasm_bindgen(..)]`. @@ -94,35 +95,24 @@ macro_rules! methods { ($(($name:ident, $variant:ident($($contents:tt)*)),)*) => { $(methods!(@method $name, $variant($($contents)*));)* - #[cfg(feature = "strict-macro")] - 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 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")); - } - 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(()) + 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, ..) => { + syn::parse_quote_spanned!(*span => $name) + })* + } + }) + ); + }); } }; @@ -218,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(); } } @@ -353,7 +343,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" + })); } } @@ -411,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; } @@ -436,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, @@ -662,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) } @@ -695,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, @@ -734,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, @@ -770,7 +764,7 @@ impl ConvertToAst for syn::ItemFn { None, false, )?; - attrs.check_used()?; + attrs.check_used(); Ok(ret.0) } } @@ -1049,7 +1043,7 @@ impl<'a> MacroParse for &'a mut syn::ItemImpl { } } Diagnostic::from_vec(errors)?; - opts.check_used()?; + opts.check_used(); Ok(()) } } @@ -1160,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(()) } } @@ -1229,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); } _ => {} @@ -1239,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(); @@ -1353,7 +1347,7 @@ impl MacroParse for syn::ItemConst { } } - opts.check_used()?; + opts.check_used(); Ok(()) } @@ -1401,7 +1395,7 @@ impl MacroParse for syn::ItemForeignMod { } } Diagnostic::from_vec(errors)?; - opts.check_used()?; + opts.check_used(); Ok(()) } } @@ -1617,12 +1611,22 @@ pub fn reset_attrs_used() { ATTRS.with(|state| { state.parsed.set(0); state.checks.set(0); + 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()); + let unused_attrs = &*state.unused_attrs.borrow(); + if !unused_attrs.is_empty() { + 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..f517dcc4b55 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, final)] +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..c4ba32f0817 100644 --- a/crates/macro/ui-tests/unused-attributes.stderr +++ b/crates/macro/ui-tests/unused-attributes.stderr @@ -1,11 +1,41 @@ -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, 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 + | +26 | #[wasm_bindgen(getter, typescript_type = "Thing[]")] + | ^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_typescript_type`