Fix how allow/warn/deny/forbid warnings is handled

[FabianWolff]

This pull request attempts to fix #41941, fix #70819 (the command line part, which is why the issue was reopened), and fix #75668. The current implementation of warnings (as in -D warnings or #[allow(warnings)]) is "lazy" in that warnings is not considered by the linter until it is about to emit a warning, in which case it checks whether an allow/deny/forbid of warnings is in scope and up-/downgrades the warning accordingly.

The problem with this approach is that one can never get an (e.g.) #![allow(warnings)] out of scope again except by shadowing it with another #[warn/deny/forbid(warnings)]; ideally, though, one would also want more fine-grained control, such as

#![allow(warnings)]

fn main() {
    #[warn(unused)]
    let a = 5;
}

which compiles without warnings and errors on current nightly but causes a warning with my changes (as one would expect, I would argue).

My solution basically consists of adding a "depth" and "position" to LintLevelSource. This way, whenever a warning is about to be issued, we can check the source of the warning and the source of the warnings pseudo-lint (if any), and see whether the latter can override the former. For instance, the following causes an error on nightly and a warning with my changes:

#[warn(unused)]
#[deny(warnings)] // Same depth, greater position (in the attributes list), overrides `warn(unused)`
fn main() {
    #[warn(unused_variables)] // Greater depth, overrides `deny(warnings)`
    let a = 5;
}

I've also worked on the command line arguments: Calling rustc with -F warnings -A unused works without errors or warnings on current nightly; with my changes, I get

warning: allow(unused) incompatible with previous forbid
  |
  = note: `#[warn(forbidden_lint_groups)]` on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #81670 <https://github.com/rust-lang/rust/issues/81670>
  = note: `forbid` lint level was set on command line

[rust-highfive]

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

[petrochenkov]

r? @Mark-Simulacrum @pnkfelix (or who else dealt with these issues the last time)
(I don't currently have capacity to review nontrivial PRs outside of my main expertise areas.)

[Mark-Simulacrum]

I don't think this patch can be accepted as-is because it breaks the common (though sort of "incorrect" from first principles in some sense) pattern of specifying #![warn(...)] or -W... on some lints, perhaps additional, perhaps replacing allow at 'higher' levels, and then using -Dwarnings to change all of them to errors, independent of why they're being emitted. To some extent, I guess this desire is basically a "inverse cap-lints", but not quite rust-lang/compiler-team#434.

I suspect changes here are going to want to go through an RFC or at least some sort of broader consideration (nominating for @rust-lang/lang), because it seems like the design space here is somewhat broad and there is likely some amount of breakage to consider.

[nikomatsakis]

Hmm. I've not read deeply yet, but I will second the motivation: when I saw how the warnings lint was implemented, I was quite surprised, it doesn't really match my expectations.

[bors]

☔ The latest upstream changes (presumably #85788) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #86311) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #86009) made this pull request unmergeable. Please resolve the merge conflicts.

[nikomatsakis]

Summary for @rust-lang/lang team meeting:

• The warnings "lint group" is currently implemented in a surprising way:
  • Basically, it becomes a kind of "ambient" setting, so that when a lint would otherwise be a warning, it gets changed to the level of the warnings group
• Downside is: you cannot override it

Example:

#[warn(unused)]
#[deny(warnings)]
fn main() {
    #[warn(unused_variables)]
    let a = 5; // Still gets an error
}

yields

error: unused variable: `a`
 --> src/main.rs:5:9
  |
5 |     let a = 5; // Still gets an error
  |         ^ help: if this is intentional, prefix it with an underscore: `_a`
  |
note: the lint level is defined here
 --> src/main.rs:2:8
  |
2 | #[deny(warnings)]
  |        ^^^^^^^^
  = note: `#[deny(unused_variables)]` implied by `#[deny(warnings)]`

• The PR changes this so that lint level declarations that are "inside" the warnings take precedence.
• Things that come at the same level, but "later" take precedence

[joshtriplett]

We discussed this in today's @rust-lang/lang meeting.

Several people expected that deny(warnings) should behave like -Werror, which it effectively does today. Those people felt the current behavior should be kept.

Others expected that it behaved like a lint group, but still felt that it may not be worth breaking compatibility with existing code and expectations.

Based on that, the general consensus was roughly that we should keep the current behavior for deny(warnings), but that we should document that behavior more clearly and explain that deny(warnings) has a distinct semantic that isn't interpreted in terms of lint groups. We might also be open to warnings/errors that flag cases where this behavior may be confusing.

We may also want to examine the behavior of allow(warnings) to figure out if it does what we want.

[nikomatsakis]

I was just thinking after the meeting:

I think it may be worth deprecating the "warnings" lint group and introducing an alternative. @Mark-Simulacrum mentioned the -Zforce-warn option that we are using for the edition transition -- which basically forces a given set of lints to be a warning, regardless of whether the user attempted to suppress them -- and there does indeed seem to be some sort of relationship.

Personally, I think I would prefer an attribute like #[warnings(lint_level)]:

#![warnings(deny)]

and then a command line like --warnings-level deny. I probably would not allow #[warnings(forbid)], as that is kind of nonsensical, although it could plausibly be used to mean "and you cannot change the warnings level within this code". So maybe it does make sense.

That would seem much clearer to me. We could then deprecate the warnings group and suggest a rewrite when it appears in the code to this new form.

That said, I don't want to derail this PR with discussion of that specific idea. The long and short of it is that it seemed clear from the meeting that it is best not to think of "warnings" as a lint group, but rather its own thing. I would prefer that if we go that route, we stick to it.

(This is true even though I always thought of it as a group, and I can see some value to having a group, perhaps called default_warnings or something, that corresponds to "any lint that is configured to warn-by-default.)

[joshtriplett]

I definitely think it'd be clearer that way, but the transition cost seems painful for all the people using this in their projects (including on the command line for CI).

[pnkfelix]

I definitely think it'd be clearer that way, but the transition cost seems painful for all the people using this in their projects (including on the command line for CI).

I was assuming we could continue supporting the old forms indefinitely, but provide a lint that would warn ... or, hmm, would that run afoul of our chicken/egg issues that deny(warnings) makes such lints into errors...? Is that the reason that doesn't work? (Or would the lint not be in the scope of the deny(warnings), and so it wouldn't hit that problem?)