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

Cleanup region hierarchy code, especially around closures #3696

Closed
nikomatsakis opened this Issue Oct 8, 2012 · 12 comments

Comments

Projects
None yet
7 participants
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Oct 8, 2012

This is a big that's been on the back of my mind for a while but I don't think it has a bug associated with it. When we build the region hierarchy, we assume that any ||-closure argument is limited to the lifetime of the call where it appears. However, since the more general form of fn& was implemented, I don't believe this is enforced. We should place an upper-bound on the || lifetime and add some tests. In an ideal world, maybe we would always infer the lifetime of an ||-closure, but this is tricky to do because the region hierarchy is built before region inference (and indeed it must be, in order to compute LUB/GLB and so forth).

@bblum

This comment has been minimized.

Copy link
Contributor

bblum commented Oct 11, 2012

I had a weird idea a while ago, for a higher-order function involving returning a stack closure something like:

fn a(blk: &a/fn(c: &blk/fn() -> &blk/fn())) {
    let d = || { ... };
    do blk {
        return d;
    }
}

And then use it like:

do a |c| {
    let d = c();
    d(); // 'd' cannot escape this block
}

I recall that I couldn't make it compile. This sounds like it might enable this (crazy, but cool) sort of thing.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Mar 29, 2013

An example from #3283 that should not build but does (leading to segfaults):

fn foo(+blk: fn(+p: &a/fn() -> &a/fn())) {
    let mut state = 0;
    let statep = &mut state;
    do blk {
        || { *statep = 1; }
    }
}
fn main() {
    do foo |p| { p()() }
}
@cmr

This comment has been minimized.

Copy link
Member

cmr commented Jul 21, 2013

Could not get to compile with:

fn foo<'a>(blk: &fn(p: &'a fn() -> &'a fn())) {
    let mut state = 0;
    let statep = &mut state;
    do blk {
        || { *statep = 1; }
    }
}
fn main() {
    do foo |p| { p()() }
}
@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jul 24, 2013

I did fix some problems in this area though this is still an issue. I have to come up with a good representative test case.

@catamorphism

This comment has been minimized.

Copy link
Contributor

catamorphism commented Oct 14, 2013

@nikomatsakis Does #2202 subsume this, or is there still something left to do?

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Feb 4, 2014

@edwardw

This comment has been minimized.

Copy link
Contributor

edwardw commented Mar 13, 2014

The PR #12828 tries to apply variance rule to self type substitution, also part of #5781, but the following begins to fail:

pub fn main() {
    let a = ~[~""];
    let b: ~[~[&str]] = a.map(|s| s.lines().collect()); // error: cannot infer an appropriate lifetime for autoref due to conflicting requirements
}

First of all, should this compile as it currently does? If yes, then how is the self type substitution intertwined with with closure lifetime inference?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Oct 14, 2014

@bkoropoff I'd love to talk over this issue with you if you're interested in looking at something else after #17403 :) -- I think the two are related, or at least touching pretty similar code.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Dec 9, 2014

@pnkfelix has been working on this

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Feb 14, 2015

@nikomatsakis what's the state of this with the new closure stuff? is it still relevant?

Manishearth added a commit to Manishearth/rust that referenced this issue Mar 2, 2015

Rollup merge of rust-lang#22966 - nikomatsakis:closure-region-hierarc…
…hy, r=pnkfelix

 Remove the synthetic \"region bound\" from closures and instead update how
type-outlives works for closure types so that it ensures that all upvars
outlive the region in question. This gives the same guarantees but
without introducing artificial regions (and gives better error messages
to boot). This is refactoring towards rust-lang#3696.

r? @pnkfelix

Manishearth added a commit to Manishearth/rust that referenced this issue Mar 2, 2015

Rollup merge of rust-lang#22966 - nikomatsakis:closure-region-hierarc…
…hy, r=pnkfelix

 Remove the synthetic \"region bound\" from closures and instead update how
type-outlives works for closure types so that it ensures that all upvars
outlive the region in question. This gives the same guarantees but
without introducing artificial regions (and gives better error messages
to boot). This is refactoring towards rust-lang#3696.

r? @pnkfelix

Manishearth added a commit to Manishearth/rust that referenced this issue Mar 2, 2015

Rollup merge of rust-lang#22966 - nikomatsakis:closure-region-hierarc…
…hy, r=pnkfelix

 Remove the synthetic \"region bound\" from closures and instead update how
type-outlives works for closure types so that it ensures that all upvars
outlive the region in question. This gives the same guarantees but
without introducing artificial regions (and gives better error messages
to boot). This is refactoring towards rust-lang#3696.

r? @pnkfelix

bors added a commit that referenced this issue Apr 1, 2015

Auto merge of #23109 - nikomatsakis:closure-region-hierarchy, r=pnkfelix
Adjust internal treatment of the region hierarchy around closures. Work towards #3696.

r? @pnkfelix

Manishearth added a commit to Manishearth/rust that referenced this issue Apr 1, 2015

Rollup merge of rust-lang#23109 - nikomatsakis:closure-region-hierarc…
…hy, r=pnkfelix

Adjust internal treatment of the region hierarchy around closures. Work towards rust-lang#3696.

r? @pnkfelix

@nikomatsakis nikomatsakis changed the title Define region hierarchy for closures Cleanup region hierarchy code, especially around closures Jul 31, 2015

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jul 31, 2015

I'm updating the title on this issue and repurposing it to also cover the fact that it would be nice if the representation of "free" regions did not always have them attached to a scope. Instead, a free region should just have a number, and we should indicate the relationship that A >= X for some scope X as an in-scope where clause.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jan 26, 2017

@nikomatsakis Still a bug?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 26, 2017

So this is still a bug, but I hope that we will fix it for good once we fully move borrow/region-checking to MIR. The idea is that MIR has fully desugared closures. I am going to close this bug though because it's kind of vague and I don't see how having it open is helpful in any particular way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.