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 <me@rreverser.com>

* 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 <lukas@lihotzki.de>

crates/macro-support/src/lib.rs

@@ -35,7 +35,7 @@ pub fn expand(attr: TokenStream, input: TokenStream) -> Result<TokenStream, Diag
     // If we successfully got here then we should have used up all attributes
     // and considered all of them to see if they were used. If one was forgotten
     // that's a bug on our end, so sanity check here.
-    parser::assert_all_attrs_checked();
+    parser::check_unused_attrs(&mut tokens);
 
     Ok(tokens)
 }
@@ -51,7 +51,6 @@ pub fn expand_class_marker(
 
     let mut program = backend::ast::Program::default();
     item.macro_parse(&mut program, (&opts.class, &opts.js_class))?;
-    parser::assert_all_attrs_checked(); // same as above
 
     // This is where things are slightly different, we are being expanded in the
     // context of an impl so we can't inject arbitrary item-like tokens into the
@@ -76,6 +75,7 @@ pub fn expand_class_marker(
         if let Err(e) = program.try_to_tokens(tokens) {
             err = Some(e);
         }
+        parser::check_unused_attrs(tokens); // same as above
         tokens.append_all(item.attrs.iter().filter(|attr| match attr.style {
             syn::AttrStyle::Inner(_) => true,
             _ => false,

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<usize>,
     checks: Cell<usize>,
+    unused_attrs: RefCell<Vec<Ident>>,
 }
 
 /// 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<BindgenAttrs> 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<BindgenAttrs> 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<String> = 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<ast::ImportModule>)> 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<BindgenAttrs> 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<ast::ImportModule>)> 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<BindgenAttrs> for syn::ItemFn {
             None,
             false,
         )?;
-        attrs.check_used()?;
+        attrs.check_used();
         Ok(ret.0)
     }
 }
@@ -1049,7 +1043,7 @@ impl<'a> MacroParse<BindgenAttrs> 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<BindgenAttrs> for syn::ItemConst {
             }
         }
 
-        opts.check_used()?;
+        opts.check_used();
 
         Ok(())
     }
@@ -1401,7 +1395,7 @@ impl MacroParse<BindgenAttrs> 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: ();)*
+                };
+            });
+        }
     })
 }
 

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" }

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()
     }
 }
 

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`