Repository navigation
Don't emit diagnostics during early lint attribute parsing - #164898
JonathanBrouwer wants to merge 5 commits into
Conversation
|
@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.
Don't emit diagnostics during early lint attribute parsing
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (9509e76): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @bors rollup=iffy rustc-perf 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.3%, secondary -0.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 0.4%, secondary 0.5%)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: 487.351s -> 487.992s (0.13%) |
This comment has been minimized.
This comment has been minimized.
`AttrErrorGuaranteed` represents that `ShouldEmit::emit_err` was called, which may or may not represent an actual diagnostic being emitted. This new type is needed to remove the unchecked `ErrorGuaranteed` when emitting `ShouldEmit::NothingNoBug`.
0807391 to
76f6ef4
Compare
|
This needs a @rust-lang/lang FCP. |
|
@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.
Don't emit diagnostics during early lint attribute parsing
This comment has been minimized.
This comment has been minimized.
|
|
||
| #[deny("literal")] //~ ERROR expected a lint, found literal | ||
| //[duplicate]~| ERROR expected a lint, found literal | ||
| //[duplicate]~| ERROR expected a lint, found literal |
There was a problem hiding this comment.
This test needs to be changed
|
Personally I don't like assigning semantics to these attributes pre-expansion, perhaps we could get rid of that so we don't need to parse them at all. Perhaps this could be done by getting rid of pre expansion lints entirely (for rustc, it's just the keywords in new edition lint). Or we could make these lints crate level only, so we need not think about lint attributes on such items. See also #162813 (comment) (cc @petrochenkov) |
|
I agree, I think the easiest thing to do is to make them crate-level only, as that's trivial to implement and preserves the behaviour of the lint which is slightly better pre expansion (tho debatable if that's even worth the implementation complexity) If lang decides to do that I can change this PR to implement that instead (or do it as a follow-up). It would also make the code a lot nicer, as the entire last commit is not needed then :) |
|
☔ The latest upstream changes (presumably #165002) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
This PR makes the compiler no longer emit any errors on pre-expansion lint attributes.
That is, the following code currently doesn't compile, but compiles after this PR:
This was historically hard to fix, but since we ported the lint attributes to the attribute parsing system, this is now relatively easy.
Blocked on #164985 and #164895. This PR is stacked on top of those two changes.
Reviewable commit by commit, every commit passes tests. The first 3 commits are from the stacked PRs.
This PR introduces the
AttrErrorGuaranteedtype, which is similar toErrorGuaranteed.AttrErrorGuaranteedrepresents thatShouldEmit::emit_errwas called, which may or may not represent an actual diagnostic being emitted, because attributes parsed pre-expansion should not emit diagnostics.r? @mejrs