Better error for HRTB error from generator interior

[jackh726]

cc #100013

This is just a first pass at an error. It could be better, and shouldn't really be emitted in the first place. But this is better than what was being emitted before.

[rust-highfive]

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

[jackh726]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit b47f3458e8b1859ea9d15dbe3fd1e7850ac34b4f with merge d24c3df4273af3abc5a386905c589339b1b8477e...

[bors]

☀️ Try build successful - checks-actions
Build commit: d24c3df4273af3abc5a386905c589339b1b8477e (d24c3df4273af3abc5a386905c589339b1b8477e)

[rust-timer]

Queued d24c3df4273af3abc5a386905c589339b1b8477e with parent 194140b, future comparison URL.

[rust-timer]

Finished benchmarking commit (d24c3df4273af3abc5a386905c589339b1b8477e): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	0.2%	[0.2%, 0.3%]	5
Regressions ❌ (secondary)	5.7%	[4.3%, 7.4%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.2%, 0.3%]	5

Max RSS (memory usage)

Results

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.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.9%	[2.0%, 4.1%]	17
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

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.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.4%	[3.6%, 5.1%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[jackh726]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 1c10a919f87a6a230239853a038a2d1abe74d484 with merge d7e1dc0229298b3f13ad956805b1db04248cf8f4...

[bors]

☀️ Try build successful - checks-actions
Build commit: d7e1dc0229298b3f13ad956805b1db04248cf8f4 (d7e1dc0229298b3f13ad956805b1db04248cf8f4)

[rust-timer]

Queued d7e1dc0229298b3f13ad956805b1db04248cf8f4 with parent a24a020, future comparison URL.

[rust-timer]

Finished benchmarking commit (d7e1dc0229298b3f13ad956805b1db04248cf8f4): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	0.2%	[0.2%, 0.3%]	4
Regressions ❌ (secondary)	3.3%	[2.5%, 4.2%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.2%, 0.3%]	4

Max RSS (memory usage)

Results

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.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.8%	[2.8%, 2.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

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.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.6%	[2.1%, 5.4%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.0%	[-3.0%, -3.0%]	1
All ❌✅ (primary)	-	-	0

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[jackh726]

I think that's likely as good as we'll get here. We can no longer deduplicate as many types (and have to do two folds), so it's strictly more work.

Given that this regression only happens in a single secondary benchmark, and the above statement, I'm going to mark the perf regression as triaged.

[cjgillot]

TBH, I'm not really fond of adding a span to each BrAnon. It feels a bit wrong to have spans inside Ty data structures. This may become a bit of the footgun when comparing bound types to one another.

Why doesn't the reported bug cause an error before region erasure?
Should we try to prove WF(Option<I::Future<'_, '_>>) in order to trigger this kind of error earlier?

[jackh726]

TBH, I'm not really fond of adding a span to each BrAnon. It feels a bit wrong to have spans inside Ty data structures. This may become a bit of the footgun when comparing bound types to one another.

I considered giving then an optional DefId instead, but that doesn't quite work, because we create them so late. Also can't store them in a side table somewhere, since they aren't really "unique" enough for that.

Why doesn't the reported bug cause an error before region erasure? Should we try to prove WF(Option<I::Future<'_, '_>>) in order to trigger this kind of error earlier?

This is an interesting thought I hadn't considered. Thinking a bit about this, unless we construct that obligation in just the right way, it's going to be hard to trigger (and even then...). We specifically get this error because these are late-bound vars, that get turned into placeholders, that we normalize, and register a constraint for, that gets solved during borrowck. This is specifically difficult, because it's not the type being WF that's the problem, but the type being live across the await point.

I don't think I'm doing a good job putting this into words. But essentially, it sounds reasonable, but I don't think it will actually work. Either because we don't know that we'll emit this error (because we don't know it will live across an await point), because we can't really construct an obligation that will emit the error but not run into the same problem (since we can't e.g. use a BrNamed instead, since we don't have a DefId), or both.

[cjgillot]

Looking a bit more to the test case, the error is a quirk of the way we bind lifetimes in the generator witness types.
Can I conclude that this PR will be reverted once we get some kind of implied bounds in binders?
Then, should we mention in the error that this is a known limitation due to the way we bind those lifetimes?

[jackh726]

Yes, this PR should be able to be reverted once we no longer error for this. And that is likely going to come from implied bounds here. I can mention that this is a bug.

[cjgillot]

In that case, r=me after rebase.
Thanks you for going through the boilerplate here.

[jackh726]

@bors r=cjgillot

[bors]

📌 Commit 3c71faf has been approved by cjgillot

It is now in the queue for this repository.

[bors]

⌛ Testing commit 3c71faf with merge bc2504a...

[bors]

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing bc2504a to master...

[rust-timer]

Finished benchmarking commit (bc2504a): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.6%	[1.3%, 2.1%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

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.2%, 1.2%]	1
Regressions ❌ (secondary)	2.6%	[2.6%, 2.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	2
All ❌✅ (primary)	1.2%	[1.2%, 1.2%]	1

Cycles

Results

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)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

[nnethercote]

Is probably noise.

@rustbot label: +perf-regression-triaged