Eagerly fetch typeck results when linting#157928
Conversation
|
cc @rust-lang/clippy |
|
r? @Kivooeo rustbot has assigned @Kivooeo. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Turns out that was load bearing for rustdoc. That's now been worked around and documented properly. |
This comment has been minimized.
This comment has been minimized.
|
@bors try parent=4fc35880d9636c5a2679d18b034038123b09b294 @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Eagerly fetch typeck results when linting
This comment has been minimized.
This comment has been minimized.
|
Error occured while categorizing benchmark run:
|
This comment has been minimized.
This comment has been minimized.
|
@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.
Eagerly fetch typeck results when linting
This comment has been minimized.
This comment has been minimized.
|
@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.
Eagerly fetch typeck results when linting
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (09bb986): comparison URL. Overall result: no relevant changes - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (primary -2.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 510.446s -> 504.716s (-1.12%) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
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. |
|
@rustbot reroll |
| body: BodyId, | ||
| ) -> Option<&'tcx TypeckResults<'tcx>>; | ||
| } | ||
| impl TypeckBody for () { |
There was a problem hiding this comment.
() is kinda confusing. Could you use a more descriptive name and add some comments to this trait definition?
Or just use const parameter like RequiresTypeck: bool on LateContextAndPass?
There was a problem hiding this comment.
A const param would likely be better.
| // being queried. | ||
| // The body and typeck results are also set in `visit_fn`. | ||
| // Only fetch the results if this is for a new body. | ||
| if old_enclosing_body != Some(body_id) { |
There was a problem hiding this comment.
It seems that this is the only place we use enclosing_body. Can this condition ever be false?
And do we need to do this save and restore stuff for typeck_result at all if we eagerly fetch typeck result for every body? 🤔
There was a problem hiding this comment.
Const bodies also flow through here without hitting check_fn.
There was a problem hiding this comment.
Restoring has to happen anytime a nested body occurs. e.g. nested function, closure, const block...
80ae54c to
6883244
Compare
View all comments
The first commit avoids running the lint visitor if all lints were filtered out. In every other case the typeck results are accessed on every body making the delayed load pointless overhead.