Stop considering moved-out locals when computing auto traits for generators (rebased)

[compiler-errors]

This PR revives #112279. I wanted to reopen this to gauge @cjgillot's thoughts about this, since it's been a while since -Zdrop-tracking-mir has landed.

If this PR looks OK, I can flesh it out and write up an FCP report -- I think this is a T-types FCP, since this has to do with the types that are considered live for auto traits, and doesn't otherwise affect the layout of coroutines.

Open questions:

• "Why do we require storage to be live if the locals is not initialized?" (Stop considering moved-out locals when computing auto traits for generators #112279 (comment))
  • I agree that we should eventually fix the storage analysis for coroutines, but this seems like a problem that can be fixed after we fix witnesses for the purposes of traits here.
  • As far as I could tell, fixing the problem of moved locals for storage would require insertion of additional StorageDead statements, or would require further entangling the type system with the operational semantics in ways that T-opsem seemed uncomfortable with when I asked. cc @RalfJung
  • Relevant: What about: use-after-move and (maybe) use-after-drop unsafe-code-guidelines#188

Fixes #128095
Fixes #94067
Fixes #129325

r? @cjgillot

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[compiler-errors]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 9c519d5 with merge 7093781...

[bors]

☀️ Try build successful - checks-actions
Build commit: 7093781 (709378137e05d0649c71b323e6246f5d48dd8bff)

[rust-timer]

Finished benchmarking commit (7093781): 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-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.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.4%, 0.5%]	2
Regressions ❌ (secondary)	2.8%	[0.8%, 5.6%]	15
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.4%, 0.5%]	2

Max RSS (memory usage)

Results (primary 4.2%, secondary -1.0%)

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)	4.2%	[3.5%, 4.9%]	2
Regressions ❌ (secondary)	2.6%	[2.6%, 2.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-3.1%, -2.4%]	2
All ❌✅ (primary)	4.2%	[3.5%, 4.9%]	2

Cycles

Results (secondary 3.3%)

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)	3.3%	[2.0%, 4.2%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 761.822s -> 762.913s (0.14%)
Artifact size: 337.08 MiB -> 337.07 MiB (-0.00%)

[compiler-errors]

I think we should eat the perf cost of the analysis. I'm happy to write up an FCP report for this if someone would like to actually review the changes.

[cjgillot]

It was initially a design goal to have the witness types and the runtime transform use the same code. To ensure that they don't diverge and that we don't introduce unsoundness.

On this particular change, why can't apply it to the runtime transform too?

[yotamofek]

Your friendly neighborhood spell checker 😁

Suggested change

	/// A local that is live but is not initialized does not need to accounted in auto trait checking.
	/// A local that is live but is not initialized does not need to be accounted for in auto trait checking.

[compiler-errors]

@cjgillot: I'm not totally sure if I'm understanding what you're asking for. 🤔

Are you saying that we shouldn't just be using this for auto trait coroutine witnesses, but also we should use whether a variable is uninitialized to make it so that we don't even store it across await points?

If so, I'm not totally sure that would be sound, given that we don't have a StorageDrop until possibly some other coroutine state... If not, then could you elaborate?

[cjgillot]

What I'm asking is to have the same logic for both the types and runtime.

The core idea behind type witnesses is to list the types which will appear in the runtime representation.

If the types and runtime have the same logic, we can be confident that whatever happens between both analyses, we will have something consistent.

For instance, we could perform the runtime transform after some opts, and be happy with it.

If we have different analyses, I fear we will have to prove each time that we have that consistency, which will be hard.

So my question is: what would it take to make your PR sourd for the runtime transform.

Do I make sense?