Move check_rustc_pub_transparent into the attribute parser#158496
Move check_rustc_pub_transparent into the attribute parser#158496obeis wants to merge 3 commits into
check_rustc_pub_transparent into the attribute parser#158496Conversation
|
Some changes occurred in compiler/rustc_attr_parsing cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
4756680 to
36e0436
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. |
36e0436 to
a5834bd
Compare
| .filter_map(|attr| attr.args.as_ref()?.as_list()) | ||
| .flat_map(|list| list.mixed()) | ||
| .filter_map(|item| item.meta_item()) | ||
| .any(|meta| meta.path().word_is(sym::transparent)) |
There was a problem hiding this comment.
Hmmm I don't really like that we're duplicating some of the Repr parsing logic here.
How hard would it be to get access to the parsed repr attribute here?
There was a problem hiding this comment.
I see two ways to handle this:
- Extract a helper function:that holds the iteration logic currently inlined in
lint_helpers.rs. I'd move it next to the repr parser inrepr.rsso the logic lives in one place. - Add the parsed attributes to
FinalizeContext, so we can match onAttributeKind::Reprdirectly instead of re-walking the raw attribute args.
I think the second approach is much cleaner, since it avoids re-parsing entirely.
There was a problem hiding this comment.
I prefer #2 strongly but the parsed attributes are only available after finalization, I think it's possible to work around that tho
|
Reminder, once the PR becomes ready for a review, use |
the parsed attributes of an item were only fully populated after all finalizers had run, so `finalize_check` (run during finalization) could only inspect attribute *paths* via `FinalizeContext::all_attrs`, not the parsed `AttributeKind`s. split finalization into two passes: first run all finalizers to produce the parsed attributes, then run the cross-attribute checks. The checks are deferred via a new `AttributeParser::deferred_finalize_check`, and can now inspect the fully parsed attributes through a new `FinalizeContext::parsed_attrs` field.
the check that `#[rustc_pub_transparent]` is only applied to `#[repr(transparent)]` types needs to see the parsed `Repr` attribute, so it now lives in `RustcPubTransparentParser::finalize_check`, using the freshly available `FinalizeContext::parsed_attrs`.
a5834bd to
b6b2e9c
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Move `check_rustc_pub_transparent` into the attribute parser
| // inspect the fully parsed attributes via `FinalizeContext::parsed_attrs`. | ||
| for (check, attr_span) in deferred_checks { | ||
| check( | ||
| &FinalizeContext { |
There was a problem hiding this comment.
I think it doesn't make sense for check to use the FinalizeContext.
Because of this choice, in the code above you have to lie and give an &[].
Can you make a new context for this?
There was a problem hiding this comment.
I added the new context.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (1e998ba): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.8%, secondary 2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 57.6%, secondary -0.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 490.513s -> 491.871s (0.28%) |
|
Perf result looks like a lot of noise, the libc improvements might be real but no clue why they would improve? |
|
☔ The latest upstream changes (presumably #159444) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
Updates #153101
Verify the
#[repr(transparent)]requirement inRustcPubTransparentParser::finalize_check, replacingcheck_rustc_pub_transparentinrustc_passes.r? @JonathanBrouwer