regression 1.50: deny after forbid breaks build

[rylev]

The following code compiles on 1.49 but does not compile on 1.50 beta.

#![forbid(warnings)]
#![deny(bad_style)]

fn main() {}

This seems to be a similar (the same?) issue as #77713 though I'm not sure how it would have been introduced.

This breaks several crates in the latest crater run. Interestingly, some of the broken crates had some strange error messages:

   --> src/lib.rs:132:17
    |
25  |     warnings
    |     -------- `forbid` level set here
...
132 | #[derive(Debug, PartialEq, Eq, Hash, Clone)]
    |                 ^^^^^^^^^ overruled by previous forbid

@rustbot modify labels: +regression-from-stable-to-beta

[rylev]

I believe the issue here is that in 1.49 this behaviour was reverted as a backport to beta and never changed in master.

@Mark-Simulacrum since you did the previous backport, what's the right step here? Do we backport again? Shouldn't we also revert in master?

[Mark-Simulacrum]

#78864 is the "fix" which should mean that the breakage is limited to leaf crates, which aren't lint capped.

I believe we're otherwise likely to accept the breakage in crates - my recollection is that lang felt that it was more important that forbid actually took effect than to retain compatibility, because people specifying forbid are making a pretty specific request.

I will look into the specific crates that broke here though, since they don't obviously look like ones I'd expect to get broken. Maybe there's some bug in the implementation or we need to polish our built-in derives.

[apiraino]

Assigning P-high as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

[scottmcm]

my recollection is that lang felt that it was more important that forbid actually took effect than to retain compatibility, because people specifying forbid are making a pretty specific request.

That's certainly my position. If they wanted to allow tweaking the level later, they could have used #![deny(warnings)] instead.

[Mark-Simulacrum]

#![forbid(
    exceeding_bitshifts, mutable_transmutes, no_mangle_const_items, unknown_crate_types, warnings
)]
#![deny(
    bad_style, deprecated, improper_ctypes, missing_docs, non_shorthand_field_patterns,
    overflowing_literals, plugin_as_library, private_no_mangle_fns, private_no_mangle_statics,
    stable_features, unconditional_recursion, unknown_lints, unsafe_code, unused, unused_allocation,
    unused_attributes, unused_comparisons, unused_features, unused_parens, while_true
)]
#![warn(
    trivial_casts, trivial_numeric_casts, unused_extern_crates, unused_import_braces,
    unused_qualifications, unused_results
)]
#![allow(
    box_pointers, missing_copy_implementations, missing_debug_implementations,
    variant_size_differences
)]

I think this exposes a potential problem in the current implementation - forbidding a lint group prevents you from then lowering the lint level for some parts of that group. This could be seen as a positive, but it's also pretty restrictive, and could be annoying for users. I feel that we should close this as accepted breakage, documenting in relnotes that forbid now applies at the same level as well as a compatibility note.

[Mark-Simulacrum]

As another note, I noticed the crater run was cap-lints=warn, which means the quantity of crates doing this isn't fully assessed. It looks like rustdoc doc tests ignore this for some reason.

[Mark-Simulacrum]

#![forbid(warnings)]

#[derive(serde::Serialize)]
struct Bar;

This code fails to compile on beta, because serde expands with a number of allow attributes. I guess it's an open question exactly what kind of hygiene is appropriate here, but presumably the intent would be that we don't propagate lint levels into macro-expanded code. I imagine that's going to be a pain to fix though.

I admit to being a bit baffled as to why this wasn't hit previously - I am not seeing anything obvious in eb4860c which I confirmed via bisection to be the cause of the problem here...

[scottmcm]

I think my main takeaway here is that nobody should ever do something as broad as #![forbid(warnings)] -- that picks up enough things that there's no way they need that strong a guarantee about it, and they should be using #![deny(warnings)] instead.

But bleh, hygiene is hard. Should macro-expanded code be using the lint levels from the crate defining the macro?