Use true previous lint level when detecting overriden forbids

[Mark-Simulacrum]

Previously, cap-lints was ignored when checking the previous forbid level, which
meant that it was a hard error to do so. This is different from the normal
behavior of lints, which are silenced by cap-lints; if the forbid would not take
effect regardless, there is not much point in complaining about the fact that we
are reducing its level.

It might be considered a bug that even --cap-lints deny would suffice to
silence the error on overriding forbid, depending on if one cares about failing
the build or precisely forbid being set. But setting cap-lints to deny is quite
odd and not really done in practice, so we don't try to handle it specially.

This also unifies the code paths for nested and same-level scopes. However, the
special case for CLI lint flags is left in place (introduced by #70918) to fix
the regression noted in #70819. That means that CLI flags do not lint on forbid
being overridden by a non-forbid level. It is unclear whether this is a bug or a
desirable feature, but it is certainly inconsistent. CLI flags are a
sufficiently different "type" of place though that this is deemed out of scope
for this commit.

r? @pnkfelix perhaps?

cc #77713 -- not marking as "Fixes" because of the lack of proper unused attribute handling in this PR

[Mark-Simulacrum]

I am nominating this for lang discussion, as we had wanted to run an FCP on this. The PR description contains a description of the new behavior; it essentially matches #77713 (comment), though that had not really made clear that the behavior for nested scopes here also changes, avoiding a hard error that was previously present (but seemed undesirable).

[Mark-Simulacrum]

I'm also marking as relnotes because I would like for the regression here to be documented as a compatibility note. Technically it is really introduced by #77534 (so can point there instead) but the discussion here is likely to be a little more up to date.

The regression from 1.48 is that same-level forbid followed by a non-forbid level will now be an error, but was a silent override previously. This does not affect non-local crates (e.g., crates.io dependencies) so should be a minimal impact for most users.

[pnkfelix]

Hey @Mark-Simulacrum , is there any unit test here that demonstrates the specific case you describe in the comment (for example, something where one uses --cap-lints=deny on a file that itself has a #[forbid(..)] in it, and so you don't actually get an error from compiling the source due to the presence of --cap-lints ?

I skimmed the tests and nothing seemed to match that case, which is changing behavior here. If there is no such test yet, can you add one?

[pnkfelix]

r=me once that test is added (or shown to otherwise exist).

(But even with an r=me, this is still S-waiting-on-team for the T-lang review.)

[Mark-Simulacrum]

Yes, I can add such a test. Could you kick off t-lang fcp? I think that's the next step here in that regard, and there's no need to block it on a test.

[nikomatsakis]

I was going to make an fcp merge, but I wanted to float one variation. I might prefer a rule that says "violating a forbid is an error unless cap-lints is allow, in which case it's ignored". This fits the logic of "who cares if the lint is forbid given that it would be allow anyway".

[Mark-Simulacrum]

I personally see that as more complicated / confusing, in some sense. The behavior this PR implements is that the error only happens if a forbid lint would have occurred, that is, we error when lowering the level from forbid to not forbid. With cap-lints set to allow, warn, or deny, no forbid lint can occur; this means that the level is not lowered.

[nikomatsakis]

@rfcbot fcp merge

I am proposed to merge this PR. It adopts the following semantics:

• It is an error to lower the lint level from forbid to any other level.
• However, if cap-lints is set to something less than forbid, then the lint will never become forbid level, so no error is reported.

In terms of delta from nightly, the second point is newly introduced, so the net effect of the PR is to remove errors in the case where you are compiling code from crates.io.

In terms of delta from older versions, there were some cases, specifically where forbid and allow attributes were combined in the same scope, where the code was accepted. Those cases became errors in #77534 and they remain so under this PR (unless cap-lints is being used). This triggered however some regressions for extant code under crates.io, which motivates this PR.

[rfcbot]

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @withoutboats

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!

See this document for info about what commands tagged team members can give me.

[Mark-Simulacrum]

Note that lowering the forbid level (either on the same level or in a nested scope) is already an error on nightly, due to #77534 - this relaxes this restriction to only apply with cap-lints=forbid or no cap-lints at all, both in nested and same level scopes.

So the FCP here is really about removing errors in some cases, I think? Or at least that's what merging this PR means.

[nikomatsakis]

My write-up was supremely unclear. Apologies. I've edited it for clarity.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[Mark-Simulacrum]

@bors r=pnkfelix per #78864 (comment), added a test: https://github.com/rust-lang/rust/pull/78864/files#diff-d8f375061a7e2139c11ece2ac8a2499ff55d374a84a2ad8bc889393932c5d269.

[bors]

📌 Commit 64efcbe has been approved by pnkfelix

[bors]

⌛ Testing commit 64efcbe with merge eb4860c...

[bors]

☀️ Test successful - checks-actions
Approved by: pnkfelix
Pushing eb4860c to master...