Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix late-bound lifetime closure ICEs in HIR typeck and MIR borrowck #103780

Merged
merged 6 commits into from
Nov 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_borrowck/src/region_infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2314,7 +2314,7 @@ impl<'tcx> ClosureRegionRequirementsExt<'tcx> for ClosureRegionRequirements<'tcx
tcx,
closure_substs,
self.num_external_vids,
tcx.typeck_root_def_id(closure_def_id),
closure_def_id.expect_local(),
);
debug!("apply_requirements: closure_mapping={:?}", closure_mapping);

Expand Down
127 changes: 93 additions & 34 deletions compiler/rustc_borrowck/src/universal_regions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ use rustc_hir::{BodyOwnerKind, HirId};
use rustc_index::vec::{Idx, IndexVec};
use rustc_infer::infer::{InferCtxt, NllRegionVariableOrigin};
use rustc_middle::ty::fold::TypeFoldable;
use rustc_middle::ty::{self, InlineConstSubsts, InlineConstSubstsParts, RegionVid, Ty, TyCtxt};
use rustc_middle::ty::{
self, DefIdTree, InlineConstSubsts, InlineConstSubstsParts, RegionVid, Ty, TyCtxt,
};
use rustc_middle::ty::{InternalSubsts, SubstsRef};
use std::iter;

Expand Down Expand Up @@ -241,15 +243,15 @@ impl<'tcx> UniversalRegions<'tcx> {
tcx: TyCtxt<'tcx>,
closure_substs: SubstsRef<'tcx>,
expected_num_vars: usize,
typeck_root_def_id: DefId,
closure_def_id: LocalDefId,
) -> IndexVec<RegionVid, ty::Region<'tcx>> {
let mut region_mapping = IndexVec::with_capacity(expected_num_vars);
region_mapping.push(tcx.lifetimes.re_static);
tcx.for_each_free_region(&closure_substs, |fr| {
region_mapping.push(fr);
});

for_each_late_bound_region_defined_on(tcx, typeck_root_def_id, |r| {
for_each_late_bound_region_in_recursive_scope(tcx, tcx.local_parent(closure_def_id), |r| {
Copy link
Member

@aliemjay aliemjay Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This functions now assumes being called from the strict parent of the closure, not any higher parent. Can we make it a method on UniversalRegions and add assert_eq!(self.len(), expected_num_vars) and assert_eq!(self.defining_ty.def_id(), tcx.parent(closure_def_id))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make it a method on UniversalRegions

Make what a method? for_each_late_bound_region_in_recursive_scope?

and add assert_eq!(self.len(), expected_num_vars)

What's the difference between this assertion and the one that happens below?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make what a method?

I meant closure_mapping.

What's the difference between this assertion and the one that happens below?

Oops, I missed that. I guess it is sufficient then.

region_mapping.push(r);
});

Expand Down Expand Up @@ -339,9 +341,8 @@ impl<'tcx> UniversalRegions<'tcx> {
// tests, and the resulting print-outs include def-ids
// and other things that are not stable across tests!
// So we just include the region-vid. Annoying.
let typeck_root_def_id = tcx.typeck_root_def_id(def_id);
for_each_late_bound_region_defined_on(tcx, typeck_root_def_id, |r| {
err.note(&format!("late-bound region is {:?}", self.to_region_vid(r),));
for_each_late_bound_region_in_recursive_scope(tcx, def_id.expect_local(), |r| {
err.note(&format!("late-bound region is {:?}", self.to_region_vid(r)));
});
}
DefiningTy::Generator(def_id, substs, _) => {
Expand All @@ -354,9 +355,8 @@ impl<'tcx> UniversalRegions<'tcx> {
// FIXME: As above, we'd like to print out the region
// `r` but doing so is not stable across architectures
// and so forth.
let typeck_root_def_id = tcx.typeck_root_def_id(def_id);
for_each_late_bound_region_defined_on(tcx, typeck_root_def_id, |r| {
err.note(&format!("late-bound region is {:?}", self.to_region_vid(r),));
for_each_late_bound_region_in_recursive_scope(tcx, def_id.expect_local(), |r| {
err.note(&format!("late-bound region is {:?}", self.to_region_vid(r)));
});
}
DefiningTy::FnDef(def_id, substs) => {
Expand Down Expand Up @@ -421,13 +421,24 @@ impl<'cx, 'tcx> UniversalRegionsBuilder<'cx, 'tcx> {
first_extern_index
} else {
// If this is a closure, generator, or inline-const, then the late-bound regions from the enclosing
// function are actually external regions to us. For example, here, 'a is not local
// function/closures 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 = ...; }
// }
self.infcx
.replace_late_bound_regions_with_nll_infer_vars(self.mir_def.did, &mut indices);
for_each_late_bound_region_in_recursive_scope(
self.infcx.tcx,
self.infcx.tcx.local_parent(self.mir_def.did),
|r| {
debug!(?r);
if !indices.indices.contains_key(&r) {
let region_vid = self.infcx.next_nll_region_var(FR);
debug!(?region_vid);
indices.insert_late_bound_region(r, region_vid.to_region_vid());
}
},
);

// Any regions created during the execution of `defining_ty` or during the above
// late-bound region replacement are all considered 'extern' regions
self.infcx.num_region_vars()
Expand All @@ -444,12 +455,16 @@ impl<'cx, 'tcx> UniversalRegionsBuilder<'cx, 'tcx> {
bound_inputs_and_output,
&mut indices,
);
// Converse of above, if this is a function then the late-bound regions declared on its
// signature are local to the fn.
if self.mir_def.did.to_def_id() == typeck_root_def_id {
self.infcx
.replace_late_bound_regions_with_nll_infer_vars(self.mir_def.did, &mut indices);
}
// Converse of above, if this is a function/closure then the late-bound regions declared on its
// signature are local.
for_each_late_bound_region_in_item(self.infcx.tcx, self.mir_def.did, |r| {
debug!(?r);
if !indices.indices.contains_key(&r) {
let region_vid = self.infcx.next_nll_region_var(FR);
debug!(?region_vid);
indices.insert_late_bound_region(r, region_vid.to_region_vid());
}
});

let (unnormalized_output_ty, mut unnormalized_input_tys) =
inputs_and_output.split_last().unwrap();
Expand Down Expand Up @@ -692,7 +707,13 @@ trait InferCtxtExt<'tcx> {
where
T: TypeFoldable<'tcx>;

fn replace_late_bound_regions_with_nll_infer_vars(
fn replace_late_bound_regions_with_nll_infer_vars_in_recursive_scope(
&self,
mir_def_id: LocalDefId,
indices: &mut UniversalRegionIndices<'tcx>,
);

fn replace_late_bound_regions_with_nll_infer_vars_in_item(
&self,
mir_def_id: LocalDefId,
indices: &mut UniversalRegionIndices<'tcx>,
Expand Down Expand Up @@ -746,13 +767,28 @@ impl<'tcx> InferCtxtExt<'tcx> for InferCtxt<'tcx> {
/// set of late-bound regions and checks for any that we have not yet seen, adding them to the
/// inputs vector.
#[instrument(skip(self, indices))]
fn replace_late_bound_regions_with_nll_infer_vars(
fn replace_late_bound_regions_with_nll_infer_vars_in_recursive_scope(
&self,
mir_def_id: LocalDefId,
indices: &mut UniversalRegionIndices<'tcx>,
) {
for_each_late_bound_region_in_recursive_scope(self.tcx, mir_def_id, |r| {
debug!(?r);
if !indices.indices.contains_key(&r) {
let region_vid = self.next_nll_region_var(FR);
debug!(?region_vid);
indices.insert_late_bound_region(r, region_vid.to_region_vid());
}
});
}

#[instrument(skip(self, indices))]
fn replace_late_bound_regions_with_nll_infer_vars_in_item(
&self,
mir_def_id: LocalDefId,
indices: &mut UniversalRegionIndices<'tcx>,
) {
let typeck_root_def_id = self.tcx.typeck_root_def_id(mir_def_id.to_def_id());
for_each_late_bound_region_defined_on(self.tcx, typeck_root_def_id, |r| {
for_each_late_bound_region_in_item(self.tcx, mir_def_id, |r| {
debug!(?r);
if !indices.indices.contains_key(&r) {
let region_vid = self.next_nll_region_var(FR);
Expand Down Expand Up @@ -803,21 +839,44 @@ impl<'tcx> UniversalRegionIndices<'tcx> {
}
}

/// Iterates over the late-bound regions defined on fn_def_id and
/// invokes `f` with the liberated form of each one.
fn for_each_late_bound_region_defined_on<'tcx>(
/// Iterates over the late-bound regions defined on `mir_def_id` and all of its
/// parents, up to the typeck root, and invokes `f` with the liberated form
/// of each one.
fn for_each_late_bound_region_in_recursive_scope<'tcx>(
tcx: TyCtxt<'tcx>,
fn_def_id: DefId,
mut mir_def_id: LocalDefId,
mut f: impl FnMut(ty::Region<'tcx>),
) {
if let Some(late_bounds) = tcx.is_late_bound_map(fn_def_id.expect_local()) {
for &region_def_id in late_bounds.iter() {
let name = tcx.item_name(region_def_id.to_def_id());
let liberated_region = tcx.mk_region(ty::ReFree(ty::FreeRegion {
scope: fn_def_id,
bound_region: ty::BoundRegionKind::BrNamed(region_def_id.to_def_id(), name),
}));
f(liberated_region);
let typeck_root_def_id = tcx.typeck_root_def_id(mir_def_id.to_def_id());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To have a single loop, we can use while tcx.is_typeck_child(mir_def_id.to_def_id()).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't loop over the root def id, but stops one def id before it -- I'm not sure if I understand?


// Walk up the tree, collecting late-bound regions until we hit the typeck root
loop {
for_each_late_bound_region_in_item(tcx, mir_def_id, &mut f);

if mir_def_id.to_def_id() == typeck_root_def_id {
break;
} else {
mir_def_id = tcx.local_parent(mir_def_id);
}
}
}

/// Iterates over the late-bound regions defined on `mir_def_id` and all of its
/// parents, up to the typeck root, and invokes `f` with the liberated form
/// of each one.
fn for_each_late_bound_region_in_item<'tcx>(
tcx: TyCtxt<'tcx>,
mir_def_id: LocalDefId,
mut f: impl FnMut(ty::Region<'tcx>),
) {
if !tcx.def_kind(mir_def_id).is_fn_like() {
return;
}

for bound_var in tcx.late_bound_vars(tcx.hir().local_def_id_to_hir_id(mir_def_id)) {
let ty::BoundVariableKind::Region(bound_region) = bound_var else { continue; };
let liberated_region = tcx
.mk_region(ty::ReFree(ty::FreeRegion { scope: mir_def_id.to_def_id(), bound_region }));
f(liberated_region);
}
}
7 changes: 4 additions & 3 deletions compiler/rustc_hir_analysis/src/collect/lifetimes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1377,11 +1377,12 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
} else if let Some(body_id) = outermost_body {
let fn_id = self.tcx.hir().body_owner(body_id);
match self.tcx.hir().get(fn_id) {
Node::Item(&hir::Item { kind: hir::ItemKind::Fn(..), .. })
| Node::TraitItem(&hir::TraitItem {
Node::Item(hir::Item { kind: hir::ItemKind::Fn(..), .. })
| Node::TraitItem(hir::TraitItem {
kind: hir::TraitItemKind::Fn(..), ..
})
| Node::ImplItem(&hir::ImplItem { kind: hir::ImplItemKind::Fn(..), .. }) => {
| Node::ImplItem(hir::ImplItem { kind: hir::ImplItemKind::Fn(..), .. })
| Node::Expr(hir::Expr { kind: hir::ExprKind::Closure(..), .. }) => {
let scope = self.tcx.hir().local_def_id(fn_id);
def = Region::Free(scope.to_def_id(), def.id().unwrap());
}
Expand Down
9 changes: 9 additions & 0 deletions src/test/ui/closures/binder/late-bound-in-body.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// check-pass

#![feature(closure_lifetime_binder)]

fn main() {
let _ = for<'a> || -> () {
let _: &'a bool = &true;
};
}
9 changes: 9 additions & 0 deletions src/test/ui/closures/binder/nested-closures-regions.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// check-pass

#![feature(closure_lifetime_binder)]
#![feature(rustc_attrs)]

#[rustc_regions]
fn main() {
for<'a> || -> () { for<'c> |_: &'a ()| -> () {}; };
}
38 changes: 38 additions & 0 deletions src/test/ui/closures/binder/nested-closures-regions.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
note: external requirements
--> $DIR/nested-closures-regions.rs:8:24
|
LL | for<'a> || -> () { for<'c> |_: &'a ()| -> () {}; };
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: defining type: main::{closure#0}::{closure#0} with closure substs [
i8,
extern "rust-call" fn((&(),)),
(),
]
= note: late-bound region is '_#4r
= note: late-bound region is '_#2r
= note: number of external vids: 3
= note: where '_#1r: '_#2r
= note: where '_#2r: '_#1r

note: no external requirements
--> $DIR/nested-closures-regions.rs:8:5
|
LL | for<'a> || -> () { for<'c> |_: &'a ()| -> () {}; };
| ^^^^^^^^^^^^^^^^
|
= note: defining type: main::{closure#0} with closure substs [
i8,
extern "rust-call" fn(()),
(),
]
= note: late-bound region is '_#2r

note: no external requirements
--> $DIR/nested-closures-regions.rs:7:1
|
LL | fn main() {
| ^^^^^^^^^
|
= note: defining type: main

7 changes: 7 additions & 0 deletions src/test/ui/closures/binder/nested-closures.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// check-pass

#![feature(closure_lifetime_binder)]

fn main() {
for<'a> || -> () { for<'c> |_: &'a ()| -> () {}; };
}