Skip to content

[PERF] allow region unification to change region universes #108867

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

Closed
wants to merge 4 commits into from
Closed
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
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,13 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
}
}

struct LeakCheck<'me, 'tcx> {
struct LeakCheck<'me, 'rcc, 'tcx> {
tcx: TyCtxt<'tcx>,
universe_at_start_of_snapshot: ty::UniverseIndex,
/// Only used when reporting region errors.
overly_polymorphic: bool,
mini_graph: &'me MiniGraph<'tcx>,
rcc: &'me RegionConstraintCollector<'me, 'tcx>,
rcc: &'me mut RegionConstraintCollector<'rcc, 'tcx>,

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

impl<'me, 'tcx> LeakCheck<'me, 'tcx> {
impl<'me, 'rcc, 'tcx> LeakCheck<'me, 'rcc, 'tcx> {
fn new(
tcx: TyCtxt<'tcx>,
universe_at_start_of_snapshot: ty::UniverseIndex,
max_universe: ty::UniverseIndex,
overly_polymorphic: bool,
mini_graph: &'me MiniGraph<'tcx>,
rcc: &'me RegionConstraintCollector<'me, 'tcx>,
rcc: &'me mut RegionConstraintCollector<'rcc, 'tcx>,
) -> Self {
let dummy_scc_universe = SccUniverse { universe: max_universe, region: None };
Self {
Expand Down
25 changes: 16 additions & 9 deletions compiler/rustc_infer/src/infer/region_constraints/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,7 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
/// Not legal during a snapshot.
pub fn into_infos_and_data(self) -> (VarInfos, RegionConstraintData<'tcx>) {
assert!(!UndoLogs::<super::UndoLog<'_>>::in_snapshot(&self.undo_log));
// FIXME update universes of var_infos
(mem::take(&mut self.storage.var_infos), mem::take(&mut self.storage.data))
}

Expand All @@ -397,11 +398,11 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
// should think carefully about whether it needs to be cleared
// or updated in some way.
let RegionConstraintStorage {
var_infos: _,
var_infos,
data,
lubs,
glbs,
unification_table: _,
unification_table,
any_unifications,
} = self.storage;

Expand All @@ -420,7 +421,11 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
// `RegionConstraintData` contains the relationship here.
if *any_unifications {
*any_unifications = false;
self.unification_table_mut().reset_unifications(|_| UnifiedRegion(None));
let mut ut = ut::UnificationTable::with_log(unification_table, &mut self.undo_log);
ut.reset_unifications(|key| UnifiedRegion {
universe: var_infos[key.vid].universe,
value: None,
});
}

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

let u_vid = self.unification_table_mut().new_key(UnifiedRegion(None));
let u_vid = self.unification_table_mut().new_key(UnifiedRegion { universe, value: None });
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
pub(super) fn var_universe(&mut self, vid: RegionVid) -> ty::UniverseIndex {
self.unification_table_mut().probe_value(vid).universe
}

/// Returns the origin for the given variable.
Expand Down Expand Up @@ -522,7 +527,9 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
(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(Some(value)));
let value =
UnifiedRegion { universe: self.universe(value), value: Some(value) };
self.unification_table_mut().union_value(*vid, value);
self.any_unifications = true;
}
(_, _) => {}
Expand Down Expand Up @@ -642,7 +649,7 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
) -> ty::Region<'tcx> {
let mut ut = self.unification_table_mut(); // FIXME(rust-lang/ena#42): unnecessary mut
let root_vid = ut.find(vid).vid;
ut.probe_value(root_vid).0.unwrap_or_else(|| tcx.mk_re_var(root_vid))
ut.probe_value(root_vid).value.unwrap_or_else(|| tcx.mk_re_var(root_vid))
}

fn combine_map(&mut self, t: CombineMapType) -> &mut CombineMap<'tcx> {
Expand Down Expand Up @@ -681,7 +688,7 @@ 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
Expand Down
19 changes: 6 additions & 13 deletions compiler/rustc_middle/src/infer/unify_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ pub trait ToType {
}

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

Choose a reason for hiding this comment

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

similar to TypeVariableValue and ConstVariableValue change this to an enum and only have a universe in the Unknown case? 🤔

Copy link
Member Author

@aliemjay aliemjay Mar 8, 2023

Choose a reason for hiding this comment

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

although it seems redundant, but I thought it might be a good idea to store the universe of universals and existentials in the same place.

Copy link
Contributor

Choose a reason for hiding this comment

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

if region unification is infallible i think we even have to keep the universe even if value is Some, don't we? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. universe field here is redundant if value is Some.


#[derive(PartialEq, Copy, Clone, Debug)]
pub struct RegionVidKey<'tcx> {
Expand Down Expand Up @@ -43,18 +46,8 @@ impl<'tcx> UnifyKey for RegionVidKey<'tcx> {
impl<'tcx> UnifyValue for UnifiedRegion<'tcx> {
type Error = NoError;

fn unify_values(value1: &Self, value2: &Self) -> Result<Self, NoError> {
Ok(match (value1.0, value2.0) {
// 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(_), Some(_)) => *value1,

(Some(_), _) => *value1,
(_, Some(_)) => *value2,

(None, None) => *value1,
})
fn unify_values(value1: &Self, value2: &Self) -> Result<Self, Self::Error> {
Ok(cmp::min_by_key(*value1, *value2, |val| (val.universe, val.value.is_none())))
Copy link
Contributor

Choose a reason for hiding this comment

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

this can pull down the universe of known regions which seems wrong 🤔 I would expect us to have to eagerly error here if we relate with a known region

Copy link
Member Author

@aliemjay aliemjay Mar 8, 2023

Choose a reason for hiding this comment

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

The idea is that unification should always be infallible, the constraint graph will eventually emit a universe error in this case.

The only invariant we should maintain in the unification table is that an existential cannot resolve to a region in a higher universe (plus one nice thing to have is to prefer universals over existentials within the same universe, but that's not necessary for soundness).

}
}

Expand Down
4 changes: 2 additions & 2 deletions tests/rustdoc/normalize-assoc-item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,12 @@ impl<'a> Lifetimes<'a> for usize {
type Y = &'a isize;
}

// @has 'normalize_assoc_item/fn.g.html' '//pre[@class="rust item-decl"]' "pub fn g() -> &isize"
// @has 'normalize_assoc_item/fn.g.html' '//pre[@class="rust item-decl"]' "pub fn g() -> &'static isize"
pub fn g() -> <usize as Lifetimes<'static>>::Y {
&0
}

// @has 'normalize_assoc_item/constant.A.html' '//pre[@class="rust item-decl"]' "pub const A: &isize"
// @has 'normalize_assoc_item/constant.A.html' '//pre[@class="rust item-decl"]' "pub const A: &'static isize"
pub const A: <usize as Lifetimes<'static>>::Y = &0;

// test cross-crate re-exports
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@
//
// In particular, we test this pattern in trait solving, where it is not connected
// to any part of the source code.
//
// check-pass
// Oops!

use std::cell::Cell;

Expand All @@ -28,5 +25,5 @@ fn main() {
// yielding `fn(&!b u32)`, in a fresh universe U1
// - So we get `?a = !b` but the universe U0 assigned to `?a` cannot name `!b`.

foo::<()>();
foo::<()>(); //~ ERROR implementation of `Trait` is not general enough
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
error: implementation of `Trait` is not general enough
--> $DIR/hrtb-exists-forall-trait-invariant.rs:28:5
|
LL | foo::<()>();
| ^^^^^^^^^^^ implementation of `Trait` is not general enough
|
= note: `()` must implement `Trait<for<'b> fn(Cell<&'b u32>)>`
= note: ...but it actually implements `Trait<fn(Cell<&'0 u32>)>`, for some specific lifetime `'0`

error: aborting due to previous error

6 changes: 6 additions & 0 deletions tests/ui/higher-rank-trait-bounds/hrtb-just-for-static.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ LL | fn give_some<'a>() {
| -- lifetime `'a` defined here
LL | want_hrtb::<&'a u32>()
| ^^^^^^^^^^^^^^^^^^^^ requires that `'a` must outlive `'static`
|
note: due to current limitations in the borrow checker, this implies a `'static` lifetime
--> $DIR/hrtb-just-for-static.rs:9:15
|
LL | where T : for<'a> Foo<&'a isize>
| ^^^^^^^^^^^^^^^^^^^^^^

error: implementation of `Foo` is not general enough
--> $DIR/hrtb-just-for-static.rs:30:5
Expand Down