Warn or error on duplicate attributes

[jdonszelmann]

During the port of built-in attributes we've found that some attributes give an error if repeated, some a warning, and some a "future error". i.e. warning for now saying that some day it's going to be an error.

@JonathanBrouwer started changing some to used with future error, and I think that's not unreasonable. For example, #[used] will under his PR error when used multiple times. See #142818 (comment), #142818 (comment) and #142823 (comment)

There is some value in allowing duplicates. For example, an attribute macro might emit #[used] and also #[allow(duplicate_attributes)] to make a function used regardless of a user making already having marked it as used. That saves the attribute macro from checking if the attribute is already there. Is that a good usecase? I don't know. However, since we currently do have all three options, I thought it might be good to open a discussion on what we should actually want, in general.

Some comments were made on zulip already. The questions to be answered are:

• Do we want the attributes that warn now to keep warning or to promise future error
• Attributes that promise future error now, when is that future? Do we actually want to make them an error or instead downgrade them to just a warning? See also A path towards erroring on nonsense attributes #142838
• Can we even come up with a policy or is it inherently attribute-dependent

And, if answers to those questions are complicated, what policy should we maintain for future attributes. Should list-like attributes always allow duplicates? Also if they're nested?

Some examples of the current state before the refactorings in progress. The refactorings can maintain the status quo, but maybe we want to formalize that.

duplicates obviously allowed

#[repr(C)]
#[repr(align(8))]
struct Foo;

duplicates allowed with warning of future error

#[inline(always)]
#[inline(never)] //~ WARN will be an error in the future
#[inline] //~ WARN will be an error in the future
fn foo() {}

duplicates allowed with a warning

#[cold]
#[cold] //~ WARN unused attribute
fn foo() {}

// NOTE: we might want to make this an error or warn-future-error? 
// but again, we have no policy why this is different, or not different than inline for example
#[used(compiler)]
#[used(linker)] // WARN unused attribute
fn bar() {}

duplicates disallowed

This one seems obvious

#[deprecated = "a"]
#[deprecated = "b"] //~ ERROR duplicate
fn foo() {}

This one also, sort of, though the keys are disjoint so in theory not much wrong with it.

#[deprecated(since="a")]
#[deprecated(note = "reason")] //~ ERROR duplicate
fn foo() {}

But this one less. It seems like splitting this over two attributes is not so different from repr for example.

#[proc_macro_derive(Foo, attributes(a, b, c))]
#[proc_macro_derive(attributes(e, f, g))] //~ ERORR duplicate
fn foo() {}

Similarly,

#[collapse_debuginfo(yes)]
#[collapse_debuginfo(external)]//~ ERORR duplicate
fn foo() {}

is not so different from inline or used, while this is a hard error and those two are future error or warning respectively.

Finally, there is also no real consensus which attribute takes precedence, the last or the first for built-in attributes. That one seems harder to fix since it might break people's code. It's easy to keep it as is, though we could think about changing that over an edition. Food for thought.

cc: @mejrs @JonathanBrouwer @oli-obk @bjorn3

[joshtriplett]

I do think whether this is a warning or error is going to depend on the attribute. For some attributes, and for some parameters of those attributes, it can be a warning; for other attributes, or for attributes with parameters that differ and conflict, I'd expect it to be an error.

[joshtriplett]

We discussed this in today's @rust-lang/lang meeting.

We agreed that we'd like to have a general policy for categories of duplicate attributes. I proposed, and there was general support for, the following three categories:

• Attributes that stack and provide non-conflicting, non-redundant information. For example, #[repr(C)] and #[repr(align(8))].
• Attributes that are redundant but non-conflicting. For example, #[inline] and #[inline], or #[repr(C)] and #[repr(C)].
• Attributes that conflict. For example, #[repr(C)] and #[repr(Rust)], or #[inline] and #[inline(never)].

Individual attributes will need to define what constitutes stacking, redundant, and conflicting.

Attributes that stack should continue to produce no warning.

We broadly agreed that we would like to at least warn on all redundant attributes, and that all such warnings should use the same lint to make it easy to allow (e.g. in a macro). We'd like to see a PR that we can FCP. Note that we did not rule out the possibility of doing something more than just warning in the future.

We broadly agreed that we would like all attributes that conflict to produce an error.

Note that any transitions should always be appropriately gradual; for instance, any transition to an error should likely go through some period of FCW.

[scottmcm]

Strongly agree this is going to depend on the details of the specific attribute.

My instinct (not speaking for the team, as I had to leave the meeting early and missed some conversation)

• Any semantic conflict needs to be a hard error. (#[repr(C)] #[repr(align(2))] might be fine, but #[repr(u32)] #[repr(u16)] clearly isn't.)
• We should prefer linting on duplicates for non-semantic things because we have more freedom to tune that later. For example, I think #[deprecated = "a"] #[deprecated = "b"] should be probably be considered fine at the "language spec" level -- then we don't have to say when it is or isn't fine in the spec -- but would obviously be worth a lint at the "hey, this is silly, we're going to ignore one of them" implementation level. (Notably, #[diagnositics::*] probably never has any hard errors for duplicates.)
  • I could even put inline under this if it was convenient for some reason, seeing as how it's not actually spec'd to do anything.
• Similarly I think I'd default to allowing duplicated semantic statements like #[repr(C)] #[repr(C)], though would also be happy for some fundamentally-semantic attributes to just not be allowed to be duplicated at all.

EDIT: Doh, raced with Josh's comment. I generally agree with those categorizations, but might be willing to give more leeway for conflicts that aren't actually semantic, like say must_use reasons.

[traviscross]

cc @ehuss