Set up API to make it possible to pass closures instead of AttributeLint#154432
Set up API to make it possible to pass closures instead of AttributeLint#154432GuillaumeGomez wants to merge 2 commits intorust-lang:mainfrom
AttributeLint#154432Conversation
|
Some changes occurred in compiler/rustc_attr_parsing cc @jdonszelmann, @JonathanBrouwer These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
|
|
This comment has been minimized.
This comment has been minimized.
38ca4a3 to
d408935
Compare
This comment has been minimized.
This comment has been minimized.
d408935 to
5bbb314
Compare
|
Added the missing file and fixed the typo. Time to run a perf check. @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.
Set up API to make it possible to pass closures instead of `AttributeLint`
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (8dcd0b9): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 3.6%, secondary 6.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 494.635s -> 485.284s (-1.89%) |
|
Nice. =D |
This comment has been minimized.
This comment has been minimized.
…Lint`. The end goal being to completely remove `AttributeLint`.
5bbb314 to
08bc9d1
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. |
| pub lint_id: LintId, | ||
| pub id: HirId, | ||
| pub span: Span, | ||
| #[stable_hasher(ignore)] |
There was a problem hiding this comment.
Is this correct? If the lint/id/span of the lint stays the same, but something in the labels changes, then the hash of the DynAttribute would remain unchanged
There was a problem hiding this comment.
I don't see a way around that, whether it's dyn Diagnostic or a callback. Although, do you see a case where a same AttributeLint is emitted on both the same HirId and span but with a different error?
| this: unused_span, | ||
| other: used_span, | ||
| warning: true, | ||
| move |dcx, level| { |
There was a problem hiding this comment.
I don't really like that a closure is needed here, I experimented a bit locally and this seems to be because we store the result in a query, and to call IntoDiagnostic we need ownership of this thing, which the query can't provide.
We could theoretically work around that with Steal but that seems a bit too extreme tool for such a small annoyance
There was a problem hiding this comment.
Maybe taking a diagnostic and requiring it to be Clone solves it?
There was a problem hiding this comment.
To be checked but it's an idea I had in mind, just didn't try it out yet.
There was a problem hiding this comment.
After testing out a bit, I think the current approach is the best one we can have because Box<dyn Diagnostic> is unhappy because dyn Diagnostic is not Sized (obviously) and keeping a &dyn Diagnostic creates more issues than it solves. I think it's (sadly) the best "form" we can have for now.
|
☔ The latest upstream changes (presumably #154637) made this pull request unmergeable. Please resolve the merge conflicts. |
Part of #153099.
This PR sets up the base implementations needed to remove
AttributeLintKindentirely and migrate two variants as examples.r? @JonathanBrouwer