New "warning incompatible with previous forbid in same scope" error

[Nemo157]

I tried this code:

#![forbid(unused_extern_crates)]
#![warn(unused_extern_crates)]

fn main() {}

I expected to see this happen: an error that can be disabled via --cap-lints=warn

Instead, this happened: a hard error

> rustc temp.rs --cap-lints=warn
error[E0453]: warn(unused_extern_crates) incompatible with previous forbid in same scope
 --> temp.rs:2:9
  |
1 | #![forbid(unused_extern_crates)]
  |           ---------------- `forbid` level set here
2 | #![warn(unused_extern_crates)]
  |         ^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

After writing this up, looking back through the history (#77534, #70819) it appears this is fixing a regression, but given that this has been accepted by the stable compiler for over 3 years at this point, maybe it's still worth having a deprecation period? (This was noticed by a user on the discord as their code started failing to compile).

Meta

> rustc --version --verbose
rustc 1.49.0-nightly (91a79fb29 2020-10-07)
binary: rustc
commit-hash: 91a79fb29ac78d057d04dbe86be13d5dcc64309a
commit-date: 2020-10-07
host: x86_64-unknown-linux-gnu
release: 1.49.0-nightly
LLVM version: 11.0

[Nemo157]

cc @Mark-Simulacrum

[Mark-Simulacrum]

Nominating for lang team. I suspect we'll want crater, but maybe we just don't fix this.

[camelid]

I know we need backwards-compatibility, but what is the usefulness of supporting

#![forbid(unused_extern_crates)]
#![warn(unused_extern_crates)]

?

[Nemo157]

The actual original code was

#![forbid(rust_2018_idioms)]
#![warn(unused_extern_crates)]

[withoutboats]

Reviewing this in the lang team meeting I was surprised that this behavior is order-dependent. That is, this works without warning or error:

#![warn(unused_extern_crates)]
#![forbid(unused_extern_crates)]

fn main() {}

Not sure if this is backwards compatible with the present situation on stable or nightly, but to me the intuitive behavior of overlapping lint-setters at the same level is this: we take the maximum value that the user set, and then warn that the other attributes do nothing. That is, both of these examples (regardless of order), should set unused_extern_crates to forbid, and produce a warning that the warn attribute does nothing.

On the other hand, considering this different example:

#![warn(rust_2018_idioms)]
#![forbid(unused_extern_crates)]

fn main() {}

This should set unused_extern_crates to forbid, and every other lint in rust_2018_idioms to warn. There should be no unused attribute warning, because both have an effective impact on lint levels.

That is, I think the behavior should be order independent - two attributes applied to the same item (in this case, the top level module) should be considered the same regardless of the order they are in the program text.

[nikomatsakis]

We spent a while discussing this in today's lang-team meeting. We didn't reach a clear consensus on what we thought was the most appropriate behavior, but we did come up with some consensus points:

• definitely we should forbid in the crate if there is a forbid at the top level
  • because: if someone writes forbid(unsafe_code) and then someone PRs in some small line with warn(unsafe_code) on a module and then allow(unsafe_code) on an unsafe usage, that's really not what the person who wrote forbid wanted
  • note though that cap-lints does mean that you can't rely on forbid as a had guarantee (but also means that dependencies don't break)
• top level should not be special, all levels should behave the same
• we should at least warn, and maybe error, for forbid followed by allow
  • "later" items might be considered "within" former items, which corresponds to an error
  • but for back-compat or other reasons a warning ("this has no effect") might be reasonable
• we can either accept, warn, or hard error for allow followed by forbid
  • as above, if you think of allow on one level and forbid in a nested level, then this would be fine
  • but it'd be nice to get a warning that this allow had no effect at all

We'll discuss a bit more next week but here are some notes and questions:

• We'd like a clear timeline of what the behavior was at each point and maybe clarify a few points (some of these I added post meeting)
  • It would be good to clarify what the behavior is (and what it used to be) when combining forbid/allow in scopes other than the top-level -- is the top-level scope here special somehow?
  • Does order effect lints at other levels? What about command-line options?
• Crater results would be useful to get a sense of overall impact
• I think we'd be happy with something that ensures that the forbid takes precedence, but where we just issue lints for "incompatible" allow/forbid at the same attribute scope

Also, one point of clarification for @Nemo157. In the OP, you wrote:

I expected to see this happen: an error that can be disabled via --cap-lints=warn

At first, we thought this might mean that, prior to #77534, the cap-lints setting was used to determine whether a hard error occurred or not. But we later concluded that you were saying "cap-lints should affect the result here because otherwise existing crates in crates.io will stop compiling", is that correct?

[Nemo157]

But we later concluded that you were saying "cap-lints should affect the result here because otherwise existing crates in crates.io will stop compiling", is that correct?

Exactly, I wrote that before I had looked into the history and realised that this was fixing a prior regression, I was just assuming it was a newly added error that was being over-zealously implemented as a hard error rather than a deny-by-default lint.

[camelid]

Assigning P-high and removing I-prioritize as discussed in the prioritization working group.