Change bindings_with_variant_name to deny-by-default

[timrobertsdev]

Changed the bindings_with_variant_name lint to deny-by-default and fixed up the affected tests.

Addresses #103442.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) soon.

Please see the contribution instructions for more information.

[fmease]

Not sure if I should comment on the issue or here on the PR. In any case, we can't just turn this into a deny-by-default lint without breaking Rust's backward compatibility guarantees, right? This should probably only ever happen in a new edition (e.g. starting from edition 2024) or after marking it as future-incompat and waiting for an extensive period of time. A crater run wouldn't be out of place either, I'd wager.

[lcnr]

Crater silently ignores lints from dependencies and having deny by default lints impact the root should be fine, as that's something the user can easily change themselves. Afaik upgrading a lint to deny is therefore generally not considered breaking.

I encountered this lint multiple times myself and iirc I never wanted to continue compilation in these cases as this lint always detected a bug.

I think this requires compiler signoff?

@rfcbot fcp merge

[rfcbot]

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

• @Aaron1011
• @cjgillot
• @davidtwco
• @eddyb
• @estebank
• @lcnr
• @matthewjasper
• @michaelwoerister
• @nagisa
• @nikomatsakis
• @oli-obk
• @petrochenkov
• @pnkfelix
• @wesleywiser

Concerns:

• proc-macros resolved by Change bindings_with_variant_name to deny-by-default #104154 (comment)

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.

[estebank]

@rfcbot concern proc-macros
How does this lint in particular interact with evaluated proc macros? I could see a weird situation where a clash occurs intentionally on the part of the macro author, but will the link either not trigger at all or only trigger on the macro's users' code?

[timrobertsdev]

@estebank
I'm not sure what you had in mind, but I put together this (very) short macro to see:

use proc_macro::TokenStream;
use quote::quote;

#[proc_macro_attribute]
pub fn foo(_input: TokenStream, _attributes: TokenStream) -> TokenStream {
    quote! {
        pub enum Foo {
            Bar,
            Baz,
        }

        pub fn bar(foo: Foo) {
            match foo {
                Bar => {},
                Baz => {},
            }
        }
    }.into()
}

Used as:

use macro_crate::foo;
#[foo]
pub struct Dummy;

Results in no errors being reported when checked or built.

[estebank]

@rfcbot resolve proc-macros

[scottmcm]

+💯. I'm a big fan of this -- warning lints for general smells, but when there are specific situations that we know are essentially always mistakes, they ought to be deny-only.

(FYI @rust-lang/lang that this is happening.)

[workingjubilee]

I agree that while technically valid Rust, it is absolutely improbable that anyone should want code that is formed like this to compile. Like "there's over 2 million Rust programmers and I expect less than 5 would want this to work, and they have already found out how to #![allow(nonstandard_style)]".

[rfcbot]

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

[bors]

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

[jhpratt]

Given that this is happening without an edition boundary, should there be a crater run? I suspect there will be minimal breakage, but it's better to be sure.

[scottmcm]

Given that this had been a warning for ages and it's so unlikely to be correct code, I think I'm fine with just letting the beta crater run catch any of them.

After all, it's still a lint, not a hard error, so even if it impacts stuff we're still allowed to do it.

[nikomatsakis]

@rfcbot fcp reviewed

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[scottmcm]

@timrobertsdev Could you rebase this, please? All the tests moved, which impacted a ton of PRs.

(Then we can merge it, since the FCP completed!)

@rustbot author

[timrobertsdev]

@scottmcm will do, it'll be a day or two until I can get to a PC to do so

[scottmcm]

Thanks, @timrobertsdev! I'm excited to start giving a deny for this.

@bors r+

[bors]

📌 Commit 1cbce72 has been approved by scottmcm

It is now in the queue for this repository.