Fix region issues when using inline const block

[nbdd0121]

Fixes #78174
Fixes #81857

r? @oli-obk

[RalfJung]

This doesn't seem right... once we erased regions, we shouldn't un-erase them. This can go wrong way too easily.

[nbdd0121]

The code above gets the type from tcx.typeck(def_id).node_type(hir_id). The type retrieved has their lifetimes erased by rustc_typeck::check::writeback already.

[oli-obk]

I believe there is an underlying issue and this PR treats the symptoms. We should look into addressing the underlying issue.

The ICE occurs in

rust/compiler/rustc_mir/src/borrow_check/universal_regions.rs

Line 654 in 4fdac23

let ty = indices.fold_to_region_vids(tcx, ty);

which never had to handle erased regions, because constants only ever had regions if they were associated consts, and then the regions were in the signature.

In

rust/compiler/rustc_mir/src/borrow_check/universal_regions.rs

Lines 422 to 433 in 4fdac23

	// If this is a closure or generator, then the late-bound regions from the enclosing
	// function are actually external regions to us. For example, here, 'a is not local
	// to the closure c (although it is local to the fn foo):
	// fn foo<'a>() {
	// let c = || { let x: &'a u32 = ...; }
	// }
	if self.mir_def.did.to_def_id() != closure_base_def_id {
	self.infcx
	.replace_late_bound_regions_with_nll_infer_vars(self.mir_def.did, &mut indices)
	}
	let bound_inputs_and_output = self.compute_inputs_and_output(&indices, defining_ty);

you can see that some preprocessing happens for closures, which have similar problems. The return type may contain lifetimes that are within the parent function's body.

We need something similar for anon consts.

The reason we can't just throw around static lifetimes is that you may have types like let x: fn(&'a) = const { .... }; which captures lifetime args of the current function. If you make everything static in the anon const you either get errors (as whatever you are passing in has a shorter lifetime), or unsoundness (silent conversion of a short lifetime to a static lifetime)

[lcnr]

the underlying issue here should also get fixed by typechecking inline consts together with their parent, see https://rust-lang.zulipchat.com/#narrow/stream/260443-project-const-generics/topic/inline.20consts.20typeck

this will cause us to modify fn closure_base_def_id to also return the parent of anon consts (and renaming it) and will probably just magically do what @oli-obk is pointing out

[nbdd0121]

I believe there is an underlying issue and this PR treats the symptoms. We should look into addressing the underlying issue.

The ICE occurs in

rust/compiler/rustc_mir/src/borrow_check/universal_regions.rs

Line 654 in 4fdac23

let ty = indices.fold_to_region_vids(tcx, ty);

which never had to handle erased regions, because constants only ever had regions if they were associated consts, and then the regions were in the signature.
In

rust/compiler/rustc_mir/src/borrow_check/universal_regions.rs

Lines 422 to 433 in 4fdac23

	// If this is a closure or generator, then the late-bound regions from the enclosing
	// function are actually external regions to us. For example, here, 'a is not local
	// to the closure c (although it is local to the fn foo):
	// fn foo<'a>() {
	// let c = || { let x: &'a u32 = ...; }
	// }
	if self.mir_def.did.to_def_id() != closure_base_def_id {
	self.infcx
	.replace_late_bound_regions_with_nll_infer_vars(self.mir_def.did, &mut indices)
	}
	let bound_inputs_and_output = self.compute_inputs_and_output(&indices, defining_ty);

you can see that some preprocessing happens for closures, which have similar problems. The return type may contain lifetimes that are within the parent function's body.
We need something similar for anon consts.

The direct cause is

rust/compiler/rustc_mir/src/borrow_check/universal_regions.rs

Line 653 in 4fdac23

let ty = tcx.type_of(self.mir_def.def_id_for_type_of());

. type_of in turn retrieves type with tcx.typeck(def_id).node_type(hir_id) which gives a Ty with lifetime erased.

The reason we can't just throw around static lifetimes is that you may have types like let x: fn(&'a) = const { .... }; which captures lifetime args of the current function. If you make everything static in the anon const you either get errors (as whatever you are passing in has a shorter lifetime), or unsoundness (silent conversion of a short lifetime to a static lifetime)

The example you give currently doesn't work since inline consts are not typechecked with the enclosing body, so inference wouldn't work across const {...}. The ultimate fix is to obviously to typecheck inline consts with the enclosing body, but that'll be a larger scale change.

I do think the fold_region un-erase lifetime part is "fixing the symptom", but I think the rest (erasing lifetime of from_anon_const during THIR building) are correct. I believe this is a reasonable temporary fix for the ICEs until we get the typechecks.

[RalfJung]

I don't think un-erasing is a reasonable fix even short-term -- the risk of unsoundness just seems too high to me.

The rest of the changes I have no strong opinion on.

[oli-obk]

I think the rest (erasing lifetime of from_anon_const during THIR building) are correct

But aren't these erasings unnecessary long term? Lifetimes will get erased during mir borrowck anyway.

I am not comfortable merging this PR as it does things that will ultimately need to be undone in order to start on a fix and conveys a wrong sense of "it works" to users of the feature gate.

[nbdd0121]

But aren't these erasings unnecessary long term? Lifetimes will get erased during mir borrowck anyway.

rust/compiler/rustc_mir_build/src/build/mod.rs

Lines 223 to 234 in 4fdac23

	// The borrow checker will replace all the regions here with its own
	// inference variables. There's no point having non-erased regions here.
	// The exception is `body.user_type_annotations`, which is used unmodified
	// by borrow checking.
	debug_assert!(
	!(body.local_decls.has_free_regions()
	|| body.basic_blocks().has_free_regions()
	|| body.var_debug_info.has_free_regions()
	|| body.yield_ty().has_free_regions()),
	"Unexpected free regions in MIR: {:?}",
	body,
	);

The MIR builder has assertions here which complains if there are any regions. We can either

• Erase regions during THIR building (as this PR current does)
• Remove the assertion here

I prefer the first option since the assertion could catch other cases where lifetime shouldn't slip through, and it'll ensure code handling MIR wouldn't have to deal with non-erased regions.

[oli-obk]

The MIR builder has assertions here which complains if there are any regions

why haven't we hit that assertion before? The assertion from the issues happens in mir borrowck, so we do get that far

[nbdd0121]

why haven't we hit that assertion before? The assertion from the issues happens in mir borrowck, so we do get that far

Because currently type_of returns an erased type while it shouldn't.

[oli-obk]

Because currently type_of returns an erased type while it shouldn't.

Right, but if we don't do your change which replaces erased regions with static regions, it will keep doing that, making all the region erasing code you wrote a no-op.

What I'm trying to say is that we should not do changes that don't change anything except add more code that the next person looking at this will try to understand the reason for and fail, because there is no reason (at the moment).

Yes anon consts are buggy and do wrong things, but I don't think we should proactively try to patch stuff to prepare for changes that no one is currently actively working on.

[nbdd0121]

@rustbot modify labels -S-waiting-on-review +S-blocked

[bors]

☔ The latest upstream changes (presumably #88088) made this pull request unmergeable. Please resolve the merge conflicts.

[nbdd0121]

Superseded by #89561