Fix ICE caused by parsed attributes applied to where bounds

[JonathanBrouwer]

Fixes #143780
Fixes #138510
Fixes #143787
Fixes #143094 still needs a test

r? @oli-obk
cc @jdonszelmann

[rustbot]

Some changes occurred in compiler/rustc_attr_data_structures

cc @jdonszelmann

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

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

cc @jdonszelmann

[JonathanBrouwer]

I fixed this issue by giving all AttributeKind variants a span. I think this is the better way, the alternative would be removing the span from all variants and adding it to Attribute::Parsed.

I went for this approach because:

1. I think Attribute::Unparsed is probably going away in the future anyways? (not sure, cc @jdonszelmann)
2. Putting the span in Attribute::Unparsed means the find_attr! macro no longer has access to it. This could be fixed by giving the macro the span as a separate argument but that makes the macro calls quite a bit uglier

[rustbot]

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

[cyrgani]

does this also fix #143787?

[JonathanBrouwer]

Yes it does fix the ICE, I've added it to the fixes list and added it to the tests

[jdonszelmann]

This exact pr has been proposed several times at this point by different people. However, I'm again blocking it since eventually I want to remove the .span() function from attributes. It's there just for the transition period. Many attributes don't have a single span to point to. Maybe, just maybe I'm in favor of merging this when the method is called "first_span" but even then I think people will use it in a way that makes diagnostics confusing. Diagnostics that care about the span of attributes should either be about a specific attribute (in which case the code knows what all the spans in the parsed attribute mean) or should happen before parsing. Finally there's the case of Unparsed, which will only stick around for tool attributes. My plan for those is to parse those into a kind of meta item syntax tree (remove the tokens) and expose that to whoever wants to deal with tool attrs.

[jdonszelmann]

I know im blocking those ICEs from being fixed rn. But I strongly believe those ICEs should be fixed by not using .span(). Not by expanding the method.

[JonathanBrouwer]

@jdonszelmann I understand completely, and I agree that this method should be avoided as much as possible

However, I don't see a way to fix this specific ICE (http://github.com/rust-lang/rust/issues/138510) without such a method, even after all attributes are parsed. This specific check has an allowlist of allowed attributes, and needs to span to error on attributes that are not on that allowlist. And I think this is an ICE that I would like to see fixed, it can be triggered even without the feature flag and has been reported 3 times now.

I propose to create a second method (first_span, like you suggested) that has the implementation from this PR, and make the span method error on ALL parsed attributes (there are a few allowed right now). We should only use this method in very specific places (such as this check) where using it actually makes sense. Would you be ok with this?

r? @jdonszelmann

[rustbot]

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