Check for MSRV attributes in late passes using the HIR #13821
Conversation
rustbot
added
the
S-waiting-on-review
blyxyas
added
the
performance-project
Alexendoo
force-pushed
the
late-pass-msrv-rework
branch
from
0b1f5c8 to
507e6f0
Compare
|
☔ The latest upstream changes (presumably 1dddeab) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Generally looks good to me. I still have some comments (and questions), though not all necessarily blockers
Also, not really related to this PR, but is there anywhere to read more about specifics of incremental lints that you mentioned? (I'm curious how that will work e.g. for some of the lints that keep state across modules or delay their warnings)
| pass: &'a str, | ||
| pass: Pass, |
| if self.msrv.current().is_none() { | ||
| if self.msrv.current(cx).is_none() { | ||
| // If there is no MSRV, then no need to check anything... | ||
| return; | ||
| } |
There was a problem hiding this comment.
It's probably worth delaying this check to run after some of the HIR tree checks now that it potentially does a lot more work? (Relevant in a few more places throughout the modified files where this is the first check in e.g. check_expr but I'm trying to keep it in one place).
I don't know of any use case for actually using these outside of the test suite, but for projects that do use this, it seems like that could get pretty expensive to iterate parent nodes for every expression in the common path
| /// `#[clippy::msrv]` attributes are rarely used outside of Clippy's test suite, as a basic | ||
| /// optimization we can skip traversing the HIR in [`Msrv::meets`] if we never saw an MSRV attribute | ||
| /// during the early lint passes |
There was a problem hiding this comment.
Could this break if the user allows all early lints (eg -Aclippy::all -Wclippy::some_lint) and all early lint passes get filtered out such that there is nothing to update the static? If I remember right only late lint passes are filtered out, so I guess not (at the moment), but I don't know if that could break in the future?
|
I've been working on getting this benchmarks to be done & analyzing them for about 2 days. Here's the Everything perf-wise seems correct, the optimization is working as expected! |
Closes #13169
Late lints now use a parent iter to check for
#[clippy::msrv]attributes instead of keeping track withextract_msrv_attr. This is required for incremental lints since they run per module instead of per crate so don't visit all the necessary attributesAs a basic optimisation if no
#[clippy::msrv]attributes are discovered in early passes the HIR access is skipped completely and just the configured MSRV is used, for most code bases this will be the casechangelog: none