Integrate attributes as part of the crate hash #83901
Conversation
rust-highfive
added
the
S-waiting-on-review
|
cc @rust-lang/wg-incr-comp: I'd like someone else to look this over as well. |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit a9aa397ed83b9f5c801984389ea35a14f711e0f5 with merge 22900b6fe4bfd5fc30cb590708b3dd6eed766118... |
|
☀️ Try build successful - checks-actions |
|
Queued 22900b6fe4bfd5fc30cb590708b3dd6eed766118 with parent d322385, future comparison URL. |
|
Finished benchmarking try commit (22900b6fe4bfd5fc30cb590708b3dd6eed766118): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
|
LGTM r? @wesleywiser for a final review, since they approved #79519 |
|
Perf results look slightly negative overall with a few small wins. Is that expected? |
|
Yes - we're now doing necessary work (hashing attributes) that we weren't doing before. This leads to issues like #83887, where we don't re-compute a query on a foreign crate because the crate hash was unchanged. |
|
The regressions should probably be interpreted as #79519 being slightly less of a performance win than it initially appeared. I'm not sure what to make of the performance gains in this PR. |
|
@wesleywiser: Does this look good to you? It would be really good to get this merged, since it fixes some ICEs with incremental compilation. |
|
@Aaron1011 thanks for the reminder! I will review tomorrow. |
|
|
|
@bors r=wesleywiser |
|
📌 Commit 1891da0 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Visited for perf triage, there was a regression, seems roughly consistent with the results that we got from the perf run on the PR, so this regression was expected outcome. |
…henkov Fix unused attributes on macro_rules. The `unused_attributes` lint wasn't firing on attributes of `macro_rules` definitions. The consequence is that many attributes are silently ignored on `macro_rules`. The reason is that `unused_attributes` is a late-lint pass, and only has access to the HIR, which does not have macro_rules definitions. My solution here is to change `non_exported_macro_attrs` to be `macro_attrs` (a list of all attributes used for `macro_rules`, instead of just those for `macro_export`), and then to check this list in the `unused_attributes` lint. There are a number of alternate approaches, but this seemed the most reliable and least invasive. I am open to completely different approaches, though. One concern is that I don't fully understand the implications of extending `non_exported_macro_attrs` to include non-exported macros. That list was originally added in rust-lang#62042 to handle stability attributes, so I suspect it was just an optimization since that was all that was needed. It was later extended to be included in SVH in rust-lang#83901. rust-lang#80641 also added a use to check for `invalid` attributes, which seems a little odd to me (it didn't validate non-exported macros, and seems highly specific). Overall, there doesn't seem to be a clear story of when `unused_attributes` should be used versus an error like E0518. I considered alternatively using an "allow list" of built-in attributes that can be used on macro_rules (allow, warn, deny, forbid, cfg, cfg_attr, macro_export, deprecated, doc), but I feel like that could be a pain to maintain. Some built-in attributes already present hard-errors when used with macro_rules. These are each hard-coded in various places: - `derive` - `test` and `bench` - `proc_macro` and `proc_macro_derive` - `inline` - `global_allocator` The primary motivation is that I sometimes see people use `#[macro_use]` in front of `macro_rules`, which indicates there is some confusion out there (evident that there was even a case of it in rustc).
pnkfelix
added
beta-nominated
|
discussed in steering meeting about incr-comp fingerprints. beta backport approved. |
pnkfelix
added
the
beta-accepted
Mark-Simulacrum
removed
the
beta-nominated
|
Do we have a test for this PR? It doesn't look like it backports cleanly (the crate_hash function was added after beta branched) |
Mark-Simulacrum
added
the
beta-nominated
|
No test were written at the time. I am not sure how to make one, #83887 had no MCVE. |
|
If you can, that'd be great. Happy to r+ that pretty quickly; it should target the current beta branch. Let me know if I should expect that by Monday -- otherwise I will invest effort over the weekend myself. |
…lacrum Integrate attributes as part of the crate hash Backport of rust-lang#83901 r? `@Mark-Simulacrum`
Mark-Simulacrum
removed
the
beta-nominated
r? @Aaron1011