Remove special-casing for argument patterns in MIR typeck (attempt to fix perf regression of #133858)

[dianne]

See my comment on #133858 for more information. This is just a guess as to what went wrong, and I haven't been able to get the profiler running locally, so I'll need a perf run to make sure this actually helps.

There's one test's stderr that suffers a bit, but this was just papering over the issue anyway. Making region errors point to the correct constraints in the presence of invariance/contravariance is a broader problem; the current way it's handled is mostly based on guesswork, luck, and hoping it works out. Properly handling that (somehow) would improve the test's stderr without the hack that this PR reverts.

[rustbot]

r? @wesleywiser

rustbot has assigned @wesleywiser.
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

[dianne]

I suppose I should maybe cc @rust-lang/wg-compiler-performance since this is following up on a perf regression (sorry for the ping if it's unnecessary!)

[lqd]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 72945be with merge 445852ab3c7ebf27903981a26388ea9877a819e3...

[bors]

☀️ Try build successful - checks-actions
Build commit: 445852a (445852ab3c7ebf27903981a26388ea9877a819e3)

[rust-timer]

Finished benchmarking commit (445852a): 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)	-3.2%	[-17.4%, -0.2%]	15
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.2%	[-17.4%, -0.2%]	15

Max RSS (memory usage)

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

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)	-4.2%	[-6.2%, -2.8%]	3
Improvements ✅ (secondary)	-0.9%	[-0.9%, -0.9%]	1
All ❌✅ (primary)	-4.2%	[-6.2%, -2.8%]	3

Cycles

Results (primary -5.7%)

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)	-5.7%	[-16.9%, -1.6%]	8
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-5.7%	[-16.9%, -1.6%]	8

Binary size

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

Bootstrap: 764.798s -> 766.902s (0.28%)
Artifact size: 325.77 MiB -> 325.80 MiB (0.01%)

[lqd]

Maybe differentiating the assignments generated from lowering argument patterns to where they are lowered instead would still allow to do this diagnostics improvement at a lower cost -- or the opposite, using the same method but on the error path, where we know it's not as perf-sensitive.

[dianne]

Unfortunately, using the same method only on the error path doesn't seem clean to me, at least in the general case. The constraint category is used by best_blame_constraint, which runs during borrowck on the happy path when outlives constraints from closures need to be propagated to whatever body they're defined in. We could maybe track in best_blame_constraint whether we're emitting an error or checking a closure, but we'd risk not blaming a propagated outlives constraint from a closure if it happened to be from an argument pattern. There's already a bit of weirdness with not blaming ideal constraints when they're propagated from closures, so I'd rather not make that more confusing.

Differentiating assignments during lowering would work, I think. I'd looked into putting the information in the assignment's span (as a new kind of desugaring) to avoid needing to store anything extra in the MIR itself. I opted not to go for that though since it looked like keeping track of which assignments came from argument patterns would require an unpleasant amount of bookkeeping during either MIR or THIR construction. It wouldn't be hard, but the added complexity didn't seem worth it to me for such a niche diagnostic tweak¹. I don't think I checked how much bookkeeping it'd be to do elsewhere, though.

Another option would be to give up (for now) on preventing assignments from argument patterns from being blamed, and instead just pick a different name for assignments (like "binding", maybe²) when they're syntactically not from assignment-like syntax. This should be doable entirely on the error path, and would also mean we could avoid calling match expressions "assignments" too if we'd like.

We could also try an entirely different heuristic to hopefully better address the issue this hack was papering over (see the footnote below) but it'd be a substantial enough change that it should have its own PR, and I'm honestly not confident the fix I have in mind would work. There might be some other less-fragile/hacky way of writing best_blame_constraint altogether, but I'm not sure how if so. borrowck as written makes it difficult to tell exactly why region errors occur, hence the need for best_blame_constraint to divine some meaning.

I'm not sure which of these options would be best to pursue, but I think just picking a different word for assignments from argument patterns (and maybe also match expressions) would be easiest and least intrusive.

Footnotes

1. I think the argument pattern is only being blamed here instead of the more relevant assignment in the function body because of how we handle invariance in diagnostics. The main trick best_blame_constraint uses to guess what to blame for region errors breaks down when the regions involved are related contravariantly, and we currently don't have a reliable way of knowing when that's the case. Hence all the heuristics like that one to try and blame something hopefully-reasonable even in those cases. ↩

2. I'm not totally sure on what word would be best. The nice thing about "assignment" is it's very clear about the relationship being described. ↩

[lqd]

Seems that was the cause of the perf regression, thanks!. However, the diagnostics change is unfortunate.

@lcnr, as the reviewer for #133858, this PR reintroduces the diagnostics issue you pointed it in 50222db#r1888290353 do you prefer:

• reverting best_blame_constraint: Blame better constraints when the region graph has cycles from invariance or 'static #133858 to have time to fix the perf regression without the diagnostics regression
• fix the perf regression, with the diagnostics regression, by landing this PR

This PR removes the hack linked that also caused the perf regression, without diagnostics getting too much worse, so it's not the end of the world.

It seems possible to avoid choosing the least bad outcome of the two though, so my personal preference is for the revert now, and reland the changes when both issues are fixed. What is your preference?

[lcnr]

It seems possible to avoid choosing the least bad outcome of the two though, so my personal preference is for the revert now, and reland the changes when both issues are fixed. What is your preference?

seems fine to me, have to admit I don't have an opinion on this 😅

[lcnr]

I also think the diagnostics regression of this pr is small enough to also be totally fine 🤷 would be happy if someone (@dianne :p) were to really dive into mir borrowck diagnostics for a while to really improve/clean them up, but I don't think small changes to the status quo (i.e. the diagnostics regression in this pr) are too impactful either way ^^'

[lqd]

Ok thank you, I’ll ask in todays meeting to check with the others.

[lqd]

https://rust-lang.zulipchat.com/#narrow/channel/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202025-01-09/near/492749840

After discussing in today's t-compiler meeting, we chose to land this perf fix with the small diagnostics regression rather than the revert.

@bors r+

[bors]

📌 Commit 72945be has been approved by lqd

It is now in the queue for this repository.

[bors]

⌛ Testing commit 72945be with merge b93d4b1...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

The job armhf-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

curl: (22) The requested URL returned error: 404
curl: (22) The requested URL returned error: 404
ERROR: failed to download llvm from ci

    HELP: There could be two reasons behind this:
        1) The host triple is not supported for `download-ci-llvm`.
        2) Old builds get deleted after a certain time.
    HELP: In either case, disable `download-ci-llvm` in your config.toml:
    [llvm]
    download-ci-llvm = false
    
Build completed unsuccessfully in 0:00:23

[lqd]

downloading https://ci-artifacts.rust-lang.org/rustc-builds-alt/824759493246ee383beb9cd5ceffa0e15deb9fa4/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz
curl: (22) The requested URL returned error: 404

@bors retry network issue

[Kobzol]

(Turns out it was a skill issue, rather than a network issue, after all.)