Move attribute safety checking to attribute parsing #149524
Conversation
|
Some changes occurred in compiler/rustc_attr_parsing |
rustbot
added
A-attributes
This comment has been minimized.
This comment has been minimized.
|
:c |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
☔ The latest upstream changes (presumably #147634) made this pull request unmergeable. Please resolve the merge conflicts. |
JonathanBrouwer
force-pushed
the
move_attr_safety
branch
from
987d5fe to
0b3e60d
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. |
| #[rustc_clean(cfg = "cfail5")] | ||
| #[rustc_clean(cfg = "cfail6")] | ||
| #[no_mangle] | ||
| #[unsafe(no_mangle)] |
There was a problem hiding this comment.
The extra unsafe is necessary because without it these tests produces a lint, meaning fail2 and cfail5 are not clean. This was not previously a problem because the lint was emitted early, while it is now stored in the hir.
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
@rustbot ready |
JonathanBrouwer
force-pushed
the
move_attr_safety
branch
from
0b3e60d to
1d204fc
Compare
rustbot
removed
the
S-waiting-on-author
rustbot
added
the
S-waiting-on-review
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
Rollup of 9 pull requests Successful merges: - #147224 (Emscripten: Turn wasm-eh on by default) - #149405 (Recover on misspelled item keyword) - #149443 (Tidying up UI tests [6/N]) - #149524 (Move attribute safety checking to attribute parsing) - #149593 (powf, powi: point out SNaN non-determinism) - #149605 (Use branch name instead of HEAD when unshallowing) - #149612 (Apply the `bors` environment also to the `outcome` job) - #149623 (Don't require a normal tool build of clippy/rustfmt when running their test steps) - #149627 (Point to the item that is incorrectly annotated with `#[diagnostic::on_const]`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #149524 - JonathanBrouwer:move_attr_safety, r=jdonszelmann Move attribute safety checking to attribute parsing This PR moves attribute safety checking to be done during attribute parsing. The `cfg` and `cfg_attr` attribute no longer need special-cased safety checking, yay! This PR is a part 1 of 2, in the second part I'd like to define attribute safety in the attribute parsers rather than getting the information from BUILTIN_ATTRIBUTE_MAP, but to keep PRs reviewable lets do that separately. Fixes #148453 by reordering the diagnostics. The "cannot find attribute" diagnostic now appears first, but both diagnostics still appear. r? `@jdonszelmann`
Rollup of 9 pull requests Successful merges: - rust-lang/rust#147224 (Emscripten: Turn wasm-eh on by default) - rust-lang/rust#149405 (Recover on misspelled item keyword) - rust-lang/rust#149443 (Tidying up UI tests [6/N]) - rust-lang/rust#149524 (Move attribute safety checking to attribute parsing) - rust-lang/rust#149593 (powf, powi: point out SNaN non-determinism) - rust-lang/rust#149605 (Use branch name instead of HEAD when unshallowing) - rust-lang/rust#149612 (Apply the `bors` environment also to the `outcome` job) - rust-lang/rust#149623 (Don't require a normal tool build of clippy/rustfmt when running their test steps) - rust-lang/rust#149627 (Point to the item that is incorrectly annotated with `#[diagnostic::on_const]`) r? `@ghost` `@rustbot` modify labels: rollup
|
perf triage: Hey, looks like this PR caused a regression in #149646, is this expected/justified? Should we revert? perf results for this PR: #149646 (comment) |
|
This seems to be specifically a regression for incremental. My guess is this is caused by the lints now being stashed in the HIR, which is hashed for incremental. |
|
The regression is caused by that we now lower spans of attribute paths. We didn't do this before and not doing this is wrong. |
|
well actually |
|
this might be a real regression haha. take a look at this pr: #148863. it seems that lowering spans simply isn't always worth it weirdly |
|
still trying to come up with a good strategy, but lowering spans takes time, sometimes more than just recompiling the thing. I actually deliberately skipped attr paths in that pr since so many spans in attributes aren't lowered (like whole tokenstreams worth of them) that lowering them would be a giant regression haha |
|
cc: @oli-obk I guess we need to come up with a policy here: do we want to prefer correctness in incremental and lower all spans we can find regardless of the runtime cost or do we want to look for places not to lower spans for some perf wins haha |
|
I guess the real solution is to make span lowering faster. I'll look into it. |
6 participants
This PR moves attribute safety checking to be done during attribute parsing. The
cfgandcfg_attrattribute no longer need special-cased safety checking, yay!This PR is a part 1 of 2, in the second part I'd like to define attribute safety in the attribute parsers rather than getting the information from BUILTIN_ATTRIBUTE_MAP, but to keep PRs reviewable lets do that separately.
Fixes #148453 by reordering the diagnostics. The "cannot find attribute" diagnostic now appears first, but both diagnostics still appear.
r? @jdonszelmann