TyCtxt::get_attr should check that no duplicates are allowed

[chenyukang]

Fixes #100631

[rust-highfive]

r? @compiler-errors

(rust-highfive has picked a reviewer for you, use r? to override)

[compiler-errors]

r? @lcnr

[chenyukang]

r? @lcnr
Not sure we need to check any other parts.
I tested with UI, not assertion crash will be triggered.

[lcnr]

we can't check whether there are multiple attributes, but instead have to check that we only use get_attr in cases where the compiler can reasonably deal with multiple attributes.

e.g. the following would ICE with your change (please add that as a test)

#[repr(C)]
#[repr(C)]
enum Foo {}

fn main() {}

instead check - similar to is_builtin_only_local - whether the feature definition in rustc_feature either warns or errors on duplicate attributes. This might not be enough as there could be some attributes where duplicates are allowed, but useless, but that's a bridge we can burn when we get to it.

[chenyukang]

got it, I will dig more and ping you if I need help.

[chenyukang]

We should check:

pub enum AttributeDuplicates {
    #[default]
    DuplicatesOk
    ...
}

rust/compiler/rustc_feature/src/builtin_attrs.rs

Line 101 in 8556e66

pub enum AttributeDuplicates {

If the attribute is marked as DuplicatesOk WarnFollowing, or WarnFollowingWordOnly, then we allow multiple attributes, otherwise raise an error?

[chenyukang]

We have a check for proper diagnostics,

rust/compiler/rustc_passes/src/check_attr.rs

Line 2223 in e1b28cd

ErrorFollowing | ErrorPreceding => match seen.entry(attr.name_or_empty()) {

[lcnr]

If the attribute is marked as DuplicatesOk WarnFollowing, or WarnFollowingWordOnly, then we allow multiple attributes, otherwise raise an error?

The other way around. We should only use get_attr for attributes which are either WarnFollowing, ErrorFollowing, ErrorPreceding, FutureWarnFollowing and FutureWarnPreceding. For all other attributes using get_attr is a compiler bug so we should ICE there. We should not emit errors for users in get_attr.

[chenyukang]

@lcnr Code updated and added a testcase.

[lcnr]

some nits, then r=me

thanks 👍

[chenyukang]

r=@lcnr

[compiler-errors]

@bors r=lcnr

[bors]

📌 Commit 5e8b356775549c66eec8cd31dcde60576268d766 has been approved by lcnr

It is now in the queue for this repository.

[bors]

@chenyukang: 🔑 Insufficient privileges: Not in reviewers

[chenyukang]

r? @lcnr

[lcnr]

@bors r+ rollup

[bors]

📌 Commit 7d45a75 has been approved by lcnr

It is now in the queue for this repository.

[JohnTitor]

Failed on rollup: #101463 (comment)
This needs a rebase and to move the test outside of src/test/ui/issues.

@bors r-

[chenyukang]

Failed on rollup: #101463 (comment) This needs a rebase and to move the test outside of src/test/ui/issues.

@bors r-

Rebase and updated.
Out of curious, why we would want to limit the file number in src/test/ui/issues?

[lcnr]

Out of curious, why we would want to limit the file number in src/test/ui/issues?

because large directories are difficult to navigate, both in editors and in the github view. E.g. https://github.com/rust-lang/rust/tree/master/src/test/ui/issues currently does not show 1146 entries, which is a annoying for people manually searching for a file

@bors delegate+

(after fixing the nit you can run @bors r=lcnr)

[bors]

✌️ @chenyukang can now approve this pull request

[bors]

📌 Commit 575e9b6414f9986f5dcdfbfc06356d554636aa86 has been approved by lcnr)`

It is now in the queue for this repository.

[lcnr]

woops
@bors r-

[chenyukang]

@bors r=lcnr

[bors]

📌 Commit 00b10a5 has been approved by lcnr

It is now in the queue for this repository.

[compiler-errors]

(side-note @chenyukang it's usually best to wait until CI is green until approving a PR, just in case it fails!)