Implement argument-dependent target checking for the #[repr] parser#157215
Implement argument-dependent target checking for the #[repr] parser#157215JonathanBrouwer wants to merge 5 commits into
#[repr] parser#157215Conversation
|
Some changes occurred in compiler/rustc_passes/src/check_attr.rs Some changes occurred in compiler/rustc_attr_parsing |
rustbot
added
A-attributes
JonathanBrouwer
force-pushed
the
repr-target-checking3
branch
from
7758293 to
d58ea4a
Compare
|
I like this approach of using ManuallyChecked and asserting whether it was! Good solution :3 |
There was a problem hiding this comment.
Ugh, #[repr] gives me a headache.
Anyway, the implementation looks good generally.
What is the plan going forward? I imagine a series of prs like:
- macrocall checking for the other attrs in #156499
- some kind of "this attribute does nothing even if this macro call expands into a valid target" note on the warning
- a PR to raise the lint level to deny by default for repr attrs on macro calls and do something about the
repr(simd)thing
| @@ -1,8 +1,10 @@ | |||
| #### Note: this error code is no longer emitted by the compiler. | |||
|
|
|||
There was a problem hiding this comment.
Same with the other PR, please direct to the new error code.
There was a problem hiding this comment.
There is no new error code, the new diagnostic doesn't have one :p.
Added a comment saying this
| let (reprs, _first_attr_span) = | ||
| find_attr!(attrs, Repr { reprs, first_span } => (reprs.as_slice(), Some(*first_span))) | ||
| .unwrap_or((&[], None)); | ||
|
|
There was a problem hiding this comment.
| let (reprs, _first_attr_span) = | |
| find_attr!(attrs, Repr { reprs, first_span } => (reprs.as_slice(), Some(*first_span))) | |
| .unwrap_or((&[], None)); | |
| let reprs = find_attr!(attrs, Repr { reprs, .. } => reprs.as_slice()).unwrap_or(&[]); |
Also do we still need to keep first_span in the AttributeKind::Repr?
There was a problem hiding this comment.
Yes we do sadly :c
It is still used for a few diagnostics later, I might try moving those in a followup PR
This comment has been minimized.
This comment has been minimized.
|
(still need to actually push my changes that fixed your review comments, blocked on #157291) |
The first one was already my plan indeed, the other two also sound like great ideas :) |
…cking, r=mejrs Clean up attribute target checking diagnostics Split off from me trying to process your review comments in rust-lang#157215 Thanks to @GuillaumeGomez for making this possible <3 r? @mejrs
mejrs
added
S-waiting-on-author
Rollup merge of #157291 - JonathanBrouwer:cleanup-target-checking, r=mejrs Clean up attribute target checking diagnostics Split off from me trying to process your review comments in #157215 Thanks to @GuillaumeGomez for making this possible <3 r? @mejrs
JonathanBrouwer
force-pushed
the
repr-target-checking3
branch
from
d58ea4a to
2a8fee0
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
| Warn(Target::MacroCall), | ||
| ]), // FIXME: This is not feature gated (!!) |
There was a problem hiding this comment.
one line off, but this fixme should be removed soon anyway 🤷
4 participants
Fixes some of #156499
Fixes some of #153101
Alternative approach to #156569, which was suggested in the comments here: #156569 (review)
How do you feel about this approach?
r? @mejrs