Trigger warnings for unused wasm-bindgen attributes

[RReverser]

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 (the "prefix it with an underscore" part is obviously wrong) - but, until Rust has a built-in compile_warning!, nothing is and this provides a better experience than the current status quo as it can help users find problematic attributes without actually breaking their builds.

Fixes #3038.
Fixes #2874.

[lukaslihotzki]

I like this approach. The case when the user does was the warning suggests should be handled: The proc macro should detect attributes starting with underscores, and produce a helpful compile_error! there, like "Attributes starting with underscores are not supported. If attributes are unused, it's safe to remove them entirely.". This will guide users to the correct thing to do in two steps.

[Liamolucko]

This looks good to me. It might be a good idea to add some UI tests for this, seeing as the whole point is the error messages.

[RReverser]

The case when the user does was the warning suggests should be handled: The proc macro should detect attributes starting with underscores, and produce a helpful compile_error! there, like "Attributes starting with underscores are not supported. If attributes are unused, it's safe to remove them entirely.". This will guide users to the correct thing to do in two steps.

Those already error out as unknown attributes:

error: unknown attribute
  --> tests\headless\snippets.rs:67:28
   |
67 |     #[wasm_bindgen(getter, _typescript_type = "foo")]
   |                            ^^^^^^^^^^^^^^^^

error: could not compile `wasm-bindgen` due to previous error

This looks good to me. It might be a good idea to add some UI tests for this, seeing as the whole point is the error messages.

Yeah, that makes sense.

[lukaslihotzki]

Those already error out as unknown attributes

Yes, but the current error on unknown attributes is confusing for users who tried to fix the warning with the suggested fix. They just get another error without hint what to do now. The sentence "If attributes were unused, it's safe to remove them entirely." would make it clear how to fix the warning correctly without looking it up elsewhere or thinking about not to accidentally break the code. I'll implement this as a PR to your branch.

[RReverser]

They just get another error without hint what to do now. The sentence "If attributes were unused, it's safe to remove them entirely." would make it clear how to fix the warning correctly without looking it up elsewhere or thinking about not to accidentally break the code.

Yeah I guess it might be still confusing, although I'd hope everyone would ignore the underscore suggestion in the first place since, unlike for variables, it's not very useful to explicitly mark attributes as unused anyway (even if it worked).

[RReverser]

This looks good to me. It might be a good idea to add some UI tests for this, seeing as the whole point is the error messages.

Hm, this is a bit annoying to do because by default all ui tests are executed with strict-mode feature enabled. I'd have to change CI to run it both ways I guess, with both feature enabled and disabled :/ Or I could remove the default strict-mode and test only the warnings mode from this PR. What do you think?

[alexcrichton]

This seems like a clever trick to me, nice find!

One possibility is that we can manipulate the spans for tokens that we generate and I think rustc does have a lint for "unused attribute" so the warning can probably be improved here to even generate "unused attribute". For example this source:

#[macro_use()]
extern crate std;

generates:

warning: unused attribute
 --> src/lib.rs:1:1
  |
1 | #[macro_use()]
  | ^^^^^^^^^^^^^^ help: remove this attribute
  |
  = note: `#[warn(unused_attributes)]` on by default
  = note: attribute `macro_use` with an empty list has no effect

which is almost what we want except for the final note:

There's a few other ways to get rustc to say "unused attribute" but I think all of them have some sort of note: which isn't what we want. Overall though I don't mind either way, this seems like the least bad option to me and is an improvement over what is there today.

[RReverser]

@alexcrichton Interesting, probably worth looking into. Another option I considered & tried is generating

pub const _: () = {
    struct WasmBindgenAttrs {
        bar: (),
        baz: (),
    }

    let _wasm_bindgen_attrs: WasmBindgenAttrs ;
};

instead (with pub due to rust-lang/rust#101532 discovered in the process), to get

warning: fields `bar` and `baz` are never read
 --> <source>:3:9
  |
2 |     struct Foo {
  |            --- fields in this struct
3 |         bar: (),
  |         ^^^^^^^
4 |         baz: (),
  |         ^^^^^^^
  |
  = note: `#[warn(dead_code)]` on by default

but with spans pointing to attributes.

That would avoid the wrong "prefix with underscore" hint, resulting in a cleaner message, but, unfortunately, this warning doesn't trigger when generated by the macro for some reason, while unused variables do.

[alexcrichton]

That also seems reasonable to me yeah (if the warning actually triggered). In any case no need to golf this too much, I'm personally a fan of the idea and while I see the possibility for confusion we're also doing the best we can.

[RReverser]

Sounds good. What do you think about the ui-tests problem from #3073 (comment)? Should I disable strict-mode for tests by default or run both on CI?

[alexcrichton]

Removing strict-mode entirely seems reasonable to me since there's not any reason for it to stick around any more.

[RReverser]

Removing strict-mode entirely seems reasonable to me since there's not any reason for it to stick around any more.

Hmm, that would break existing code that uses it in their Cargo.toml though? Or should I keep the feature but make it defunct?

[alexcrichton]

Indeed just leave the feature there and have it not do anything is what I mean.

[RReverser]

but, unfortunately, this warning doesn't trigger when generated by the macro for some reason, while unused variables do

I played with this a bit more anyway, but yeah, can't get it to work easily so I'll leave as-is for now and merge RReverser#1 instead.

[RReverser]

Ok I removed the strict-macro feature + added a deprecation message for those who use it and updated the UI tests.

[RReverser]

One more thing... Just realised that it fails on attributes like final which are reserved words.

[RReverser]

^ Fixed.