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

Unboxed closure metabug #18101

Closed
45 of 47 tasks
aturon opened this issue Oct 16, 2014 · 17 comments
Closed
45 of 47 tasks

Unboxed closure metabug #18101

aturon opened this issue Oct 16, 2014 · 17 comments
Labels
A-closures Area: Closures (`|…| { … }`) metabug Issues about issues themselves ("bugs about bugs")

Comments

@aturon
Copy link
Member

aturon commented Oct 16, 2014

This is a metabug for tracking work remaining to be done on unboxed closures. It contains only the high-level remaining issues. For a comprehensive list of closure bugs, search using the A-closures label.

1.0 Issues

Completed issues

@aturon aturon added the metabug Issues about issues themselves ("bugs about bugs") label Oct 16, 2014
@huonw huonw added the A-closures Area: Closures (`|…| { … }`) label Oct 16, 2014
@aturon
Copy link
Member Author

aturon commented Oct 16, 2014

cc @pcwalton @nikomatsakis

@japaric
Copy link
Member

japaric commented Nov 2, 2014

Re: #14798 -- Remove old (boxed) closures from the language

Are we going to replace all the uses of boxed closures in the standard library with unboxed closures? I think that in some cases it may not be doable (*) without abstract return types (ART). Let me explain why with a (rather long) story:

(*) Ok, it may be doable, but tedious. See alternatives section at the bottom:


I was navigating ancient issues today, and found #12677: "std::str::Bytes should implement Clone". So I decided to investigate why string.bytes() wasn't cloneable:

The bytes method returns an iter::Map which can't implement clone. This is the current definition of iter::Map:

#[deriving(Clone)]
struct Map<'a, A, B, I> {
    iter: I,
    f: |A|:'a -> B,  //~ error: type `|A| -> B` does not implement any method in scope named `clone`
}

So, my first attempt at a solution was: "Let's change Map to use unboxed closures":

#[deriving(Clone)]
struct Map<A, B, I: Iterator<A>, F: FnMut(A) -> B> {
    iter: I,
    f: F,
}

impl<A, B, I: Iterator<A>, F: FnMut(A) -> B> Iterator<B> for Map<A, B, I, F> {
    fn next(&mut self) -> Option<B> {
        self.iter.next().map(|elt| self.f.call_mut((elt,)))
    }
}

Which compiles fine, it's cloneable and should work in principle, but when I tried to replicate the bytes method, I found a more serious problem:

fn bytes<'a>(slice: &'a str) -> () {
    Map {
        iter: slice.as_bytes().iter(),
        f: |&mut: &b| b,
    }
}

I wrote () as the return type to make the compiler tell me what's the actual return type:

error: mismatched types: expected `()`, found `Map<_, _, core::slice::Items<'_, u8>, closure>` (expected (), found struct Map)

Note the 4th specialization argument of Map: closure. I realized that this function is impossible to define because the return type can't be "named", in particular the unboxed closure has an anonymous type.

If we had ART, then the return type could be Map<&'a u8, u8, slice::Items<'a, u8>, impl <'a> FnMut(&'a u8) -> u8, or more succinctly, just: impl Iterator<u8>.


Alternatives

Since we don't have ART, here are a few alternatives for the particular case of the bytes method:

  • Make bytes return a Bytes struct that implements Iterator<u8>. This basically bypasses the use of unboxed closures, buts add yet another struct to the library.
  • Create a DerefCopy struct that implements the general |:&mut &t| t operation, and change the return type of bytes to Map<_, _, _, DerefCopy>. The DerefCopy could be used in some other places of the stdlib.

@aturon This is relevant to the library stabilization process:

  • As part of the stabilization of std::iter, are we going to fully move to unboxed closures?
  • If yes, how do we dealt with these cases where the return type that can't be named?
  • Are we going to stabilize functions that return iterator adapters? Or perhaps should we mark them as [unstable = "return type may change"] to account for the introduction of ART in the future.

@aturon
Copy link
Member Author

aturon commented Nov 2, 2014

@japaric Note that you will be able to use "unboxed closures" in boxed form, something like: Box<Fn(uint) -> uint> for example. For library stabilization, we will be switching all APIs currently using closures to use the new closure system, but they will not necessarily all be boxed.

This is just the usual tradeoff between generics (with specialization) and trait objects (with dynamic dispatch). Right now, since we don't have abstract "unboxed" return types, anything returning a closure will have to use the trait object (boxed) form.

Put another way, "unboxed closures" is really an unfortunate misnomer. What's really changing is that closures are going away as a "special" thing, and instead being replaced by use of the existing trait system. That means you can choose to use them in both boxed and unboxed forms.

As we've been stabilizing APIs, we've been leaving anything that uses closures as #[unstable] for this reason. We'll be making a sweep later and determining which kinds of closures to use once the feature is ready.

@japaric
Copy link
Member

japaric commented Nov 2, 2014

Note that you will be able to use "unboxed closures" in boxed form, something like: Box<Fn(uint) -> uint> for example.

I'm aware that we can still use boxed closures, but it seemed a rather bad idea to use a boxed closure for the bytes method, hence I didn't listed it under the alternatives.

As we've been stabilizing APIs, we've been leaving anything that uses closures as #[unstable] for this reason.

Since we have to stabilize (as in mark as #[stable]) most of stdlib so it can be used on the stable/beta channels, does this mean well have to make tradeoffs like this one:

  • Make bytes return Map<_, _, _, Box<Fn(_) -> _>, which uses boxed closures and its bad for performance, or
  • Make bytes return Bytes (which would also need to be marked stable), where Bytes implements Iterator<u8>, which means we'll fill stdlib with more one use structs

right? (FWIW, I prefer the latter option wherever possible)

@alexcrichton
Copy link
Member

In this case Box<Fn(..)> may not be that bad because a 0-sized allocation (there's no environment for most of these functions) costs nothing. We may also be able to get away with &'static Fn(..) depending on how much trans wants to cooperate (in theory).

@japaric
Copy link
Member

japaric commented Nov 2, 2014

In this case Box<Fn(..)> may not be that bad because a 0-sized allocation (there's no environment for most of these functions) costs nothing.

Doesn't each call still has to go through the trait object tough? Or is LLVM able to optimize it to a function call (or maybe even better: inline it away)?

@alexcrichton
Copy link
Member

Yes, but that's precisely what happens today (relying on LLVM to inline), as in all closures are boxed today.

@TeXitoi
Copy link
Contributor

TeXitoi commented Nov 3, 2014

At worst, if you want to return an unboxed closure, you can write the struct by hand and not rely on the syntaxic sugar. It correspond to your DerefCopy alternative. And you can write polymorphic closure! http://is.gd/iqdHfp

@cristicbz
Copy link
Contributor

Found two more ICE-s today: #18652 (to do with move capture) and #18661 (to do with passing Fn-s as args).

@cristicbz
Copy link
Contributor

A whole bunch of these bugs have been fixed:

ICE-s
#16790
#16774
#18378
#18453

Other bugs
#17703

@carllerche
Copy link
Member

Infer move using Send trait: #18799

@abonander
Copy link
Contributor

Can check off #17661. HRTB landed today.

@abonander
Copy link
Contributor

Just opened #19135, related to HRTBs and lifetimes on unboxed closures.

@alexchandel
Copy link

An ICE related to rust-call: #16039

@tomaka
Copy link
Contributor

tomaka commented Nov 30, 2014

Also #19374

(this is very selfish, but the glium library will become unusable if #19338 lands before #19374 is fixed)

@bstrie
Copy link
Contributor

bstrie commented Dec 5, 2014

#19575

@steveklabnik
Copy link
Member

Given that only two issues remain, I don't think this metabug is particularly valuable anymore, so I'm gonna close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: Closures (`|…| { … }`) metabug Issues about issues themselves ("bugs about bugs")
Projects
None yet
Development

No branches or pull requests