Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

strict-macro checking should be enabled by default #3038

Closed
lukaslihotzki opened this issue Aug 20, 2022 · 4 comments · Fixed by #3073
Closed

strict-macro checking should be enabled by default #3038

lukaslihotzki opened this issue Aug 20, 2022 · 4 comments · Fixed by #3073
Labels

Comments

@lukaslihotzki
Copy link
Contributor

Describe the Bug

Unapplicable attributes are accepted inside wasm_bindgen impl and extern blocks.

Steps to Reproduce

#[wasm_bindgen]
pub struct Foo;

#[wasm_bindgen]
impl Foo {
    #[wasm_bindgen(inline_js = "hello world", raw_module = "something.js")]
    pub fn foo() {
    }
}

#[wasm_bindgen]
extern "C" {
    #[wasm_bindgen(inline_js = "hello world", raw_module = "something.js")]
    pub fn bar();
}

Expected Behavior

Compile errors in Foo::foo and bar because the attributes inline_js and raw_module are not applicable for these items.

Actual Behavior

These attributes are silently ignored.

@Liamolucko
Copy link
Collaborator

wasm-bindgen already supports checking for this when the strict-macro feature is enabled:

error: unused #[wasm_bindgen] attribute
 --> src/lib.rs:8:20
  |
8 |     #[wasm_bindgen(inline_js = "hello world", raw_module = "something.js")]
  |                    ^^^^^^^^^

error: unused #[wasm_bindgen] attribute
 --> src/lib.rs:8:47
  |
8 |     #[wasm_bindgen(inline_js = "hello world", raw_module = "something.js")]
  |                                               ^^^^^^^^^^

error: unused #[wasm_bindgen] attribute
  --> src/lib.rs:14:20
   |
14 |     #[wasm_bindgen(inline_js = "hello world", raw_module = "something.js")]
   |                    ^^^^^^^^^

error: unused #[wasm_bindgen] attribute
  --> src/lib.rs:14:47
   |
14 |     #[wasm_bindgen(inline_js = "hello world", raw_module = "something.js")]
   |                                               ^^^^^^^^^^

error: could not compile `wasm-playground` due to 4 previous errors

So, perhaps that should be enabled by default? It would be a breaking change to do that, but it could also be argued that any code that does that is mistaken.

To do it non-breakingly, it could be a warning by default instead.

@lukaslihotzki
Copy link
Contributor Author

Can proc_macros warn in stable Rust?

@lukaslihotzki
Copy link
Contributor Author

Anyway, I think the strict-macro feature is poor design, because Cargo features should be additive, and this feature is not. Its sole purpose is to put additional restrictions on user code. This obviously breaks when some crate A uses unapplicable attributes, and crate B enables the strict-macro feature. Also, this feature is more useful for developers who do not have a lot of experience with wasm-bindgen, but these developers are unlikely to know this feature.

Therefore, I think strict checking should be always on, and the strict-macro feature should be removed (or made an no-op for compatibility). This is also allowed by the SemVer Compatibility chapter of the Cargo book:

Some changes are marked as "minor", even though they carry the potential risk of breaking a build. This is for situations where the potential is extremely low, and the potentially breaking code is unlikely to be written in idiomatic Rust, or is specifically discouraged from use.

@lukaslihotzki lukaslihotzki changed the title Unapplicable attributes accepted inside wasm_bindgen blocks strict-macro checking should be enabled by default Aug 21, 2022
@lukaslihotzki
Copy link
Contributor Author

#2874 is another example of the confusion caused by the current approach.

RReverser added a commit to RReverser/wasm-bindgen that referenced this issue Sep 6, 2022
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 generate unused variables with spans pointing to unused attributes, so that users get a usable 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 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.
RReverser added a commit to RReverser/wasm-bindgen that referenced this issue Sep 6, 2022
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.
RReverser added a commit to RReverser/wasm-bindgen that referenced this issue Sep 6, 2022
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.
RReverser added a commit to RReverser/wasm-bindgen that referenced this issue Sep 6, 2022
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.
RReverser added a commit to RReverser/wasm-bindgen that referenced this issue Sep 6, 2022
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.
RReverser added a commit to RReverser/wasm-bindgen that referenced this issue Sep 7, 2022
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.
alexcrichton pushed a commit that referenced this issue Sep 8, 2022
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants