Allow #[deny] inside #[forbid] as a no-op with a warning
#121560
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.
rustbot
added
S-waiting-on-review
| /// 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.
better lint name ideas welcome
Nilstrieb
added
S-waiting-on-team
|
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 |
rustbot
added
the
S-waiting-on-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. |
joshtriplett
added
T-lang
|
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. |
rfcbot
added
proposed-final-comment-period
|
@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. |
rustbot
removed
the
I-lang-nominated
|
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. |
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