-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Allow #[deny]
inside #[forbid]
as a no-op
#121560
base: master
Are you sure you want to change the base?
Conversation
Forbid cannot be overriden. When someome tries to do this anyways, it results in a hard error. That makes sense. Except it doesn't, because macros. Macros may reasonably use `#[deny]` (or `#[warn]` for an allow-by-default lint) in their expansion to assert that their expanded code follows the lint. This is doesn't work when the output gets expanded into a `forbid()` context. This is pretty silly, since both the macros and the code agree on the lint! By making it a warning instead, we remove the problem with the macro, which is now nothing as warnings are suppressed in macro expanded code, while still telling users that something is up.
/// expanded output, which might get expanded into a context that is `forbid()` already, | ||
/// this is not a hard error. | ||
/// As lints in macros are suppressed, so this lint will not show up in these cases. | ||
pub UNOVERRIDABLE_LINT_LEVEL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better lint name ideas welcome
The job Click to see the possible cause of the failure (guessed by this bot)
|
My understanding is that this allows this pattern: #![forbid(lint)]
#[deny(lint)]
fn something() {
// code
} Can this come with a test for this case? #![forbid(lint)]
#[deny(lint)]
fn something() {
#[allow(lint)]
{
// linted code
}
} Apologies if this test already exists and I just didn't notice it. |
@rustbot author |
When do people really want If people hit this, maybe the resolution instead would be to say "well use |
It seems completely fine to The thing that shouldn't be allowed is using |
@workingjubilee Agreed that we should have a test for allow-inside-deny-inside-forbid, as well as a test for warn-inside-deny-inside-forbid, both of which should be an error. (The test should check the case where that happens via a macro, as well.) |
We discussed this at the end of the lang triage call today and did not reach a resolution before running out of time. We'll leave this nominated so as to discuss in the meeting on a future week. |
We discussed this again in today's @rust-lang/lang meeting. Concrete proposal to get consensus on:
@rfcbot merge |
Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
@Nilstrieb Could you please remove the warning, and silently allow |
@rustbot labels -I-lang-nominated As mentioned above, this was discussed today and the mood in the meeting was positive on doing what Josh proposed. Since it was discussed and there's now a propose FCP here, let's unnominate. |
Thanks! This will help me significantly for situations where a |
☔ The latest upstream changes (presumably #122483) made this pull request unmergeable. Please resolve the merge conflicts. |
@rfcbot reviewed Note that the PR still needs to be updated to reflect the lang team consensus (no warning on deny-in-forbid, but it's a no-op). Personally I'm in favor of going farther and allowing |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
#[deny]
inside #[forbid]
as a no-op with a warning#[deny]
inside #[forbid]
as a no-op
Can you please send a PR to the reference to update the lint docs for the changes from this PR? |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
Forbid cannot be overriden. When someome tries to do this anyways, it results in a hard error. That makes sense.
Except it doesn't, because macros. Macros may reasonably use
#[deny]
(or#[warn]
for an allow-by-default lint) in their expansion to assert that their expanded code follows the lint. This is doesn't work when the output gets expanded into aforbid()
context. This is pretty silly, since both the macros and the code agree on the lint!By making it a warning instead, we remove the problem with the macro, which is now nothing as warnings are suppressed in macro expanded code, while still telling users that something is up.
fixes #121483