Skip if-let-rescope lint unless requested by migration

[dingxiangfei2009]

Tracked by #124085
Related to #131984 (comment)

Given that if-let-rescope is a lint to be enabled globally by an edition migration, there is no point in extracting the precise lint level on the HIR expression. This mitigates the performance regression discovered by the earlier perf-run.

cc @Kobzol @rylev @traviscross I propose a rust-timer run to measure how much performance that we can recover from the mitigation. 🙇

[rustbot]

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[dingxiangfei2009]

r? @Kobzol

[lqd]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 193fe5a with merge 748da1d...

[bors]

☀️ Try build successful - checks-actions
Build commit: 748da1d (748da1d278d92651895457bc7ba20c125a24fa1f)

[rust-timer]

Finished benchmarking commit (748da1d): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-1.9%, -0.2%]	97
Improvements ✅ (secondary)	-1.4%	[-4.0%, -0.1%]	67
All ❌✅ (primary)	-0.7%	[-1.9%, -0.2%]	97

Max RSS (memory usage)

Results (primary 1.2%, secondary -0.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.2%	[1.1%, 1.5%]	3
Regressions ❌ (secondary)	0.6%	[0.5%, 1.0%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.2%	[-4.2%, -4.2%]	1
All ❌✅ (primary)	1.2%	[1.1%, 1.5%]	3

Cycles

Results (primary -0.9%, secondary -2.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.9%	[-0.9%, -0.9%]	1
Improvements ✅ (secondary)	-2.2%	[-2.4%, -2.1%]	3
All ❌✅ (primary)	-0.9%	[-0.9%, -0.9%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 779.813s -> 778.613s (-0.15%)
Artifact size: 335.27 MiB -> 335.46 MiB (0.06%)

[dingxiangfei2009]

Looks like a GREAT SUCCESS to me.

[Kobzol]

Looks great indeed :) I assume that there are tests that check that the lint does in fact fire during the edition migration?

[dingxiangfei2009]

There is a test called lint-if-let-rescope-gated.rs but to be honest it is still a test based on edition flag. I am still figuring out the best way to tell with certainty that a migration is run. So far I think there is no such thing yet.

A question to the t-compiler is, do we need a flag for this purpose? To be honest, only edition migration lints would be the potential users. Potentially lints for the match ergonomics and the async closures can benefit from the flag as well, because they are technically not executed outside the migration scenario.

[dingxiangfei2009]

r? @compiler-errors

It looks like we need a look from someone on t-compiler to see this makes sense. Shall we?

[compiler-errors]

I'm confused. Why do we need to manually call this here? Shouldn't lints automatically be filtering using lints_that_dont_need_to_run? Isn't that the whole point of #125116?

I think we just need to remove the lint_level_at_node call, right?

[compiler-errors]

Do we even need this call to cx.tcx.lints_that_dont_need_to_run(())? Aren't lints already automatically being filtered by the late lint infrastructure?

[dingxiangfei2009]

Apparently it is not, at least when I instrumented the fn check_expr the log suggests that this lint was still called, which explains why the instruction count has increased significantly. I am looking for time to sit down and investigate why #125116 did not stop this lint from running.