Skip to content

Be more careful when selecting LUB of free regions #27892

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.

Sign up for GitHub

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 17 commits into from
Aug 22, 2015
Merged

Be more careful when selecting LUB of free regions #27892

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
5448de7
generalize bitvector code into a bitmatrix; write some unit tests, but
nikomatsakis Aug 18, 2015
38df8c6
reorder imports to respect the alphabet, only one of the single most
nikomatsakis Aug 18, 2015
4756d4a
remove unused import
nikomatsakis Aug 18, 2015
5e126e4
implement transitive relation type that can compute transitive
nikomatsakis Aug 18, 2015
4b1d3b7
rewrite `free_region`/`region_inference` to use newly minted
nikomatsakis Aug 18, 2015
dcf6f08
kill the old funky `can_reach` fn
nikomatsakis Aug 18, 2015
d5b679b
add a regression test for #27583. Fixes #27583.
nikomatsakis Aug 18, 2015
36809bf
clarify diagonal arrows
nikomatsakis Aug 21, 2015
2a25876
add test cases suggested by pnkfelix
nikomatsakis Aug 21, 2015
aa469a9
add final test case, correct one of the others (both versions produced
nikomatsakis Aug 21, 2015
10b8941
fix accidental reversal of 'static, and add a test
nikomatsakis Aug 21, 2015
b247402
nits from pnkfelix
nikomatsakis Aug 21, 2015
a54b91f
remove use of swap_remove and compress the list as we go instead
nikomatsakis Aug 21, 2015
1630c79
rename `best_upper_bound` to `postdom_upper_bound`
nikomatsakis Aug 21, 2015
63eedfc
missed one reference to "best"
nikomatsakis Aug 21, 2015
57909f7
move the reverse into the iterator
nikomatsakis Aug 21, 2015
81eab1c
completely aborted comment
nikomatsakis Aug 21, 2015
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
83 changes: 57 additions & 26 deletions src/librustc/middle/free_region.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,18 @@

//! This file defines
Copy link
Member

Choose a reason for hiding this comment

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

I love


use middle::ty::{self, FreeRegion, Region};
use middle::wf::ImpliedBound;
use middle::ty::{self, FreeRegion};
use util::common::can_reach;
use util::nodemap::{FnvHashMap, FnvHashSet};
use rustc_data_structures::transitive_relation::TransitiveRelation;

#[derive(Clone)]
pub struct FreeRegionMap {
/// `map` maps from a free region `a` to a list of
/// free regions `bs` such that `a <= b for all b in bs`
map: FnvHashMap<FreeRegion, Vec<FreeRegion>>,
/// regions that are required to outlive (and therefore be
/// equal to) 'static.
statics: FnvHashSet<FreeRegion>
relation: TransitiveRelation<Region>
Copy link
Member

Choose a reason for hiding this comment

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

I know the code is practically self-documenting, but it might be nice to at least state here what relation is encoded in this field. (Namely whether a relation a -> b is interpreted as a: b or as b: a.)

}

impl FreeRegionMap {
pub fn new() -> FreeRegionMap {
FreeRegionMap { map: FnvHashMap(), statics: FnvHashSet() }
FreeRegionMap { relation: TransitiveRelation::new() }
}

pub fn relate_free_regions_from_implied_bounds<'tcx>(&mut self,
Expand Down Expand Up @@ -84,22 +78,38 @@ impl FreeRegionMap {
}

fn relate_to_static(&mut self, sup: FreeRegion) {
self.statics.insert(sup);
self.relation.add(ty::ReStatic, ty::ReFree(sup));
}

fn relate_free_regions(&mut self, sub: FreeRegion, sup: FreeRegion) {
let mut sups = self.map.entry(sub).or_insert(Vec::new());
if !sups.contains(&sup) {
sups.push(sup);
}
self.relation.add(ty::ReFree(sub), ty::ReFree(sup))
}

/// Determines whether two free regions have a subregion relationship
/// by walking the graph encoded in `map`. Note that
/// it is possible that `sub != sup` and `sub <= sup` and `sup <= sub`
/// (that is, the user can give two different names to the same lifetime).
pub fn sub_free_region(&self, sub: FreeRegion, sup: FreeRegion) -> bool {
can_reach(&self.map, sub, sup) || self.is_static(&sup)
let result = sub == sup || {
let sub = ty::ReFree(sub);
let sup = ty::ReFree(sup);
self.relation.contains(&sub, &sup) || self.relation.contains(&sup, &ty::ReStatic)
Copy link
Member

Choose a reason for hiding this comment

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

The right-hand disjunct, self.relation.contains(&sup, &ty::ReStatic), does not seem right to me.

  • (I.e., it seems like we are saying "you can conclude A <= B if either 1. (A,B) in REL, or 2. (B,'static) in REL", but why would that relation of B with 'static have anything to do with A and B ?)

(When I first read it, I thought that it was trying to capture some sort of reasoning like "'static <= B therefore 'static == B therefore A <= B", but on my second read it seems like it was just a mistranslation of the self.is_static(&sup) disjunct from the old code.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pnkfelix

The right-hand disjunct, self.relation.contains(&sup, &ty::ReStatic), does not seem right to me.

Indeed, you'd think I should be able to make some sort of test that will fail here... or at least fail to compile when expected. I think it doesn't make everything go completely nuts simply because ty::ReStatic is usually not in the relation (and when it is, it never has incoming edges).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fn test<'a,'b>(x: &'a i32) -> &'b i32
    where 'a: 'static
{
    x
}

fn main() { }

};
debug!("sub_free_region(sub={:?}, sup={:?}) = {:?}", sub, sup, result);
result
}

pub fn lub_free_regions(&self, fr_a: FreeRegion, fr_b: FreeRegion) -> Region {
let r_a = ty::ReFree(fr_a);
let r_b = ty::ReFree(fr_b);
let result = if fr_a == fr_b { r_a } else {
match self.relation.best_upper_bound(&r_a, &r_b) {
None => ty::ReStatic,
Some(r) => *r,
}
};
debug!("lub_free_regions(fr_a={:?}, fr_b={:?}) = {:?}", fr_a, fr_b, result);
result
}

/// Determines whether one region is a subregion of another. This is intended to run *after
Expand All @@ -109,10 +119,7 @@ impl FreeRegionMap {
sub_region: ty::Region,
super_region: ty::Region)
-> bool {
debug!("is_subregion_of(sub_region={:?}, super_region={:?})",
sub_region, super_region);

sub_region == super_region || {
let result = sub_region == super_region || {
match (sub_region, super_region) {
(ty::ReEmpty, _) |
(_, ty::ReStatic) =>
Expand All @@ -121,23 +128,47 @@ impl FreeRegionMap {
(ty::ReScope(sub_scope), ty::ReScope(super_scope)) =>
tcx.region_maps.is_subscope_of(sub_scope, super_scope),

(ty::ReScope(sub_scope), ty::ReFree(ref fr)) =>
tcx.region_maps.is_subscope_of(sub_scope, fr.scope.to_code_extent()),
(ty::ReScope(sub_scope), ty::ReFree(fr)) =>
tcx.region_maps.is_subscope_of(sub_scope, fr.scope.to_code_extent()) ||
self.is_static(fr),

(ty::ReFree(sub_fr), ty::ReFree(super_fr)) =>
self.sub_free_region(sub_fr, super_fr),

(ty::ReStatic, ty::ReFree(ref sup_fr)) => self.is_static(sup_fr),
(ty::ReStatic, ty::ReFree(sup_fr)) =>
self.is_static(sup_fr),

_ =>
false,
}
}
};
debug!("is_subregion_of(sub_region={:?}, super_region={:?}) = {:?}",
sub_region, super_region, result);
result
}

/// Determines whether this free-region is required to be 'static
pub fn is_static(&self, super_region: &ty::FreeRegion) -> bool {
pub fn is_static(&self, super_region: ty::FreeRegion) -> bool {
debug!("is_static(super_region={:?})", super_region);
self.statics.iter().any(|s| can_reach(&self.map, *s, *super_region))
self.relation.contains(&ty::ReStatic, &ty::ReFree(super_region))
}
}

#[cfg(test)]
fn free_region(index: u32) -> FreeRegion {
use middle::region::DestructionScopeData;
FreeRegion { scope: DestructionScopeData::new(0),
bound_region: ty::BoundRegion::BrAnon(index) }
}

#[test]
fn lub() {
// a very VERY basic test, but see the tests in
// TransitiveRelation, which are much more thorough.
let frs: Vec<_> = (0..3).map(|i| free_region(i)).collect();
let mut map = FreeRegionMap::new();
map.relate_free_regions(frs[0], frs[2]);
map.relate_free_regions(frs[1], frs[2]);
assert_eq!(map.lub_free_regions(frs[0], frs[1]), ty::ReFree(frs[2]));
}

37 changes: 4 additions & 33 deletions src/librustc/middle/infer/region_inference/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -812,8 +812,8 @@ impl<'a, 'tcx> RegionVarBindings<'a, 'tcx> {
ReScope(self.tcx.region_maps.nearest_common_ancestor(a_id, b_id))
}

(ReFree(ref a_fr), ReFree(ref b_fr)) => {
self.lub_free_regions(free_regions, a_fr, b_fr)
(ReFree(a_fr), ReFree(b_fr)) => {
free_regions.lub_free_regions(a_fr, b_fr)
}

// For these types, we cannot define any additional
Expand All @@ -825,35 +825,6 @@ impl<'a, 'tcx> RegionVarBindings<'a, 'tcx> {
}
}

/// Computes a region that encloses both free region arguments. Guarantee that if the same two
/// regions are given as argument, in any order, a consistent result is returned.
fn lub_free_regions(&self,
free_regions: &FreeRegionMap,
a: &FreeRegion,
b: &FreeRegion)
-> ty::Region
{
return match a.cmp(b) {
Less => helper(self, free_regions, a, b),
Greater => helper(self, free_regions, b, a),
Equal => ty::ReFree(*a)
};

fn helper(_this: &RegionVarBindings,
free_regions: &FreeRegionMap,
a: &FreeRegion,
b: &FreeRegion) -> ty::Region
{
if free_regions.sub_free_region(*a, *b) {
ty::ReFree(*b)
} else if free_regions.sub_free_region(*b, *a) {
ty::ReFree(*a)
} else {
ty::ReStatic
}
}
}

fn glb_concrete_regions(&self,
free_regions: &FreeRegionMap,
a: Region,
Expand Down Expand Up @@ -892,8 +863,8 @@ impl<'a, 'tcx> RegionVarBindings<'a, 'tcx> {
b));
}

(ReFree(ref fr), ReScope(s_id)) |
(ReScope(s_id), ReFree(ref fr)) => {
(ReFree(fr), ReScope(s_id)) |
(ReScope(s_id), ReFree(fr)) => {
let s = ReScope(s_id);
// Free region is something "at least as big as
// `fr.scope_id`." If we find that the scope `fr.scope_id` is bigger
Expand Down
43 changes: 0 additions & 43 deletions src/librustc/util/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,49 +203,6 @@ pub fn block_query<P>(b: &ast::Block, p: P) -> bool where P: FnMut(&ast::Expr) -
return v.flag;
}

/// K: Eq + Hash<S>, V, S, H: Hasher<S>
///
/// Determines whether there exists a path from `source` to `destination`. The
/// graph is defined by the `edges_map`, which maps from a node `S` to a list of
/// its adjacent nodes `T`.
///
/// Efficiency note: This is implemented in an inefficient way because it is
/// typically invoked on very small graphs. If the graphs become larger, a more
/// efficient graph representation and algorithm would probably be advised.
pub fn can_reach<T, S>(edges_map: &HashMap<T, Vec<T>, S>, source: T,
destination: T) -> bool
where S: HashState, T: Hash + Eq + Clone,
{
if source == destination {
return true;
}

// Do a little breadth-first-search here. The `queue` list
// doubles as a way to detect if we've seen a particular FR
// before. Note that we expect this graph to be an *extremely
// shallow* tree.
let mut queue = vec!(source);
let mut i = 0;
while i < queue.len() {
match edges_map.get(&queue[i]) {
Some(edges) => {
for target in edges {
if *target == destination {
return true;
}

if !queue.iter().any(|x| x == target) {
queue.push((*target).clone());
}
}
}
None => {}
}
i += 1;
}
return false;
}

/// Memoizes a one-argument closure using the given RefCell containing
/// a type implementing MutableMap to serve as a cache.
///
Expand Down
Loading