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

Enforce strict attribute checking #3070

Closed
wants to merge 1 commit into from
Closed

Enforce strict attribute checking #3070

wants to merge 1 commit into from

Conversation

lukaslihotzki
Copy link
Contributor

Enforce strict attribute checking as I proposed in #3038. Enabling features should not break code, because the crate enabling the feature and the crate whose code breaks may be unrelated. This change is also allowed without major update 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.

I don't know whether the precedence is stronger in the "or" or in the "and". In any case, potentially breaking code is unlikely to be written in idiomatic Rust because the idiomatic tool for specifying information with no effect are comments, not attributes. And such code is specifically discouraged from use, because the strict-macro feature is a form of discourgement.

@alexcrichton
Copy link
Contributor

Personally I'm not confident in doing this. Focusing on the impact of a change like this, I don't think there's any way to really gauge how much breakage is going to happen since it's not easy to enumerate the list of users of wasm-bindgen. I don't really want to lawyer our way into making a change like this, instead I'd prefer to focus on the practical impacts.

I completely agree that the existence of this feature is not how Cargo features are supposed to work. It is indeed not additive. There's nothing that can really be done about that without a new major version which I'm reluctant to do.

This seems like the lesser of two evils for now, personally. If/when an 0.3 or new major version bump ever happens this feature should of course be removed and enabled by default, I don't think anyone is disputing that.

RReverser added a commit to RReverser/wasm-bindgen that referenced this pull request 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 pull request 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 pull request Sep 6, 2022
Merged
@RReverser
Copy link
Member

Here's an alternative approach, sort of a compromise: #3073. @lukaslihotzki @alexcrichton would love your thoughts.

RReverser added a commit to RReverser/wasm-bindgen that referenced this pull request 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 pull request 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 pull request 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.
@lukaslihotzki
Copy link
Contributor Author

Ok, let's merge #3073 instead.

RReverser added a commit to RReverser/wasm-bindgen that referenced this pull request 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 pull request 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
Reviewers

@alexcrichton alexcrichton Awaiting requested review from alexcrichton

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lukaslihotzki @alexcrichton @RReverser