-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Recover non-list repr attr with a span for later attr validation #143533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Some changes occurred in compiler/rustc_passes/src/check_attr.rs Some changes occurred in compiler/rustc_attr_parsing |
@@ -2049,12 +2049,6 @@ impl<'tcx> CheckAttrVisitor<'tcx> { | |||
if item.is_some() { | |||
match target { | |||
Target::Struct | Target::Union | Target::Enum => continue, | |||
Target::Fn | Target::Method(_) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was telling us that #[repr]
was actually #[repr(align(..))]
which is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this fix is not included in #143522, we should make sure this does not get lost
| ^^^^^^^^ | ||
| | | ||
| expected this to be a list | ||
| help: must be of the form: `#[repr(C | Rust | align(...) | packed(...) | <integer type> | transparent)]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This help is a bit misleading at the crate level, but seems inevitable unless we are aware of the attr target when we emit this error, which seems unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might soon start emitting warnings like this earlier which regains us that info. Not for this pr but am aware of it :)
@@ -567,32 +576,59 @@ LL | | #[coroutine = 63] || {} | |||
LL | | } | |||
| |_- not a `const fn` | |||
|
|||
error[E0517]: attribute should be applied to a struct, enum, or union |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is:
#[repr]
fn foo() {}
Previously we didn't emit an error at all here. See comment above about it being mistakenly considered like repr(align(..))
, which is fixed by just removing that arm from the ReprEmpty
case.
d7ee65c
to
d668c53
Compare
@compiler-errors I just approved a pr this morning by @JonathanBrouwer which I think conflicts with this one so you might need to do a rebase I'm afraid |
Apart from that these changes look nice, but I'll wait till tomorrow with the r+ since I have a feeling some of the impl needs to change a bit after the other one. Wait let me link it |
That PR just fixes #143522, so I'll close this one. |
Oh awesome, didn't expect the other one to do that much for this issue but I guess it makes sense. okay! |
Fixes #143522
This seems like an obvious solution, but I'm open to other approaches. Pardon the span cascade in the test file, but that's unfortunately a consequence of adding a
#![]
attr to the test :)r? @jdonszelmann