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.

Sign up for GitHub

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

Jump to bottom

wasm_bindgen attribute shouldn't accept typescript_type without using it #2874

Closed
TedDriggs opened this issue Apr 27, 2022 · 1 comment · Fixed by #3073
Closed

wasm_bindgen attribute shouldn't accept typescript_type without using it #2874

TedDriggs opened this issue Apr 27, 2022 · 1 comment · Fixed by #3073
Labels
bug

Comments

@TedDriggs
Copy link

Describe the Bug

The #[wasm_bindgen] attribute accepts (some?) fields without error, but doesn't use them.

Steps to Reproduce

#[wasm_bindgen]
pub struct MyStruct {
    hello: String,
}

#[wasm_bindgen]
impl MyStruct {
    #[wasm_bindgen(getter, typescript_type = "Thing[]")]
    pub things(&self) -> Vec<JsValue> {
        todo!()
    }
}

Expected Behavior

Either the return type of the getter would be Thing[] or the macro would have produced an error on the attempt to use typescript_typethere.

Actual Behavior

The typescript_type field will be ignored, and the getter will have a return type of any[]
@TedDriggs TedDriggs added the bug label Apr 27, 2022
TedDriggs pushed a commit to TedDriggs/wasm-bindgen that referenced this issue Apr 28, 2022
Use conventional meta-item syntax for attributes
77dd217 
Using the conventional form for attributes, which is compatible with
rustfmt and syn's meta-item parsing. This change is breaking and
substantial, but the necessary edits are largely mechanical.

With meta-items, it becomes possible to offload attribute parsing and
validation to `darling`. This offload fixes rustwasm#2874, as `darling` will
automatically generate errors for unexpected meta-item names. This also
makes it easier for people who are new to the codebase to understand
how to add new options.
@TedDriggs TedDriggs mentioned this issue Apr 28, 2022
Closed
TedDriggs pushed a commit to TedDriggs/wasm-bindgen that referenced this issue Apr 28, 2022
Use conventional meta-item syntax for attributes
3078ea5 
Using the conventional form for attributes, which is compatible with
rustfmt and syn's meta-item parsing. This change is breaking and
substantial, but the necessary edits are largely mechanical.

With meta-items, it becomes possible to offload attribute parsing and
validation to `darling`. This offload fixes rustwasm#2874, as `darling` will
automatically generate errors for unexpected meta-item names. This also
makes it easier for people who are new to the codebase to understand
how to add new options.
TedDriggs pushed a commit to TedDriggs/wasm-bindgen that referenced this issue Apr 28, 2022
Use conventional meta-item syntax for attributes
45030a1 
Using the conventional form for attributes, which is compatible with
rustfmt and syn's meta-item parsing. This change is breaking and
substantial, but the necessary edits are largely mechanical.

With meta-items, it becomes possible to offload attribute parsing and
validation to `darling`. This offload fixes rustwasm#2874, as `darling` will
automatically generate errors for unexpected meta-item names. This also
makes it easier for people who are new to the codebase to understand
how to add new options.
TedDriggs pushed a commit to TedDriggs/wasm-bindgen that referenced this issue Apr 29, 2022
Use conventional meta-item syntax for attributes
9b42f94 
Using the conventional form for attributes, which is compatible with
rustfmt and syn's meta-item parsing. This change is breaking and
substantial, but the necessary edits are largely mechanical.

With meta-items, it becomes possible to offload attribute parsing and
validation to `darling`. This offload fixes rustwasm#2874, as `darling` will
automatically generate errors for unexpected meta-item names. This also
makes it easier for people who are new to the codebase to understand
how to add new options.

This commit does not build: It changes the macro parser, but does not update the usage of the macro.
TedDriggs pushed a commit to TedDriggs/wasm-bindgen that referenced this issue Apr 29, 2022
Refactor #[wasm_bindgen] attribute parsing
8857f76 
This commit moves to having a struct per `syn::Item` type that the
macro operates on, then uses `darling` to handle validation of the
attribute.

This implicitly fixes rustwasm#2874, since `darling` automatically generates
errors for unexpected properties. It also makes the macro code easier
to follow by hiding the complexity associated with parsing out of sight
in a well-tested crate. This also precludes a class of bug where someone
might have attempted to read a property that was not accepted for that
syntax item, since each syntax item now has its own options receiver.

`darling` operates on `syn::Meta`, which only allows a literal to the
right side of `=` signs in attributes. This is a problem for wasm_bindgen,
which does not use them.

To maintain API compatibility while using `darling`, this commit adds
`macro_support::meta::Meta<T>`, a replica of `syn::Meta` where `Lit` is
replaced by a type parameter. This may be useful enough beyond `wasm_bindgen`
for it to move upstream to `darling`, which would mean this change would
reduce the overall amount of code in `macro-support`.
@TedDriggs TedDriggs mentioned this issue Apr 29, 2022
Closed
@lukaslihotzki
Copy link
Contributor

wasm-bindgen already supports rejecting the example case by enabling the strict-macro feature:

error: unused #[wasm_bindgen] attribute
  --> example/src/lib.rs:15:28
   |
15 |     #[wasm_bindgen(getter, typescript_type = "Thing[]")]
   |                            ^^^^^^^^^^^^^^^

error: could not compile `example` due to previous error

@lukaslihotzki lukaslihotzki mentioned this issue Aug 21, 2022
Closed
RReverser added a commit to RReverser/wasm-bindgen that referenced this issue Sep 6, 2022
@RReverser
Trigger warnings for unused wasm-bindgen attributes
387eafe 
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
@RReverser
Trigger warnings for unused wasm-bindgen attributes
70a4a9a 
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 RReverser mentioned this issue Sep 6, 2022
Merged
RReverser added a commit to RReverser/wasm-bindgen that referenced this issue Sep 6, 2022
@RReverser
Trigger warnings for unused wasm-bindgen attributes
68b734e 
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
@RReverser
Trigger warnings for unused wasm-bindgen attributes
0b6c238 
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
@RReverser
Trigger warnings for unused wasm-bindgen attributes
b4af086 
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
@RReverser
Trigger warnings for unused wasm-bindgen attributes
84d6c85 
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
@RReverser @lukaslihotzki
Trigger warnings for unused wasm-bindgen attributes (#3073)
595b04b 
* 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
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
2 participants
@TedDriggs @lukaslihotzki