Rewrite inline attribute parser to use new infrastructure and improve diagnostics for all parsed attributes
#138165
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
A-attributes
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
jdonszelmann
commented
Mar 8, 2025
| @@ -584,7 +546,6 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs { | |||
| return OptimizeAttr::Default; | |||
| }; | |||
|
|
|||
| inline_span = Some(attr.span()); | |||
There was a problem hiding this comment.
This fixes #137950
jdonszelmann
force-pushed
the
inline
branch
2 times, most recently
from
e6dad01 to
2a707bf
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #138302) made this pull request unmergeable. Please resolve the merge conflicts. |
oli-obk
added
S-waiting-on-author
jdonszelmann
force-pushed
the
inline
branch
2 times, most recently
from
b7dda57 to
88a713a
Compare
This comment has been minimized.
This comment has been minimized.
|
@rustbot review |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
ah shit, broke clippy 😭 |
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 27733d4 (parent) -> 1bb3352 (this PR) Test differencesShow 284 test diffsStage 1
Stage 2
(and 184 additional test diffs) Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 1bb335244c311a07cee165c28c553c869e6f64a9 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Yay! |
This was referenced Jun 18, 2025
nnethercote
reviewed
Jun 18, 2025
| // Latest feature: rustdoc JSON: Don't apply #[repr] privacy heuristics | ||
| pub const FORMAT_VERSION: u32 = 46; | ||
| // Latest feature: Pretty printing of inline attributes changed | ||
| pub const FORMAT_VERSION: u32 = 48; |
There was a problem hiding this comment.
Uh, we skipped 47. Oh well, not much to be done about it now.
There was a problem hiding this comment.
@GuillaumeGomez: when you add the (tidy?) check for updating the number and comment in tandem, could you also add a check that the number is being increased by one?
There was a problem hiding this comment.
I opened #142677 for that. But that's a good point. Added to my TODO list.
There was a problem hiding this comment.
#138165 (comment), earlier it was thaught that someone else was going to do 46->47, to this should do 47->48. I think the takeaways are:
a) Don't try to be too clever and do a double bounce
b) #142601 should fix this. 🤞
There was a problem hiding this comment.
I've published rustdoc-types = "0.48.0", with a note that 0.47.0/format_version=47 will never exist. https://github.com/rust-lang/rustdoc-types/blob/trunk/CHANGELOG.md#v0.48.0
There was a problem hiding this comment.
I've published rustdoc-types = "0.48.0", with a note that 0.47.0/format_version=47 will never exist. https://github.com/rust-lang/rustdoc-types/blob/trunk/CHANGELOG.md#v0.48.0
|
Finished benchmarking commit (1bb3352): comparison URL. Overall result: ❌ regressions - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression 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 -1.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 4.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.1%, secondary 0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 693.071s -> 692.01s (-0.15%) |
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
Jun 19, 2025
rewrite `optimize` attribute to use new attribute parsing infrastructure r? `@oli-obk` I'm afraid we'll get quite a few of these PRs in the future. If we get a lot of trivial changes I'll start merging multiple into one PR. They should be easy to review :) Waiting on rust-lang#138165 first
tgross35
added a commit
to tgross35/rust
that referenced
this pull request
Jun 20, 2025
rewrite `optimize` attribute to use new attribute parsing infrastructure r? ``@oli-obk`` I'm afraid we'll get quite a few of these PRs in the future. If we get a lot of trivial changes I'll start merging multiple into one PR. They should be easy to review :) Waiting on rust-lang#138165 first
tgross35
added a commit
to tgross35/rust
that referenced
this pull request
Jun 20, 2025
rewrite `optimize` attribute to use new attribute parsing infrastructure r? ```@oli-obk``` I'm afraid we'll get quite a few of these PRs in the future. If we get a lot of trivial changes I'll start merging multiple into one PR. They should be easy to review :) Waiting on rust-lang#138165 first
|
@jdonszelmann Is the binary size jump expected? I thought this would only change the infrastructure but not actual inlining? |
|
@oli-obk and I have been looking at this this morning. It has to do with attribute encoding cross crate and I'm working on a fix. The fix is luckily rather simple, but it was definitely an unexpected side effect. I expect to be busy for an hour or so more and then have a pr ready :) |
bors
added a commit
that referenced
this pull request
Jun 20, 2025
Encode hir attributes cross-crate properly r? `@oli-obk` This should return the lost perf in #138165 cc: `@therealprof`
rust-timer
added a commit
that referenced
this pull request
Jun 20, 2025
Rollup merge of #138291 - jdonszelmann:optimize-attr, r=oli-obk rewrite `optimize` attribute to use new attribute parsing infrastructure r? ```@oli-obk``` I'm afraid we'll get quite a few of these PRs in the future. If we get a lot of trivial changes I'll start merging multiple into one PR. They should be easy to review :) Waiting on #138165 first
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
r? @oli-obk
This PR:
Builds on top of #138164
Closes #137950