Rust: `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 https://github.com/rust-lang/rust/pull/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 https://github.com/laumann/compiletest-rs/issues/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: https://github.com/rust-lang/rust/pull/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 https://github.com/rust-lang/rust/pull/67885, though of course it can never hurt to double-check.
• The regression in the handling of inline attributes that I described in https://github.com/rust-lang/rust/issues/70819#issuecomment-609450864. 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 https://github.com/rust-lang/rust/commit/61d9231ff2604a0467987042d9ebf9ff9ea739b5 to https://github.com/rust-lang/rust/commit/5e7af4669f80e5f682141f050193ab679afdb4b1
regressed commit: https://github.com/rust-lang/rust/commit/5e7af4669f80e5f682141f050193ab679afdb4b1

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 https://github.com/rust-lang/rust/pull/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
}

[Centril]

Hmm... It feels clear to me that this is a bug (the attributes part, the command line aspect is up to T-compiler). So to potentially save the language team meeting some time (we didn't get to this issue on yesterday's meeting), I wonder if @pnkfelix could clarify what aspect the language team should discuss and provide feedback on here.

[pnkfelix]

The reason @rust-lang/wg-prioritization added T-lang was because figured that the semantics of the attribute-based control of the lints was part of the language, not the tooling. (The fact that the command-line controls was changed to match that of the attribute-based control is in the domain of T-compiler.)

However, it seems likely that this is "just a bug" in how lint controls in the same "scope level" are handled, and so I would be fine with treating this as solely the domain of T-compiler, not T-lang.

(Given that the attribute based controls have had this behavior since the middle of 2017, I figure it would still be reasonable to float this up to the lang team; not as something to debate, but more of a "general notice, here's a deep bug we recently discovered...")

[pnkfelix]

(anyway, we discussed this at today's ad hoc triage meeting). I suspect at this point we can remove the nomination tag; its looking like everyone agree that this is "just a bug" that needs to be fixed, not even a terribly high priority one.)

[nikomatsakis]

@pnkfelix mentioned in the compiler-team meeting today that P-medium didn't seem high enough for this bug. I'm inclined to agree. I think we ought to fix this. I wouldn't call it P-critical, but seems like P-high?

[LeSeulArtichaut]

(Added I-prioritize to discuss this issue with the Prioritization WG)

[pnkfelix]

One thing that I should have brought up sooner:

Should this bug be reclassified as a stable-to-stable regression rather than a stable-to-beta one, given that the underlying problem as identified by @RalfJung up above has been present since version 1.21 in 2017?

[pnkfelix]

To be clear, reclassifying as a stable-to-stable regression would reduce the pressure to resolve this in time for the release on June 4th.

(The stable-to-beta regression at hand was the CLI behavior regression, which was already addressed by PR #70918 )

[RalfJung]

Yeah I guess it makes sense, given that current stable already contains the regression.

[petrochenkov]

The PR fixing this (https://github.com/rust-lang/rust/pull/73379) was closed due to inactivity.
Could someone rebase it, update clippy tests and resubmit?

[Mark-Simulacrum]

I can do that.