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

Allow the unused_macro_rules lint for now #97032

Merged
merged 1 commit into from
May 15, 2022
Merged

Allow the unused_macro_rules lint for now #97032

merged 1 commit into from
May 15, 2022

Conversation

est31
Copy link
Member

@est31 est31 commented May 14, 2022 •
edited
Loading

It was newly added by #96150 with warn by default, which is great as it gave exposure to the community, and their feedback gave me ideas for improvements.

Allowing the lint is good for two reasons:

  • It makes the transition easier as e.g. allow directives won't fire the unknown lint warning once it is turned to warn by default in the future. The commit that allowed the lint in fuchsia had to allow unknown lints for example.
    This is especially important compared to other lints in the unused group,
    because the _ prefix trick doesn't exist for macro rules, allowing is the
    only option (either of unused_macro_rules, or of the entire unused group,
    but that is not as informative to readers). Allowing the lint also makes it
    possible to work on possible heuristics for disabling the macro in specific
    cases.
  • It gives time for implementing heuristics for when to suppress the lint, e.g.
    when compile_error! is invoked by that arm (so it's only there to yield an error).

See: #96150 (comment)

I would also like this to be backported to the 1.62 beta branch (cc #97016).

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 14, 2022
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2022
@petrochenkov
Copy link
Contributor

Code example in the lint description needs to be updated, CI fails due to it.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2022
@est31 est31 force-pushed the unused_macro_rules branch from e6f86f3 to 52b43f0 Compare May 14, 2022 09:48
@est31
Copy link
Member Author

est31 commented May 14, 2022 •
edited
Loading

@petrochenkov sorry about that, I'm getting symbol errors when trying to locally test rustc, so I can't do x.py test locally (edit: see #97041).

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@est31
Allow the unused_macro_rules lint for now
015e2ae 
This makes the transition easier as e.g. allow directives
won't fire the unknown lint warning once it is turned to
warn by default in the future. This is especially
important compared to other lints in the unused group
because the _ prefix trick doesn't exist for macro rules,
so allowing is the only option (either of unused_macro_rules,
or of the entire unused group, but that is not as informative
to readers). Allowing the lint also makes it possible to work
on possible heuristics for disabling the macro in specific
cases.
@est31 est31 force-pushed the unused_macro_rules branch from 52b43f0 to 015e2ae Compare May 14, 2022 10:31
@petrochenkov
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 14, 2022

📌 Commit 015e2ae has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 14, 2022
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 14, 2022
@GuillaumeGomez
Rollup merge of rust-lang#97032 - est31:unused_macro_rules, r=petroch…
e1fc39c 
…enkov

Allow the unused_macro_rules lint for now

It was newly added by rust-lang#96150 with warn by default, which is great as it gave exposure to the community, and their feedback gave me ideas for improvements.

Allowing the lint is good for two reasons:

* It makes the transition easier as e.g. allow directives won't fire the unknown lint warning once it is turned to warn by default in the future. The [commit that allowed the lint in fuchsia](https://fuchsia.googlesource.com/fuchsia/+/9d8f96517c3963de2f0e25598fd36061914524cd%5E%21/) had to allow unknown lints for example.
This is especially important compared to other lints in the unused group,
because the _ prefix trick doesn't exist for macro rules, allowing is the
only option (either of unused_macro_rules, or of the entire unused group,
but that is not as informative to readers). Allowing the lint also makes it
possible to work on possible heuristics for disabling the macro in specific
cases.
* It gives time for implementing heuristics for when to suppress the lint, e.g.
when `compile_error!` is invoked by that arm (so it's only there to yield an error).

See: rust-lang#96150 (comment)

I would also like this to be backported to the 1.62 beta branch (cc rust-lang#97016).
@GuillaumeGomez GuillaumeGomez mentioned this pull request May 14, 2022
Closed
@Mark-Simulacrum Mark-Simulacrum added the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 15, 2022
@Mark-Simulacrum
Copy link
Member

beta-nominating for 1.62 backport (necessary if it lands after #97016, which seems likely).

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 15, 2022
@Dylan-DPC
Rollup merge of rust-lang#97032 - est31:unused_macro_rules, r=petroch…
abb952c 
…enkov

Allow the unused_macro_rules lint for now

It was newly added by rust-lang#96150 with warn by default, which is great as it gave exposure to the community, and their feedback gave me ideas for improvements.

Allowing the lint is good for two reasons:

* It makes the transition easier as e.g. allow directives won't fire the unknown lint warning once it is turned to warn by default in the future. The [commit that allowed the lint in fuchsia](https://fuchsia.googlesource.com/fuchsia/+/9d8f96517c3963de2f0e25598fd36061914524cd%5E%21/) had to allow unknown lints for example.
This is especially important compared to other lints in the unused group,
because the _ prefix trick doesn't exist for macro rules, allowing is the
only option (either of unused_macro_rules, or of the entire unused group,
but that is not as informative to readers). Allowing the lint also makes it
possible to work on possible heuristics for disabling the macro in specific
cases.
* It gives time for implementing heuristics for when to suppress the lint, e.g.
when `compile_error!` is invoked by that arm (so it's only there to yield an error).

See: rust-lang#96150 (comment)

I would also like this to be backported to the 1.62 beta branch (cc rust-lang#97016).
@Dylan-DPC Dylan-DPC mentioned this pull request May 15, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request May 15, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 15, 2022
@bors
Auto merge of rust-lang#97055 - matthiaskrgr:rollup-1nqwfzx, r=matthi…
e1ec326 
…askrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#96958 (Improve settings menu display and remove theme menu)
 - rust-lang#97032 (Allow the unused_macro_rules lint for now)
 - rust-lang#97041 (Fix `download-ci-llvm` NixOS patching for `.so`s.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 673d451 into rust-lang:master May 15, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 15, 2022
github-actions bot pushed a commit to gnoliyil/fuchsia that referenced this pull request May 17, 2022
@ComputerDruid
Revert "[rust] allow unused-macro-rules to unblock toolchain roll"
d0a5d16 
This reverts commit 9d8f965.

Reason for revert: lint is allow-by-default after rust-lang/rust#97032

Original change's description:
> [rust] allow unused-macro-rules to unblock toolchain roll
>
> Bug: 100318
> Change-Id: Ided9428282c66e63cda934a5f456223750703179
> Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/679506
> Reviewed-by: Joseph Ryan <[email protected]>
> Fuchsia-Auto-Submit: Dan Johnson <[email protected]>
> Reviewed-by: Tyler Mandry <[email protected]>
> Commit-Queue: Auto-Submit <[email protected]>

Bug: 100318
Change-Id: I2ebf0b0e9c9e28d8a2588fc92dfe7428c77ca4cf
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/679988
Reviewed-by: RubberStamper 🤖 <[email protected]>
Commit-Queue: Dan Johnson <[email protected]>
Reviewed-by: Joseph Ryan <[email protected]>
@apiraino
Copy link
Contributor

Beta backport approved as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label May 19, 2022
@ehuss ehuss modified the milestones: 1.63.0, 1.62.0 Jun 1, 2022
ehuss pushed a commit to ehuss/rust that referenced this pull request Jun 1, 2022
@matthiaskrgr @ehuss
Rollup merge of rust-lang#97032 - est31:unused_macro_rules, r=petroch…
0723488 
…enkov

Allow the unused_macro_rules lint for now

It was newly added by rust-lang#96150 with warn by default, which is great as it gave exposure to the community, and their feedback gave me ideas for improvements.

Allowing the lint is good for two reasons:

* It makes the transition easier as e.g. allow directives won't fire the unknown lint warning once it is turned to warn by default in the future. The [commit that allowed the lint in fuchsia](https://fuchsia.googlesource.com/fuchsia/+/9d8f96517c3963de2f0e25598fd36061914524cd%5E%21/) had to allow unknown lints for example.
This is especially important compared to other lints in the unused group,
because the _ prefix trick doesn't exist for macro rules, allowing is the
only option (either of unused_macro_rules, or of the entire unused group,
but that is not as informative to readers). Allowing the lint also makes it
possible to work on possible heuristics for disabling the macro in specific
cases.
* It gives time for implementing heuristics for when to suppress the lint, e.g.
when `compile_error!` is invoked by that arm (so it's only there to yield an error).

See: rust-lang#96150 (comment)

I would also like this to be backported to the 1.62 beta branch (cc rust-lang#97016).
@ehuss ehuss mentioned this pull request Jun 1, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 2, 2022
@bors
Auto merge of rust-lang#97631 - ehuss:update-beta-cargo, r=ehuss
a5cf77c 
[beta] Beta backports

* Allow the unused_macro_rules lint for now rust-lang#97032
* Fix some typos in arg checking algorithm rust-lang#97303
* rustc: Fix ICE in native library error reporting rust-lang#97328
* Cargo:
    * Fix `cargo publish -p spec` rust-lang/cargo#10707
This was referenced Jun 7, 2022
Merged
Closed
naturallymitchell pushed a commit to naturallymitchell/fuchsia-storage that referenced this pull request Jun 10, 2022
@ComputerDruid
Revert "[rust] allow unused-macro-rules to unblock toolchain roll"
765852b 
This reverts commit 5909ed0.

Reason for revert: lint is allow-by-default after rust-lang/rust#97032

Original change's description:
> [rust] allow unused-macro-rules to unblock toolchain roll
>
> Bug: 100318
> Change-Id: Ided9428282c66e63cda934a5f456223750703179
> Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/679506
> Reviewed-by: Joseph Ryan <[email protected]>
> Fuchsia-Auto-Submit: Dan Johnson <[email protected]>
> Reviewed-by: Tyler Mandry <[email protected]>
> Commit-Queue: Auto-Submit <[email protected]>

Bug: 100318
Change-Id: I2ebf0b0e9c9e28d8a2588fc92dfe7428c77ca4cf
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/679988
Reviewed-by: RubberStamper 🤖 <[email protected]>
Commit-Queue: Dan Johnson <[email protected]>
Reviewed-by: Joseph Ryan <[email protected]>
@ehuss ehuss removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@petrochenkov petrochenkov

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.
9 participants
@est31 @rust-highfive @petrochenkov @rust-log-analyzer @bors @Mark-Simulacrum @apiraino @ehuss @rustbot