MIR required_consts, mentioned_items: ensure we do not forget to fill these lists

[RalfJung]

Bodies initially get created with empty required_consts and mentioned_items, but at some point those should be filled. Make sure we notice when that is forgotten.

[rustbot]

r? @pnkfelix

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

[rustbot]

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[RalfJung]

This currently ICEs for async closures, looks like we do indeed forget to compute these lists there.

@compiler-errors do async closures not go through the normal MIR passes? In that case they have to manually invoke the right passes to compute required_consts and mentioned_items. Where should that be done?

[RalfJung]

Async drop also doesn't seem to compute the required_consts.
Cc @azhogin @zetanumbers

EDIT: the failing test is async-closures/drop.rs, so actually that's probably also just the async closures issue.

[compiler-errors]

Good change 👍

[compiler-errors]

@bors r+

[bors]

📌 Commit 824dd75 has been approved by compiler-errors

It is now in the queue for this repository.

[compiler-errors]

Oops, didn't see this was still failing.

@bors r-

[RalfJung]

Yeah I need your help to fix this I think. :) Async closure MIR doesn't seem to set these lists, any idea where that might be going wrong?
Do async closures not go through the normal MIR passes? In that case they have to manually invoke the right passes to compute required_consts and mentioned_items. Where should that be done?

[compiler-errors]

@compiler-errors do async closures not go through the normal MIR passes?

They should go through normal MIR passes. We copy the body here:

rust/compiler/rustc_mir_transform/src/coroutine/by_move_body.rs

Line 199 in c0e3298

body.coroutine.as_mut().unwrap().by_move_body = Some(by_move_body);

So any passes previously applied should still be okay. Then on subsequent passes, we apply the pass to the by_move_body if it gets applied to the parent body:

rust/compiler/rustc_mir_transform/src/pass_manager.rs

Lines 186 to 190 in 97ac52f

	if let Some(coroutine) = body.coroutine.as_mut() {
	if let Some(by_move_body) = coroutine.by_move_body.as_mut() {
	run_passes_inner(tcx, by_move_body, passes, phase_change, validate_each);
	}
	}

Are we not populating required_consts and mentioned_items with a pass via the pass manager?

[RalfJung]

No those are not normal passes. required_consts usually gets populated very early, in fn mir_promoted. mentioned_items gets populated in fn inner_optimized_mir.

mentioned_items used to be a normal pass but then it doesn't get run on custom MIR that is "injected" after this pass, which is bad...

[RalfJung]

So any passes previously applied should still be okay. Then on subsequent passes, we apply the pass to the by_move_body if it gets applied to the parent body:

Wait so when passes are applied to one body we actually apply them to another? What a glorious hack.^^ But super hard to follow...

[compiler-errors]

Hm. So I guess we could just manually handle the async closure "by move" body manually for both passes, but I fear that we'll just forget to apple some other special pass in the future just like these two.

Wait so when passes are applied to one body we actually apply them to another? What a glorious hack.^^ But super hard to follow...

It's the only way that I saw for us to ensure that we're applying all the same passes to the original async closure and the "by move" body in lockstep, since the latter is generated on the fly really early during mir_build.

[RalfJung]

Not even required_consts are set which are usually set very early in MIR construction, so it also seems like this doesn't go through the normal MIR builder, or so?

The by-move body should just be its proper independent own MIR, and then when a query asks for that MIR it should go through the pass manager itself. Why should it "ride along" the other body?

[compiler-errors]

Perhaps I should just bite the bullet and synthesize real def ids for async closure "by move" bodies so I can just feed mir_built and not worry about all of this 🤔

I guess you can probably do whatever to unblock this, or if you have better ideas I'd love to hear them.

[RalfJung]

Perhaps I should just bite the bullet and synthesize real def ids for async closure "by move" bodies

Yes, I think that's what I meant. Having one DefId refer to two bodies seems like a recipe for confusion. It's already bad enough with const MIR and runtime MIR, let's not add another dimension.^^

[RalfJung]

Though some drop shims also seem to bypass that pass manager, not sure what's going on there...

  thread 'rustc' panicked at compiler/rustc_monomorphize/src/collector.rs:1232:26:
  required_consts for DefId(2:2672 ~ core[1802]::ptr::drop_in_place) have not yet been set

[compiler-errors]

It's arguable to say that drop shim has a proper def id. It uses the def id of drop_in_place, which is definitely wrong because the shim is semi-substituted with a type (stored in the InstanceKind::DropShim) and therefore is parameterized over a random set of generics that's almost certainly not the <T> that the function is generic over. I think you probably already know we have lots of hacks for handling drop shims, lol.

But that's kinda unrelated. I'll look into getting this fixed the right way, but it may be much harder than it seems, so it's up to you if you want to block this PR on that work.

[compiler-errors]

What's luckily different from the drop shim is that the by-move body doesn't need to be generated after the coroutine transformation pass, so we may be lucky and it turns out much simpler than expected :D

[RalfJung]

The first problem is earlier -- the body we see and clone in ByMoveBody already doesnt have its required_consts set. So something is already going wrong before that.

[RalfJung]

Oh wait, is mir_built before mir_promoted? In that case that's expected, I guess.

Even the pass manager for normal MIR is already such a mess, it's near impossible to remember what happens in which order.^^

[RalfJung]

Let's see, hopefully this is enough.

[RalfJung]

It's arguable to say that drop shim has a proper def id. It uses the def id of drop_in_place, which is definitely wrong because the shim is semi-substituted with a type (stored in the InstanceKind::DropShim) and therefore is parameterized over a random set of generics that's almost certainly not the <T> that the function is generic over. I think you probably already know we have lots of hacks for handling drop shims, lol.

But that's kinda unrelated. I'll look into getting this fixed the right way, but it may be much harder than it seems, so it's up to you if you want to block this PR on that work.

It's magic, but knowing that it is drop_in_place plus knowing the type is enough to uniquely identify the MIR... hm but I guess that's an Instance, not an InstanceKind, and usually Body is for an InstanceKind... so okay I agree, that's also a hack.^^ But still less hacky than this IMO, a single mir::Body still represents only a single MIR body and not two.

is parameterized over a random set of generics that's almost certainly not the <T> that the function is generic over

No idea what you mean here. The T in drop_in_place::<T> is what it is parameterized over, how is there a "random set of generics"?

[compiler-errors]

No idea what you mean here.

I'm talking about why we have https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/enum.InstanceKind.html#method.has_polymorphic_mir_body, because the body you get back from InstanceKind::DropShim(_, Some(ty)) is not parameterized over T, but whatever generics show up in ty. But this convo is kinda unrelated now, so if you want to keep talking about how drop shims are cursed then DM me or start a zulip thread or sth :D

[RalfJung]

I think we can agree that drops shims and asyn-by move bodies are both cursed, each in their own unique ways. :)

I hope that what this PR does now works. It's a hack, but we should notice fairly easily when it goes wrong (that's exactly why the first version of the PR ICEd).

[compiler-errors]

Cool. These hacks should become redundant with #128506, but they're totally fine now.

@bors r+

[bors]

📌 Commit 6d312d7 has been approved by compiler-errors

It is now in the queue for this repository.