Allow #[deny] inside #[forbid] as a no-op by Noratrieb · Pull Request #121560 · rust-lang/rust (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Noratrieb

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!

fixes #121483

@rustbot

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Feb 24, 2024

Noratrieb

@Noratrieb Noratrieb added S-waiting-on-team

Status: Awaiting decision from the relevant subteam (see the T- label).

I-lang-nominated

Nominated for discussion during a lang team meeting.

needs-fcp

This change is insta-stable, so needs a completed FCP to proceed.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Feb 24, 2024

@rust-log-analyzer

This comment has been minimized.

@workingjubilee

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.

@TaKO8Ki

@rustbot rustbot added the S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

label

Feb 26, 2024

@scottmcm

When do people really want forbid? The only thing that comes to mind for me is forbid(unsafe_code), where I could consider it a plus that a macro can't allow(unsafe_code) to use it anyway.

If people hit this, maybe the resolution instead would be to say "well use deny then"?

@joshtriplett

It seems completely fine to #[deny] inside a #[forbid], and I don't think we need a warning for that. (The lint should remain in the forbidden state, and should still not subsequently be allow-able.)

The thing that shouldn't be allowed is using #[warn] or #[allow] inside #[forbid]; that's the exact thing that #[forbid] forbids, so if you want to allow that, don't use #[forbid].

@joshtriplett

@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.)

@traviscross

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 joshtriplett added T-lang

Relevant to the language team, which will review and decide on the PR/issue.

and removed T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Mar 6, 2024

@joshtriplett

We discussed this again in today's @rust-lang/lang meeting. Concrete proposal to get consensus on:

@rfcbot merge

@rfcbot

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.
See this document for info about what commands tagged team members can give me.

@joshtriplett

@Nilstrieb Could you please remove the warning, and silently allow deny within forbid (but ensure that the state remains forbid so that allow-inside-deny-inside-forbid is still a hard error)?

@traviscross

@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.

@workingjubilee

Thanks! This will help me significantly for situations where a forbid(lint) holding is critical... yes, usually forbid(unsafe_code)... while permitting macro code to generate its own deny(lint)s so that it retains internal consistency in its expansion, so that if a macro is used in both a forbid(lint) context and outside of that context, it doesn't have to rethink how it expands certain pieces of code.

@bors

@tmandry

@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 #[warn] and even #[allow] as no-ops with a warning (and that warning would be suppressed inside macro expansions). But let's get this merged first.

@rfcbot rfcbot added the final-comment-period

In the final comment period and will be merged soon unless new substantive objections are raised.

label

Jun 28, 2024

jieyouxu

Member

@jieyouxu jieyouxu left a comment • Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this LGTM with some additional test coverage. Though may need to fix the lint docs failure, not sure what's up with that, I haven't looked closely at it.

@jieyouxu jieyouxu added A-lints

Area: Lints (warnings about flaws in source code) such as unused_mut.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

and removed S-waiting-on-team

Status: Awaiting decision from the relevant subteam (see the T- label).

T-bootstrap

Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

labels

Oct 18, 2024

@rustbot rustbot added the T-bootstrap

Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

label

Oct 18, 2024

jieyouxu

@rust-log-analyzer

This comment has been minimized.

@Noratrieb

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] 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!

Therefore, we allow #[deny(..)]ing a lint that's already forbidden, keeping the level at forbid.

@jieyouxu @Noratrieb

@Noratrieb

the lint-docs failure was because the lint docs were relying on deny inside forbid being wrong, i changed that one to warn.
i also opened a pr to the reference: rust-lang/reference#1655

@Noratrieb

@rustbot rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Oct 20, 2024

jieyouxu

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jieyouxu

@bors

📌 Commit 1af9d11 has been approved by jieyouxu

It is now in the queue for this repository.

@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-review

Status: Awaiting review from the assignee but also interested parties.

labels

Oct 20, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Oct 20, 2024

@bors

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Oct 20, 2024

@rust-timer

Rollup merge of rust-lang#121560 - Noratrieb:stop-lint-macro-nonsense, r=jieyouxu

Allow #[deny] inside #[forbid] as a no-op

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.

fixes rust-lang#121483

tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request

Jan 22, 2025

@tmeijn

This MR contains the following updates:

Package Update Change
rust minor 1.83.0 -> 1.84.0

MR created with the help of el-capitano/tools/renovate-bot.

Proposed changes to behavior should be submitted there as MRs.

Release Notes

rust-lang/rust (rust)

v1.84.0

Compare Source

==========================

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts

Cargo

Rustdoc

Compatibility Notes

Configuration

📅 Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

♻ Rebasing: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 Ignore: Close this MR and you won't be reminded about this update again.

This MR has been generated by Renovate Bot.

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request

Feb 2, 2025

@he32

Pkgsrc changes:

Upstream changes:

Version 1.84.1 (2025-01-30)

Version 1.84.0 (2025-01-09)

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts

Cargo

Rustdoc

Compatibility Notes

Labels

A-lints

Area: Lints (warnings about flaws in source code) such as unused_mut.

A-testsuite

Area: The testsuite used to check the correctness of rustc

disposition-merge

This issue / PR is in PFCP or FCP with a disposition to merge it.

finished-final-comment-period

The final comment period is finished for this PR / Issue.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-bootstrap

Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-lang

Relevant to the language team, which will review and decide on the PR/issue.