Raise some occurrences of unused_attributes to deny by default

[mejrs]

This is done by creating a new lint, harmful_unused_attributes, which is like unused_attributes but deny by default.

Some attributes use the former lint while others use the latter.

The following attributes use the new lint (fixme: double check this list):

• ABI/linking/layout/etc related: link_name, no_mangle, instruction_set, repr, export_name, link, target_feature
• semver: non_exhaustive

For example, these now all raise a deny by default lint.

macro_rules! produce_struct {
    ($name: ident) => {
        pub struct $name {}
    };
}

#[repr(align(64))] // Deny
produce_struct!(Foo);

extern "C" {
    #[link_name = "foo"] // Ok
    pub fn bar();

    #[link(name = "foo")] // Deny
    pub fn baz();
}

#[export_name = "foo"]
#[export_name = "bar"] // Deny
pub fn baz() {}

Another option is to raise unused_attributes to deny by default. But I don't think this is a good idea:

• Much code out there has allowed unused and unused_attributes already
• A previous crater run for part of this ([CRATER] Deny almost all attributes on macro calls #158816) showed that many macros expand into code that make relatively harmless mistakes like putting #[automatically_derived] on non-trait impls, for example. So raising this lint to deny by default would mean that a lot of people would have no choice but to globally allow unused_attributes.
• A lot of these cases can (perhaps should) probably just be errors, but that wouldn't simplify the compiler so I personally don't care much if it's an error or deny by default lint.
• It is implemented in this branch, I personally don't like the changes to test output, think it's too heavy handed for what are mostly just harmless mistakes.

r? @JonathanBrouwer

[rustbot]

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

[rustbot]

JonathanBrouwer is currently at their maximum review capacity.
They may take a while to respond.

[rustbot]

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

[rustbot]

miri is developed in its own repository. If the Miri part of this change can be broken out, consider making this change to rust-lang/miri instead. However, if Miri needs adjusting for rustc changes, just ignore this message.

cc @rust-lang/miri

[RalfJung]

This is not an FCW. Is that deliberate? The move to "deny" makes it seem like we may want to fully reject this in the future, or is that not planned?

View changes since the review

[mejrs]

It (and unused_attributes) does emit

   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!

but this is a part of the diagnostic struct, not the lint definition. I'm not sure why, that ought to be fixed I believe.

Anyway, yes this is the direction we'd like to go in. See #142836 (comment) and #142838 (comment) for the lang team's opinion.

The lint here covers "conflicting" attributes (as defined in #142836 (comment)) e.g. multiple export_name and link_name attributes, as well as some dangerous cases of nonsense attributes.

Crater permitting, some of these can be turned into errors at some point. But just erroring on everything here would cause far too much breakage, examples:

• erroring on #[non_exhaustive] on macro calls ([CRATER] Deny almost all attributes on macro calls #158816 (comment)) would break 600+ crates depending on old crossterm versions.
• there are several examples in our test suite (but not in the standard library, thankfully) like #[no_mangle] on structs etc, there are probably loads of these out there

[mejrs]

btw @JonathanBrouwer what's up with how the fcw is emitted?

[JonathanBrouwer]

I think this has been the case since before I joined the project, I just moved the code around a few times, I agree this should be a FCW lint and fixing this is on my todo list.

[JonathanBrouwer]

Another option is to raise unused_attributes to deny by default. But I don't think this is a good idea:

I agree, some unused_attributes are a lot more harmful than others, and breakage is not always worth it.

[JonathanBrouwer]

I think we should take a more structured approach to this problem, rather than solve the arbitrary subset of the problem that is fixed in this PR:

• Make a complete list, in an issue, of all places where UNUSED_ATTRIBUTES is emitted.
• Before implementing this in a PR, we should sit down and sort these into "harmful" unused attributes (ones we wish were errors) and "fine" unused attributes.
  • I'm happy to schedule a call for this, or do this async, whatever you prefer
  • We should also include all cases of USELESS_DEPRECATED and INVALID_DOC_ATTRIBUTES in this list
  • An example of a "fine" unused attribute is #[allow()], it has a clear meaning, but that meaning is a no-op
  • In my opinion, all target checking errors are harmful
• We should then implement this list, run crater, and sort the "harmful" category in "we can break this" and "this causes too much breakage".
• We should then implement a new PR for the "we can break this" category, convert those to an error, and propose this to T-lang.
• We should make a separate PR where we take all "this causes too much breakage" warnings, convert those to a new FCW warn-by-default lint HARMFUL_UNUSED_ATTRIBUTES, propose this to T-lang. Hopefully moving the cases to a separate lint does not cause too much breakage 🤞
• Then in a year or two we can make HARMFUL_UNUSED_ATTRIBUTES deny-by-default

How do you feel about this plan?
@rustbot author

cc @jdonszelmann as you might be interested in what we're doing (or not)

[rust-bors]

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