Skip to content

region unification: update universe of region vars #121442

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

Merged
merged 2 commits into from
Feb 23, 2024
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions compiler/rustc_infer/src/infer/canonical/canonicalizer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,12 @@ impl CanonicalizeMode for CanonicalizeQueryResponse {
),

ty::ReVar(vid) => {
let universe =
infcx.inner.borrow_mut().unwrap_region_constraints().var_universe(vid);
let universe = infcx
.inner
.borrow_mut()
.unwrap_region_constraints()
.probe_value(vid)
.unwrap_err();
canonicalizer.canonical_var_for_region(
CanonicalVarInfo { kind: CanonicalVarKind::Region(universe) },
r,
Expand Down
10 changes: 4 additions & 6 deletions compiler/rustc_infer/src/infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,10 @@ impl<'tcx> ty::InferCtxtLike for InferCtxt<'tcx> {
}

fn universe_of_lt(&self, lt: ty::RegionVid) -> Option<ty::UniverseIndex> {
Some(self.universe_of_region_vid(lt))
match self.inner.borrow_mut().unwrap_region_constraints().probe_value(lt) {
Err(universe) => Some(universe),
Ok(_) => None,
}
}

fn root_ty_var(&self, vid: TyVid) -> TyVid {
Expand Down Expand Up @@ -1155,11 +1158,6 @@ impl<'tcx> InferCtxt<'tcx> {
self.inner.borrow_mut().unwrap_region_constraints().universe(r)
}

/// Return the universe that the region variable `r` was created in.
pub fn universe_of_region_vid(&self, vid: ty::RegionVid) -> ty::UniverseIndex {
self.inner.borrow_mut().unwrap_region_constraints().var_universe(vid)
}

/// Number of region variables created so far.
pub fn num_region_vars(&self) -> usize {
self.inner.borrow_mut().unwrap_region_constraints().num_region_vars()
Expand Down
12 changes: 6 additions & 6 deletions compiler/rustc_infer/src/infer/region_constraints/leak_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,11 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
}
}

struct LeakCheck<'me, 'tcx> {
struct LeakCheck<'a, 'b, 'tcx> {
tcx: TyCtxt<'tcx>,
outer_universe: ty::UniverseIndex,
mini_graph: &'me MiniGraph<'tcx>,
rcc: &'me RegionConstraintCollector<'me, 'tcx>,
mini_graph: &'a MiniGraph<'tcx>,
rcc: &'a mut RegionConstraintCollector<'b, 'tcx>,

// Initially, for each SCC S, stores a placeholder `P` such that `S = P`
// must hold.
Expand All @@ -117,13 +117,13 @@ struct LeakCheck<'me, 'tcx> {
scc_universes: IndexVec<LeakCheckScc, SccUniverse<'tcx>>,
}

impl<'me, 'tcx> LeakCheck<'me, 'tcx> {
impl<'a, 'b, 'tcx> LeakCheck<'a, 'b, 'tcx> {
fn new(
tcx: TyCtxt<'tcx>,
outer_universe: ty::UniverseIndex,
max_universe: ty::UniverseIndex,
mini_graph: &'me MiniGraph<'tcx>,
rcc: &'me RegionConstraintCollector<'me, 'tcx>,
mini_graph: &'a MiniGraph<'tcx>,
rcc: &'a mut RegionConstraintCollector<'b, 'tcx>,
) -> Self {
let dummy_scc_universe = SccUniverse { universe: max_universe, region: None };
Self {
Expand Down
107 changes: 69 additions & 38 deletions compiler/rustc_infer/src/infer/region_constraints/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,11 @@ use super::{
};

use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::intern::Interned;
use rustc_data_structures::sync::Lrc;
use rustc_data_structures::undo_log::UndoLogs;
use rustc_data_structures::unify as ut;
use rustc_index::IndexVec;
use rustc_middle::infer::unify_key::{RegionVidKey, UnifiedRegion};
use rustc_middle::infer::unify_key::{RegionVariableValue, RegionVidKey};
use rustc_middle::ty::ReStatic;
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_middle::ty::{ReBound, ReVar};
Expand Down Expand Up @@ -292,6 +291,18 @@ type CombineMap<'tcx> = FxHashMap<TwoRegions<'tcx>, RegionVid>;
#[derive(Debug, Clone, Copy)]
pub struct RegionVariableInfo {
pub origin: RegionVariableOrigin,
// FIXME: This is only necessary for `fn take_and_reset_data` and
// `lexical_region_resolve`. We should rework `lexical_region_resolve`
// in the near/medium future anyways and could move the unverse info
// for `fn take_and_reset_data` into a separate table which is
// only populated when needed.
//
// For both of these cases it is fine that this can diverge from the
// actual universe of the variable, which is directly stored in the
// unification table for unknown region variables. At some point we could
// stop emitting bidirectional outlives constraints if equate succeeds.
// This would be currently unsound as it would cause us to drop the universe
// changes in `lexical_region_resolve`.
pub universe: ty::UniverseIndex,
}

Expand Down Expand Up @@ -395,7 +406,11 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
// `RegionConstraintData` contains the relationship here.
if *any_unifications {
*any_unifications = false;
self.unification_table_mut().reset_unifications(|_| UnifiedRegion::new(None));
// Manually inlined `self.unification_table_mut()` as `self` is used in the closure.
ut::UnificationTable::with_log(&mut self.storage.unification_table, &mut self.undo_log)
.reset_unifications(|key| RegionVariableValue::Unknown {
universe: self.storage.var_infos[key.vid].universe,
});
}

data
Expand All @@ -422,18 +437,13 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
) -> RegionVid {
let vid = self.var_infos.push(RegionVariableInfo { origin, universe });

let u_vid = self.unification_table_mut().new_key(UnifiedRegion::new(None));
let u_vid = self.unification_table_mut().new_key(RegionVariableValue::Unknown { universe });
assert_eq!(vid, u_vid.vid);
self.undo_log.push(AddVar(vid));
debug!("created new region variable {:?} in {:?} with origin {:?}", vid, universe, origin);
vid
}

/// Returns the universe for the given variable.
pub(super) fn var_universe(&self, vid: RegionVid) -> ty::UniverseIndex {
self.var_infos[vid].universe
}

/// Returns the origin for the given variable.
pub(super) fn var_origin(&self, vid: RegionVid) -> RegionVariableOrigin {
self.var_infos[vid].origin
Expand Down Expand Up @@ -467,26 +477,41 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
pub(super) fn make_eqregion(
&mut self,
origin: SubregionOrigin<'tcx>,
sub: Region<'tcx>,
sup: Region<'tcx>,
a: Region<'tcx>,
b: Region<'tcx>,
) {
if sub != sup {
if a != b {
// Eventually, it would be nice to add direct support for
// equating regions.
self.make_subregion(origin.clone(), sub, sup);
self.make_subregion(origin, sup, sub);

match (sub, sup) {
(Region(Interned(ReVar(sub), _)), Region(Interned(ReVar(sup), _))) => {
debug!("make_eqregion: unifying {:?} with {:?}", sub, sup);
self.unification_table_mut().union(*sub, *sup);
self.any_unifications = true;
self.make_subregion(origin.clone(), a, b);
self.make_subregion(origin, b, a);

match (a.kind(), b.kind()) {
(ty::ReVar(a), ty::ReVar(b)) => {
debug!("make_eqregion: unifying {:?} with {:?}", a, b);
if self.unification_table_mut().unify_var_var(a, b).is_ok() {
self.any_unifications = true;
}
}
(ty::ReVar(vid), _) => {
debug!("make_eqregion: unifying {:?} with {:?}", vid, b);
if self
.unification_table_mut()
.unify_var_value(vid, RegionVariableValue::Known { value: b })
.is_ok()
{
self.any_unifications = true;
};
}
(Region(Interned(ReVar(vid), _)), value)
| (value, Region(Interned(ReVar(vid), _))) => {
debug!("make_eqregion: unifying {:?} with {:?}", vid, value);
self.unification_table_mut().union_value(*vid, UnifiedRegion::new(Some(value)));
self.any_unifications = true;
(_, ty::ReVar(vid)) => {
debug!("make_eqregion: unifying {:?} with {:?}", a, vid);
if self
.unification_table_mut()
.unify_var_value(vid, RegionVariableValue::Known { value: a })
.is_ok()
{
self.any_unifications = true;
};
}
(_, _) => {}
}
Expand Down Expand Up @@ -603,18 +628,21 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
tcx: TyCtxt<'tcx>,
vid: ty::RegionVid,
) -> ty::Region<'tcx> {
let mut ut = self.unification_table_mut(); // FIXME(rust-lang/ena#42): unnecessary mut
let mut ut = self.unification_table_mut();
let root_vid = ut.find(vid).vid;
let resolved = ut
.probe_value(root_vid)
.get_value_ignoring_universes()
.unwrap_or_else(|| ty::Region::new_var(tcx, root_vid));

// Don't resolve a variable to a region that it cannot name.
if self.var_universe(vid).can_name(self.universe(resolved)) {
Copy link
Contributor Author

@lcnr lcnr Feb 22, 2024

Choose a reason for hiding this comment

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

if we equate an infer var with a root which is in a higher universe, we previously did not return that root vid here. This caused the PlaceholderReplacer to not map a placeholder region back to a bound var in deeply_normalize (with #119106) due to the following setup:

  • for<'a> fn(RigidAlias<'a>) gets instantiated as fn(RigidAlias<'!a>)
  • normalizing RigidAlias<'!a> emits AliasRelate(RigidAlias<'!a>, ?u)
  • uniquifying regions in the input results in the canonical goal exists<U> exists<'a> AliasRelate(RigidAlias<'a>, U)
  • inside of the query we instantiate this to get RigidAlias<'?a.1> and ?u.0
  • NormalizesTo fails, so we first instantiate ?u.0 with RigidAlias<'new.whatever>, the universe of that gets pulled down into universe 0
  • we then equate the args of RigidAlias<'new.0> with RigidAlias<'a.1>, equating 'new and 'a, using 'a as the root in the unification table
  • when canonicalizing the response, we are unable to canonicalize 'new as 'a because the universe of 'a is higher than the universe of 'new
  • because of this RigidAlias<'!a> normalizes to RigidAlias<'new> where 'new is constrained to be equal to '!a
  • we fail to map 'new back to 'a in the PlaceholderReplacer

Copy link
Member

@aliemjay aliemjay Apr 2, 2024

Choose a reason for hiding this comment

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

  • inside of the query we instantiate this to get RigidAlias<'?a.1> and ?u.0

This looks like an issue with the new solver canonicalization! the universe of ?u should certainly be able to name '?a. How does ?a end up in U1 and ?u as U0?

Copy link
Member

Choose a reason for hiding this comment

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

In the new solver all regions are canonicalized to new inference variables in a single universe that is higher than everything else in the input. ?u can name '?a even with this setup it just requires lowering '?a's universe when equating it which is something we already do for cases like ?x.0 eq ?y.1. Even if the new solver did not have this canonicalization scheme I would still expect this PR to be required somehow as it is just wrong for '?x.0 eq '?y.1 to not allow resolving '?x to '?y later on and there should be other ways to wind up with a region variable in a higher universe that is equated with something in a lower one

resolved
} else {
ty::Region::new_var(tcx, vid)
match ut.probe_value(root_vid) {
RegionVariableValue::Known { value } => value,
RegionVariableValue::Unknown { .. } => ty::Region::new_var(tcx, root_vid),
}
}

pub fn probe_value(
&mut self,
vid: ty::RegionVid,
) -> Result<ty::Region<'tcx>, ty::UniverseIndex> {
match self.unification_table_mut().probe_value(vid) {
RegionVariableValue::Known { value } => Ok(value),
RegionVariableValue::Unknown { universe } => Err(universe),
}
}

Expand Down Expand Up @@ -654,15 +682,18 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
new_r
}

pub fn universe(&self, region: Region<'tcx>) -> ty::UniverseIndex {
pub fn universe(&mut self, region: Region<'tcx>) -> ty::UniverseIndex {
match *region {
ty::ReStatic
| ty::ReErased
| ty::ReLateParam(..)
| ty::ReEarlyParam(..)
| ty::ReError(_) => ty::UniverseIndex::ROOT,
ty::RePlaceholder(placeholder) => placeholder.universe,
ty::ReVar(vid) => self.var_universe(vid),
ty::ReVar(vid) => match self.probe_value(vid) {
Ok(value) => self.universe(value),
Err(universe) => universe,
},
ty::ReBound(..) => bug!("universe(): encountered bound region {:?}", region),
}
}
Expand Down
89 changes: 45 additions & 44 deletions compiler/rustc_middle/src/infer/unify_key.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::ty::{self, Region, Ty, TyCtxt};
use crate::ty::{self, Ty, TyCtxt};
use rustc_data_structures::unify::{NoError, UnifyKey, UnifyValue};
use rustc_span::def_id::DefId;
use rustc_span::symbol::Symbol;
Expand All @@ -10,26 +10,16 @@ pub trait ToType {
fn to_type<'tcx>(&self, tcx: TyCtxt<'tcx>) -> Ty<'tcx>;
}

#[derive(PartialEq, Copy, Clone, Debug)]
pub struct UnifiedRegion<'tcx> {
value: Option<ty::Region<'tcx>>,
}

impl<'tcx> UnifiedRegion<'tcx> {
pub fn new(value: Option<Region<'tcx>>) -> Self {
Self { value }
}

/// The caller is responsible for checking universe compatibility before using this value.
pub fn get_value_ignoring_universes(self) -> Option<Region<'tcx>> {
self.value
}
#[derive(Copy, Clone, Debug)]
pub enum RegionVariableValue<'tcx> {
Known { value: ty::Region<'tcx> },
Unknown { universe: ty::UniverseIndex },
}

#[derive(PartialEq, Copy, Clone, Debug)]
pub struct RegionVidKey<'tcx> {
pub vid: ty::RegionVid,
pub phantom: PhantomData<UnifiedRegion<'tcx>>,
pub phantom: PhantomData<RegionVariableValue<'tcx>>,
}

impl<'tcx> From<ty::RegionVid> for RegionVidKey<'tcx> {
Expand All @@ -39,7 +29,7 @@ impl<'tcx> From<ty::RegionVid> for RegionVidKey<'tcx> {
}

impl<'tcx> UnifyKey for RegionVidKey<'tcx> {
type Value = UnifiedRegion<'tcx>;
type Value = RegionVariableValue<'tcx>;
#[inline]
fn index(&self) -> u32 {
self.vid.as_u32()
Expand All @@ -53,36 +43,47 @@ impl<'tcx> UnifyKey for RegionVidKey<'tcx> {
}
}

impl<'tcx> UnifyValue for UnifiedRegion<'tcx> {
type Error = NoError;
pub struct RegionUnificationError;
impl<'tcx> UnifyValue for RegionVariableValue<'tcx> {
type Error = RegionUnificationError;

fn unify_values(value1: &Self, value2: &Self) -> Result<Self, NoError> {
// We pick the value of the least universe because it is compatible with more variables.
// This is *not* necessary for completeness.
#[cold]
fn min_universe<'tcx>(r1: Region<'tcx>, r2: Region<'tcx>) -> Region<'tcx> {
cmp::min_by_key(r1, r2, |r| match r.kind() {
ty::ReStatic
| ty::ReErased
| ty::ReLateParam(..)
| ty::ReEarlyParam(..)
| ty::ReError(_) => ty::UniverseIndex::ROOT,
ty::RePlaceholder(placeholder) => placeholder.universe,
ty::ReVar(..) | ty::ReBound(..) => bug!("not a universal region"),
})
}

Ok(match (value1.value, value2.value) {
// Here we can just pick one value, because the full constraints graph
// will be handled later. Ideally, we might want a `MultipleValues`
// variant or something. For now though, this is fine.
(Some(val1), Some(val2)) => Self { value: Some(min_universe(val1, val2)) },
fn unify_values(value1: &Self, value2: &Self) -> Result<Self, Self::Error> {
match (*value1, *value2) {
(RegionVariableValue::Known { .. }, RegionVariableValue::Known { .. }) => {
Err(RegionUnificationError)
}

(Some(_), _) => *value1,
(_, Some(_)) => *value2,
(RegionVariableValue::Known { value }, RegionVariableValue::Unknown { universe })
| (RegionVariableValue::Unknown { universe }, RegionVariableValue::Known { value }) => {
let universe_of_value = match value.kind() {
ty::ReStatic
| ty::ReErased
| ty::ReLateParam(..)
| ty::ReEarlyParam(..)
| ty::ReError(_) => ty::UniverseIndex::ROOT,
ty::RePlaceholder(placeholder) => placeholder.universe,
ty::ReVar(..) | ty::ReBound(..) => bug!("not a universal region"),
};

if universe.can_name(universe_of_value) {
Ok(RegionVariableValue::Known { value })
} else {
Err(RegionUnificationError)
}
}

(None, None) => *value1,
})
(
RegionVariableValue::Unknown { universe: a },
RegionVariableValue::Unknown { universe: b },
) => {
// If we unify two unconstrained regions then whatever
// value they wind up taking (which must be the same value) must
// be nameable by both universes. Therefore, the resulting
// universe is the minimum of the two universes, because that is
// the one which contains the fewest names in scope.
Ok(RegionVariableValue::Unknown { universe: a.min(b) })
}
}
}
}

Expand Down