forbid overwritten by later allow on the same "scope level"

[RalfJung]

forbid is not honored when it is followed by an allow on the same "scope level". This compiles, but should fail:

#![forbid(unused)]
#![allow(unused)]

fn square(num: i32) -> i32 {
    num * num
}

If we move the allow down to the function level, it fails to compile as expected:

#![forbid(unused)]

#[allow(unused)]
fn square(num: i32) -> i32 {
    num * num
}

The same issue also arises with CLI arguments, which all share the "scope level". That was worked around with in #70918, but once the above is fixed, that hack can likely be removed.

Original description

According to my understanding, the purpose of -F/forbid for lints is that they can not be allowed any more. Thus I would expect that calling rustc with -Funused -Aunused will fail when there is unused code in the file.

However, that is not the case: with that sequence of arguments, unused code is silently accepted.

Cc Manishearth/compiletest-rs#216

[RalfJung]

This is in fact a stable-to-beta regression: with Rust 1.42, the code still gets rejected as expected.

[RalfJung]

Likely regression candidate: #67885

[RalfJung]

Interestingly, doing the same via flags in the source code also does not work as expected -- the allow overwrites the forbid!

#![forbid(unused)]
#![allow(unused)]

fn square(num: i32) -> i32 {
    num * num
}

This is also a regression, but not a recent one: with Rust 1.20, the code gets rejected as expected; with Rust 1.21 and later it gets accepted. It is also in direct contradiction with the documentation which says

'forbid' is a special lint level that's stronger than 'deny'. It's the same as 'deny' in that a lint at this level will produce an error, but unlike the 'deny' level, the 'forbid' level can not be overridden to be anything lower than an error.

[spastorino]

According to @RalfJung this has possibly regressed on #67885.
I guess this needs confirmation ...

@rustbot ping cleanup

[rustbot]

Hey Cleanup Crew ICE-breakers! This bug has been identified as a good
"Cleanup ICE-breaking candidate". In case it's useful, here are some
instructions for tackling these sorts of bugs. Maybe take a look?
Thanks! <3

cc @AminArria @chrissimpkins @contrun @DutchGhost @elshize @ethanboxx @h-michael @HallerPatrick @hdhoang @hellow554 @imtsuki @jakevossen5 @kanru @KarlK90 @LeSeulArtichaut @MAdrianMattocks @matheus-consoli @mental32 @nmccarty @Noah-Kennedy @pard68 @PeytonT @pierreN @Redblueflame @RobbieClarken @RobertoSnap @robjtede @SarthakSingh31 @senden9 @shekohex @sinato @spastorino @turboladen @woshilapin @yerke

[spastorino]

Assigning P-medium and removing nomination. This was discussed as part of pre-triage meeting in Zulip.

[spastorino]

Nominating also according to what was discussed during the meeting for T-lang discussion.

[RalfJung]

Note that there are two separate regressions going on:

• The CLI argument regression in the original post, which I noticed first. I am reasonably sure that one stems from rustc_session: allow overriding lint level of individual lints from a group #67885, though of course it can never hurt to double-check.
• The regression in the handling of inline attributes that I described in forbid overwritten by later allow on the same "scope level" #70819 (comment). That one is somewhere between the 1.20 and 1.21 release, and pinning this down more precisely would be very useful indeed.

[AminArria]

Bisect of CLI arguments:

searched nightlies: from nightly-2020-02-04 to nightly-2020-02-18
regressed nightly: nightly-2020-02-17
searched commits: from 61d9231 to 5e7af46
regressed commit: 5e7af46

Indeed it seems the issue comes from #67885

[tobithiel]

I opened #70918 to address the CLI argument regression

[AminArria]

Bisect of inline attributes:

searched toolchains nightly-2017-07-27 through nightly-2017-10-09
regression in nightly-2017-08-11

Checking for commit hashes in that nightly and the one before

$> rustc +nightly-2017-08-11 -vV
rustc 1.21.0-nightly (13d94d5fa 2017-08-10)
binary: rustc
commit-hash: 13d94d5fa8129a34f5c77a1bcd76983f5aed2434

$> rustc +nightly-2017-08-10 -vV
rustc 1.21.0-nightly (f14249953 2017-08-09)
binary: rustc
commit-hash: f142499539d038ef60f4e22cafe11ecdd8a29a1d

Commits by bors in that range:

$> git log --author=bors --format=oneline f142499539d038ef60f4e22cafe11ecdd8a29a1d..13d94d5fa8129a34f5c77a1bcd76983f5aed2434
13d94d5fa8129a34f5c77a1bcd76983f5aed2434 Auto merge of #43559 - Nashenas88:nll-region-renumberer, r=arielb1
b6179602bea71607a9ea63197eca423fcce5f7b0 Auto merge of #43720 - pornel:staticconst, r=petrochenkov
2400ebfe76225745d1591c8a63c54570174ab22c Auto merge of #43522 - alexcrichton:rewrite-lints, r=michaelwoerister
d21ec9b4efd1da012979b050bc0a0426fe45fcdf Auto merge of #43582 - ivanbakel:unused_mut_ref, r=arielb1
2ac5f7d249e29ee48737359e0e6dd9e59701a568 Auto merge of #43737 - GuillaumeGomez:duplicate-method, r=eddyb
16268a88fc0cfe3657439139c63913ffb904b2fa Auto merge of #43735 - est31:master, r=alexcrichton
57e720d2cd09b6befc5b6eed66b65352fc9ff537 Auto merge of #43730 - nrc:driver-shim, r=eddyb
875bcf5c5f7efcbf72e000d256b24293555edf53 Auto merge of #43627 - Aaronepower:master, r=alexcrichton

[RalfJung]

@AminArria thanks a lot! I would guess the most likely regression candidate is #43522, then.

My guess would be that somewhere around here, lint groups are not handled properly. This code looks different from the previous forbid handling, though why one would treat lint groups properly and one would not is beyond me.

Cc @alexcrichton @michaelwoerister (PR author and reviewer)... but this is from 2017 so I doubt they will remember many details. ;) Cc @rust-lang/wg-diagnostics for people with lint knowledge.

[alexcrichton]

I'm not sure I have a ton to add here myself, this definitely feels like a bug (both CLI and attribute handling) and seems reasonable to fix, and the fix is likely the same for both.

[RalfJung]

Turns out the problem is not about lint groups, it is about forbid being on the same "scope level" as allow. The following behaves as expected (i.e., it fails to compile):

#![forbid(unused)]

#[allow(unused)]
fn square(num: i32) -> i32 {
    num * num
}

[petrochenkov]

The PR fixing this (#73379) was closed due to inactivity.
Could someone rebase it, update clippy tests and resubmit?

[Mark-Simulacrum]

I can do that.

[SparrowLii]

@RalfJung The FIXME here links to this closed issue. I think it can be removed?

[RalfJung]

Maybe? It is worth trying, but I don't know that code very well.

EDIT: By "it" I mean removing the usize::MAX hack, not just the FIXME comment.

[FabianWolff]

Unfortunately not, it seems. With the following input file:

fn main() {
    let a = 5;
}

I get, on current nightly with -F unused -A unused:

error: unused variable: `a`
 --> t1.rs:2:9
  |
2 |     let a = 5;
  |         ^ help: if this is intentional, prefix it with an underscore: `_a`
  |
  = note: `-F unused-variables` implied by `-F unused`

error: aborting due to previous error

which is actually suboptimal, because there is no warning about the -A being overwritten by the -F (with crate attributes #![forbid(unused)] and #![allow(unused)], there is a warning, and it even mentions that this will become a hard error in the future).

In any case, with the following patch applied and the same command line arguments:

diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs
index f517c483758..436afad11b8 100644
--- a/compiler/rustc_session/src/config.rs
+++ b/compiler/rustc_session/src/config.rs
@@ -1161,15 +1161,7 @@ pub fn get_cmd_lint_options(
     let mut describe_lints = false;
 
     for &level in &[lint::Allow, lint::Warn, lint::Deny, lint::Forbid] {
-        for (passed_arg_pos, lint_name) in matches.opt_strs_pos(level.as_str()) {
-            let arg_pos = if let lint::Forbid = level {
-                // HACK: forbid is always specified last, so it can't be overridden.
-                // FIXME: remove this once <https://github.com/rust-lang/rust/issues/70819> is
-                // fixed and `forbid` works as expected.
-                usize::MAX
-            } else {
-                passed_arg_pos
-            };
+        for (arg_pos, lint_name) in matches.opt_strs_pos(level.as_str()) {
             if lint_name == "help" {
                 describe_lints = true;
             } else {

I get no warnings and no errors (incorrectly, because -F should not be overwritten by -A).

[RalfJung]

I get no warnings and no errors (incorrectly, because -F should not be overwritten by -A).

Yeah that's wrong... so the FIXME is still accurate, though I am not sure why.

[nikomatsakis]

I'm going to briefly re-open this issue -- I don't remember what we decided about -F and -A, but I bet @Mark-Simulacrum does =)

[pnkfelix]

Arguably this shouldn't be assigned to me anymore.

Its possible we may want to close this and open a fresh issue (potentially a P-medium one, or just freshly re-prioritized) dedicated to the command line interactions between -F and -A

[jieyouxu]

Visited during T-compiler P-high review: closing this issue as the original issue is resolved, opened #133346 for the -F and -A cli interactions.

[RalfJung]

The original issue was in fact "According to my understanding, the purpose of -F/forbid for lints is that they can not be allowed any more. Thus I would expect that calling rustc with -Funused -Aunused will fail when there is unused code in the file.", as can be seen in the issue description. That issue is unresolved.

But we can move it to a new issue, sure. 🤷

[jieyouxu]

Ah right, I think when I visited this issue I focused on the recent comments then forgot about the issue description, sorry 😅

[jieyouxu]

I tried to transcribe the inconsistency issue in the new issue #133346.