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?

[Mark-Simulacrum]

Cc #81087

Yeah, I tend to agree that forbid warnings is too broad and you shouldn't do it.

I'm not sure about derive macros. It feels like macros shouldn't necessarily hide things from forbid, but I can also see the encapsulation argument - it's not really clear what the user actually wants. Probably forbid shouldn't affect macro expanded code as it's intentionally hidden from end users.

[bugadani]

Well, even deny and warn can annoy me a lot if the proc macro generates code that doesn't have docs ;)

[Mark-Simulacrum]

I think I figured out what the actual cause of the issue here is. forbid was just fundamentally broken, IMO, on lint groups - e.g. warnings - before eb4860c. I'm going to nominate this for T-lang, and I think there are two questions to answer:

• should forbid act on lint groups such that you cannot lower individual lints within the group?
• should forbid "propagate" through macros (e.g., derives)?

#![forbid(warnings)]

#[allow(dead_code)] // error on beta 1.50, no warnings/errors on stable
struct Bar;

[Mark-Simulacrum]

IMO, the answer is probably yes for the lint groups (this seems like a clear bug in previous behavior; if a lint is forbidden it should not be lowerable, regardless of precisely how that forbid was added), and a no for the second, for which an implementation exists at #81087

[bugadani]

should forbid "propagate" through macros (e.g., derives)?

I think this should be extended to any lint level. And with the same breath I'd want to argue for not propagating them. The code macros generate is something the user has little control over, and the user probably isn't even interested in any warnings the compiler may give for those.

[scottmcm]

We discussed this in the @rust-lang/lang triage meeting today.

Given that:

• This is about lint levels, so breakage is officially "minor" -- and thus allowed -- per https://rust-lang.github.io/rfcs/1105-api-evolution.html#minor-change-introducing-new-lint-warningserrors
• Cap lints correctly applies to these cases, so one is not impacted by a dependency that encounters this
• This only applies to forbid, the entire purpose of which (compared to deny) is to be a massive hammer that prevents bypassing
• The change to avoid this, if allowing lints later was desired in a particular codebase, is a simple update to use deny instead

The outcome was to stay with the behaviour as implemented in 1.50 as by-design, and not attempt to make-and-backport fixes to change the semantics involved. To make sure we have consensus across more than just the subset that was in the meeting,

@rfcbot fcp close

---

For extra clarity, this FCP is about this regression issue only, not a statement that everything is perfect about these scenarios. There were a variety of conversations about potential tweaks that might be interesting, from never triggered stylistic lints in derive-generated code, to finding a way to report stylistic lints in the definitions of macros, to full lint hygiene. But those are future possibilities that should be discussed and tracked elsewhere, not considered as part of this 1.50.0-milestone issue.

[rfcbot]

Team member @scottmcm has proposed to close 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.

[danielhenrymantilla]

So, things like the following pattern would fail to compile?

#![forbid(const_err)]

::some_helper_crate::const_assert!( true );

• See, for instance:

  https://github.com/jyn514/saltwater/blob/5cc8a4ba33cdd557b14b6b6c15940baf23b769aa/saltwater-parser/macros.rs#L41-L50

[Mark-Simulacrum]

I would expect that to fail on stable already, macros do not currently mask or provide any kind of hygiene to forbid checking (and never have as far as I know). The new behavior that may cause local breakage is that a forbid of warnings would previously not cause allows or denies of more specific lints to error, but now does.

[danielhenrymantilla]

Hmm, you're right, that behavior seems to have been around forever. I personally find it very and deeply counterintuitive for a #[forbid(lint)] to cause further #[deny(lint)]s to fail; it also thus leads to a special handling to be done by macros:

• when dealing with non-user-provided code (i.e., only macro-specific code), #[forbid(…)] must be used instead of #[deny(…)], since the latter fails should the caller be in a forbid-scope.

• when dealing with a mix of macro-generated code and forwarded user code, there is no way to deny a lint in a robust fashion (i.e., forbiding it as per the previous point) without making it impossible for the user code to break that lint.

That is, if we take the previous example, https://github.com/jyn514/saltwater/blob/5cc8a4ba33cdd557b14b6b6c15940baf23b769aa/saltwater-parser/macros.rs#L41-L50, #[deny(const_err)] should be replaced by #[forbid(const_err)], which means that $condition can no longer opt into supporting const_errs.

In that instance, there is a convoluted way to circumvent that, but that transformation may not be always accessible to all macros:

  #[macro_export]
  macro_rules! const_assert {
      ($condition:expr) => {
-         #[deny(const_err)]
-         const _: usize = 0 - !$condition as usize;
+         const _: () = {
+             const CONDITION: bool = $condition;
+             #[forbid(const_err)]
+             const _: usize = 0 - !CONDITION as usize;
+         };
      };
  }

[Mark-Simulacrum]

I think this problem was noted in lang discussion, but I would encourage you to open a different issue about it.

[pnkfelix]

I interpreted @scottmcm 's FCP proposal as saying that the problem raised by @Mark-Simulacrum above is a more specific issue that should be dealt with on its own, and that is why i filed #81218.

(If I misinterpreted your proposal, @scottmcm, let me know. E.g. it could be that you were saying above that the observed consensus was that we are willing to accept that forbid(warnings) is going to error when combined with various derives, and that the user should be using deny(warnings) instead in all such cases (as is discussed to some degree in this issue). If that is our official policy, then we probably should have diagnostics recommending such action, at least specifically for forbid(warnings). But I would prefer we try to figure out something to deal with the derive case.)

[Mark-Simulacrum]

I think nominated label was just leftover, this seems on track for closing with fcp to close. In any case there will be some opportunity for discussion via #81218 if necessary

[nikomatsakis]

@rfcbot reviewed

[rfcbot]

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

[scottmcm]

@rfcbot cancel

We discussed this in the meeting today, and decided that forcing this to break immediately is too strong. @pnkfelix is going to be filing a backportable PR to make at least forbid(warnings) a future-incompatibility thing, so it'll complain on 1.50 but not immediately require code changes on the update. (But likely will require code changes at some point in the future.)

[rfcbot]

@scottmcm proposal cancelled.

[pietroalbini]

Fix backported to 1.50.0 and landed in 1.51.0, closing this.