Skip to content

Commit

Permalink
Trigger warnings for unused wasm-bindgen attributes
Browse files Browse the repository at this point in the history
This attempts to do something similar to rustwasm#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 rustwasm#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 rustwasm#3038.
  • Loading branch information
RReverser committed Sep 6, 2022
1 parent 5c28993 commit 68b734e
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 34 deletions.
6 changes: 2 additions & 4 deletions crates/macro-support/src/lib.rs
Expand Up @@ -23,7 +23,6 @@ mod parser;

/// Takes the parsed input from a `#[wasm_bindgen]` macro and returns the generated bindings
pub fn expand(attr: TokenStream, input: TokenStream) -> Result<TokenStream, Diagnostic> {
parser::reset_attrs_used();
let item = syn::parse2::<syn::Item>(input)?;
let opts = syn::parse2(attr)?;

Expand All @@ -35,7 +34,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_and_reset_used_attrs(&mut tokens);

Ok(tokens)
}
Expand All @@ -45,13 +44,11 @@ pub fn expand_class_marker(
attr: TokenStream,
input: TokenStream,
) -> Result<TokenStream, Diagnostic> {
parser::reset_attrs_used();
let mut item = syn::parse2::<syn::ImplItemMethod>(input)?;
let opts: ClassMarker = syn::parse2(attr)?;

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
Expand All @@ -76,6 +73,7 @@ pub fn expand_class_marker(
if let Err(e) = program.try_to_tokens(tokens) {
err = Some(e);
}
parser::check_and_reset_used_attrs(tokens); // same as above
tokens.append_all(item.attrs.iter().filter(|attr| match attr.style {
syn::AttrStyle::Inner(_) => true,
_ => false,
Expand Down
72 changes: 42 additions & 30 deletions crates/macro-support/src/parser.rs
Expand Up @@ -41,6 +41,8 @@ const JS_KEYWORDS: [&str; 20] = [
struct AttributeParseState {
parsed: Cell<usize>,
checks: Cell<usize>,
#[cfg(not(feature = "strict-macro"))]
unused_attrs: std::cell::RefCell<Vec<Ident>>,
}

/// Parsed attributes from a `#[wasm_bindgen(..)]`.
Expand Down Expand Up @@ -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(())
}
}
};

Expand Down Expand Up @@ -1613,16 +1620,21 @@ fn extract_path_ident(path: &syn::Path) -> Result<Ident, Diagnostic> {
}
}

pub fn reset_attrs_used() {
pub fn check_and_reset_used_attrs(tokens: &mut TokenStream) {
ATTRS.with(|state| {
assert_eq!(state.parsed.get(), state.checks.get());
state.parsed.set(0);
state.checks.set(0);
})
}

pub fn assert_all_attrs_checked() {
ATTRS.with(|state| {
assert_eq!(state.parsed.get(), state.checks.get());
#[cfg(not(feature = "strict-macro"))]
{
let unused = std::mem::take(&mut *state.unused_attrs.borrow_mut());
tokens.extend(quote::quote! {
// Anonymous scope to prevent name clashes.
const _: () = {
#(let #unused: ();)*
};
});
}
})
}

Expand Down

0 comments on commit 68b734e

Please sign in to comment.