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

Closure reform #2202

Closed
nikomatsakis opened this Issue Apr 14, 2012 · 29 comments

Comments

Projects
None yet
9 participants
@nikomatsakis
Contributor

nikomatsakis commented Apr 14, 2012

UPDATE:

This is a meta-bug now for tracking the work on completing closures. Now that #6801 has landed, the remaining pieces of work are:

  • Issue #3696: Define region hierarchy for closures
  • Issue #10553: We should not be accepting 'a || as syntax
  • Issue #8622: Unboxed closures -- for the purposes of #2202, just having a design and ensuring our treatment of closures is compatible with it is enough.
  • Issue #12224: Calling a closure is a kind of borrow.
  • Issue #13621: Explain closures much better in the tutorial

ORIGINAL FOLLOWS:
We need to revamp function types in light of regions. I think fn() (the "any" function) will go away, and fn@() and fn~() will be coercable to fn&(), just as with poiners/vectors/slices/etc. Also, fn&() becomes fn&r() (that is, r is a region), though the user won't typically need to write it. We can probably leave the representation as is, though we could also tweak fn&() to not have a ref count since there will be an explicit borrowing that occurs.

@ghost ghost assigned nikomatsakis Apr 14, 2012

@catamorphism

This comment has been minimized.

Show comment
Hide comment
@catamorphism

catamorphism Apr 19, 2012

Contributor

@nikomatsakis Should this be an RFC, since it would change syntax?

Contributor

catamorphism commented Apr 19, 2012

@nikomatsakis Should this be an RFC, since it would change syntax?

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 19, 2012

Contributor

I don't feel it's necessary as I think it is more like a tweak. More importantly, I don't think that the work on regions makes much sense without taking these steps.

Contributor

nikomatsakis commented Apr 19, 2012

I don't feel it's necessary as I think it is more like a tweak. More importantly, I don't think that the work on regions makes much sense without taking these steps.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 19, 2012

Contributor

But I am happy to prepare an RFC if you think it makes sense.

Contributor

nikomatsakis commented Apr 19, 2012

But I am happy to prepare an RFC if you think it makes sense.

@catamorphism

This comment has been minimized.

Show comment
Hide comment
@catamorphism

catamorphism Apr 19, 2012

Contributor

Well, by the RFC process criteria, it is a change to the AST that isn't just refactoring, doesn't relate to numerical criteria, and would be noticed by end users. But if you think that would be overkill, it's your call.

Contributor

catamorphism commented Apr 19, 2012

Well, by the RFC process criteria, it is a change to the AST that isn't just refactoring, doesn't relate to numerical criteria, and would be noticed by end users. But if you think that would be overkill, it's your call.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 19, 2012

Contributor

OK. FWIW I don't think it'd be noticed by end-users: that is, all existing syntax will remain valid, it'll just be re-interpreted to be regions-based.

Contributor

nikomatsakis commented Apr 19, 2012

OK. FWIW I don't think it'd be noticed by end-users: that is, all existing syntax will remain valid, it'll just be re-interpreted to be regions-based.

@catamorphism

This comment has been minimized.

Show comment
Hide comment
@catamorphism

catamorphism Apr 19, 2012

Contributor

I'm confused -- you said in the original post that "fn() (the "any" function) will go away"? So isn't that making existing syntax not valid anymore? (Not trying to be antagonistic here, I just don't understand.)

Contributor

catamorphism commented Apr 19, 2012

I'm confused -- you said in the original post that "fn() (the "any" function) will go away"? So isn't that making existing syntax not valid anymore? (Not trying to be antagonistic here, I just don't understand.)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 19, 2012

Contributor

You're right, what I wrote was misleading (or perhaps just plain wrong). I meant that it would go away in the type system. You'd still be able to type fn(), it'd just be omitting the region bound (region bounds can always be omitted in all cases where they appear, such as slices etc).

Contributor

nikomatsakis commented Apr 19, 2012

You're right, what I wrote was misleading (or perhaps just plain wrong). I meant that it would go away in the type system. You'd still be able to type fn(), it'd just be omitting the region bound (region bounds can always be omitted in all cases where they appear, such as slices etc).

@catamorphism

This comment has been minimized.

Show comment
Hide comment
@catamorphism

catamorphism Apr 19, 2012

Contributor

Ah ok. Then I agree, not really visible to users, hence no need for an RFC.

Contributor

catamorphism commented Apr 19, 2012

Ah ok. Then I agree, not really visible to users, hence no need for an RFC.

@catamorphism

This comment has been minimized.

Show comment
Hide comment
@catamorphism

catamorphism Oct 1, 2012

Contributor

Moved to 0.4.

Contributor

catamorphism commented Oct 1, 2012

Moved to 0.4.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Oct 1, 2012

Contributor

@catamorphism I think the beginning work here has been done. The remaining work is probably better left to 0.5. Is there some specific change you had in mind for 0.4?

Contributor

nikomatsakis commented Oct 1, 2012

@catamorphism I think the beginning work here has been done. The remaining work is probably better left to 0.5. Is there some specific change you had in mind for 0.4?

@catamorphism

This comment has been minimized.

Show comment
Hide comment
@catamorphism

catamorphism Oct 1, 2012

Contributor

@pcwalton was saying that borrowing an @fn to a &fn results in a memory leak, and that that had something to do with this bug.

Contributor

catamorphism commented Oct 1, 2012

@pcwalton was saying that borrowing an @fn to a &fn results in a memory leak, and that that had something to do with this bug.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Oct 2, 2012

Contributor

That is true. I am not sure that we need to fix that for 0.4 though.

Contributor

nikomatsakis commented Oct 2, 2012

That is true. I am not sure that we need to fix that for 0.4 though.

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Nov 6, 2012

Cleanup how we handle proto in types, remove unsound subtyping
Fixes #1896 which was never truly fixed, just masked.
The given tests would have failed had they used `~fn()` and
not `@fn()`.  They now result in compilation errors.

Fixes #2978.

Necessary first step for #2202, #2263.

nikomatsakis added a commit that referenced this issue Nov 6, 2012

Cleanup how we handle proto in types, remove unsound subtyping
Fixes #1896 which was never truly fixed, just masked.
The given tests would have failed had they used `~fn()` and
not `@fn()`.  They now result in compilation errors.

Fixes #2978.

Necessary first step for #2202, #2263.
@brendanzab

This comment has been minimized.

Show comment
Hide comment
@brendanzab

brendanzab Feb 11, 2013

Member

A possible test:

// http://www.paulgraham.com/accgen.html

fn accgen<T:Copy + Num>(n: &a/mut T) -> @a/fn(T) -> T {
    fn@(i: T) -> T { *n += i; *n }
}

fn main() {
    let mut n = 0;
    let f = accgen(&mut n);
    // Should print "1, 2, 3"
    io::println(fmt!("%?, %?, %?", f(1), f(1), f(1)));
}

fn@(...) certainly looks odd. I'm guessing you were forced into that syntax? ie. @fn(...) was for deprecated functionality?

Member

brendanzab commented Feb 11, 2013

A possible test:

// http://www.paulgraham.com/accgen.html

fn accgen<T:Copy + Num>(n: &a/mut T) -> @a/fn(T) -> T {
    fn@(i: T) -> T { *n += i; *n }
}

fn main() {
    let mut n = 0;
    let f = accgen(&mut n);
    // Should print "1, 2, 3"
    io::println(fmt!("%?, %?, %?", f(1), f(1), f(1)));
}

fn@(...) certainly looks odd. I'm guessing you were forced into that syntax? ie. @fn(...) was for deprecated functionality?

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Mar 24, 2013

Contributor

Not critical for 0.6; removing milestone

Contributor

nikomatsakis commented Mar 24, 2013

Not critical for 0.6; removing milestone

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 7, 2013

Contributor

What remains to be done:

  • define closure hierarchy more precisely (#3696)
  • custom closure kind bounds (needed by #6308) (UPDATE: #3569)
  • change how region bounds are associated with closures and add tests (related to #6308)
Contributor

nikomatsakis commented May 7, 2013

What remains to be done:

  • define closure hierarchy more precisely (#3696)
  • custom closure kind bounds (needed by #6308) (UPDATE: #3569)
  • change how region bounds are associated with closures and add tests (related to #6308)
@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 10, 2013

Contributor

Oh, I forgot to add the biggest instance of work yet to be done. Making them sound! See this blog post for details:

http://smallcultfollowing.com/babysteps/blog/2013/04/30/the-case-of-the-recurring-closure/

This is blocked on #6298.

Contributor

nikomatsakis commented May 10, 2013

Oh, I forgot to add the biggest instance of work yet to be done. Making them sound! See this blog post for details:

http://smallcultfollowing.com/babysteps/blog/2013/04/30/the-case-of-the-recurring-closure/

This is blocked on #6298.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 16, 2013

Contributor

Repurposing this issue as a metabug.

Contributor

nikomatsakis commented May 16, 2013

Repurposing this issue as a metabug.

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue May 29, 2013

Move checking for moves and initialization of local variables and pat…
…terns into

borrow checker and generalize what moves are allowed. Fixes a nasty
bug or two in the pattern move checking code. Unifies dataflow code
used for initialization and other things. First step towards
once fns. Everybody wins.

Fixes #4384. Fixes #4715. cc once fns (#2202), optimizing local moves (#5016).

bors added a commit that referenced this issue May 29, 2013

auto merge of #6784 : nikomatsakis/rust/moves-into-borrowck, r=pcwalton
Move the computation of what data is moved out of `liveness` and into `borrowck`. The resulting code is cleaner, since before we had a split distribution of responsibilities, and also this avoids having multiple implementations of the dataflow code. Liveness is still used to report warnings about useless writes. This will go away when we get the control-flow graph code landed (working on that).

Also adds borrow checker documentation.

Fixes #4384.
Required to support once fns and to properly fix closures (#2202).
First step to generalize our treatment of moves somewhat as well.
@metajack

This comment has been minimized.

Show comment
Hide comment
@metajack

metajack Jul 11, 2013

Contributor

nominating backwards compat

Contributor

metajack commented Jul 11, 2013

nominating backwards compat

@graydon

This comment has been minimized.

Show comment
Hide comment
@graydon

graydon Jul 11, 2013

Contributor

accepted for backwards-compatible milestone

Contributor

graydon commented Jul 11, 2013

accepted for backwards-compatible milestone

@graydon

This comment has been minimized.

Show comment
Hide comment
@graydon

graydon Jul 11, 2013

Contributor

accepted for backwards-compatible milestone

Contributor

graydon commented Jul 11, 2013

accepted for backwards-compatible milestone

@metajack

This comment has been minimized.

Show comment
Hide comment
@metajack

metajack Sep 5, 2013

Contributor

visiting for triage. nothing to add.

Contributor

metajack commented Sep 5, 2013

visiting for triage. nothing to add.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 26, 2013

Contributor

Update: I have a branch that does a lot of the borowck work. Semi-blocked on fixing destructor semantics.

Contributor

nikomatsakis commented Sep 26, 2013

Update: I have a branch that does a lot of the borowck work. Semi-blocked on fixing destructor semantics.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Oct 8, 2013

Contributor

Current plan of action:

  • make ~fn once fns
  • rename &fn to |A| -> B
  • rename extern "Rust" to fn
  • rename ~fn -> proc
  • replace various do &fn use cases with RAII
  • remove do for &fn
  • add capture clauses??
  • note: procs should carry file/line info for debugging?
Contributor

brson commented Oct 8, 2013

Current plan of action:

  • make ~fn once fns
  • rename &fn to |A| -> B
  • rename extern "Rust" to fn
  • rename ~fn -> proc
  • replace various do &fn use cases with RAII
  • remove do for &fn
  • add capture clauses??
  • note: procs should carry file/line info for debugging?
@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Nov 29, 2013

Member

cc me

Member

pnkfelix commented Nov 29, 2013

cc me

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jan 16, 2014

Contributor

As part of this work, we need to update the kinds for function types to take into account the mutability etc of the captured values.

Contributor

nikomatsakis commented Jan 16, 2014

As part of this work, we need to update the kinds for function types to take into account the mutability etc of the captured values.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Feb 12, 2014

Contributor

Updated main comment.

Contributor

nikomatsakis commented Feb 12, 2014

Updated main comment.

@flaper87

This comment has been minimized.

Show comment
Hide comment
@flaper87

flaper87 Apr 19, 2014

Contributor

updated the description to add #13621

Contributor

flaper87 commented Apr 19, 2014

updated the description to add #13621

@pcwalton

This comment has been minimized.

Show comment
Hide comment
@pcwalton

pcwalton Jun 9, 2014

Contributor

I propose closing this as an outdated metabug that is currently not particularly actionable. We have several other bugs for unboxed closures.

Contributor

pcwalton commented Jun 9, 2014

I propose closing this as an outdated metabug that is currently not particularly actionable. We have several other bugs for unboxed closures.

@pcwalton pcwalton added the I-nominated label Jun 9, 2014

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Jun 12, 2014

Member

Closing in favor of new unboxed closures approach we are now adopting: see rust-lang/rfcs#114

Member

pnkfelix commented Jun 12, 2014

Closing in favor of new unboxed closures approach we are now adopting: see rust-lang/rfcs#114

@pnkfelix pnkfelix closed this Jun 12, 2014

@pnkfelix pnkfelix removed the I-nominated label Jun 12, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment