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

Library modernization blockers #19186

Closed
7 of 8 tasks
japaric opened this issue Nov 21, 2014 · 9 comments
Closed
7 of 8 tasks

Library modernization blockers #19186

japaric opened this issue Nov 21, 2014 · 9 comments
Labels
metabug Issues about issues themselves ("bugs about bugs")

Comments

@japaric
Copy link
Member

japaric commented Nov 21, 2014

This is a list of the blockers (ICEs/bugs) that I found while attempting to modernize the standard library with features like unboxed closures and associated types. This list serves two purposes:

Unboxed closures

-> Move library code away from boxed closures (part of #14798)

In function arguments

Option, Result, etc

// from
fn map<U>(self,  f: |T| -> U) -> Option<U>;
// to
fn map<U, F: FnOnce(T) -> U>(self,  f: F) -> Option<U>;

Blockers

In structs fields

core::iter::Map

struct Map<A, B, I, F> where I: Iterator<A>, F: FnMut(A) -> B {
    iter: I,
    f: F,
}

Main issue here is that closures have anonymous types, so they can't be part of the signature of functions, structs, etc. See #19186 (comment)
and/or #18101 (comment) for more details.

Use bare functions instead of closures that capture nothing

core::str::Bytes, core::str::AnyLines
collections::btree::Keys, collections::hash_map::Values

As pointed by @sfackler: If the closure captures nothing, we can use a bare function instead.

type Bytes<'a> = Map<&'a u8, u8, slice::Items<'a, u8>, fn(&u8) -> u8>;

// Self = str
fn bytes(&self) -> Bytes {
    fn deref(&b: &u8) -> u8 { b }
    self.as_bytes().iter().map(deref)
}

Blockers

Use boxed closures otherwise

Add examples here

Blockers

Associated types

-> Update library code for associated output types #17826

There are several issues in this area, so I'll list the blockers in increasing complexity order:

Extension traits with only output types

AdditiveIterator, MultiplicativeIterator, etc

Blockers

Traits with only output types that are used for GP

AsSlice, Hasher, etc

Blockers

Traits with input and output types

Add, Mul, Sub, etc

Blockers


It's likely that we'll find more blockers as we progress, so I'll try to keep this list up to date.

cc @alexcrichton @aturon @nick29581 @nikomatsakis

@aturon
Copy link
Member

aturon commented Nov 21, 2014

@japaric Thanks so much for putting this together!

@aturon aturon added metabug Issues about issues themselves ("bugs about bugs") A-libs labels Nov 21, 2014
@japaric
Copy link
Member Author

japaric commented Nov 21, 2014

Added #18048 (ICE when cross-crate importing a trait that has associated output types) to the list, which blocks any work on associated types.

@aturon
Copy link
Member

aturon commented Nov 22, 2014

cc @nikomatsakis @nick29581

@japaric
Copy link
Member Author

japaric commented Nov 25, 2014

I was exploring using unboxed closures in structs like core::iter::Map, and I hit the bytes()/Bytes issue that I've mentioned before. I explored the alternatives suggested in that thread, but turns out I can't really use any of them. Let me elaborate:

  • We want to make Map use unboxed closures like this:
struct Map<A, B, I, F> where I: Iterator<A>, F: FnMut(A) -> B {
    iter: I,
    f: F,
}
  • Currently, the StrPrelude::bytes() method returns Bytes which is a type alias to Map. This is how the method looks like:
// Self = str
fn bytes<'a>(&'a self) -> Bytes<'a> {
    self.as_bytes().iter().map(|&b| b)
}
  • The issue: The bytes method can't be written like that with the "unboxed" version of Map. The reason is that Bytes would have to be an alias to Map<&'a u8, u8, slice::Items<'a, u8>, ???>, where ??? is the type of the unboxed closure. But since unboxed closures have anonymous types, that return type can't be "written down".

These are the suggestions (proposed in the other thread) to work around the issue:

  • Use a boxed closure Box<FnMut(..) -> _> instead of ???: This is not possible in this case, because we're dealing with libcore, so there is no Box here.
  • Use &'static FnMut(..) -> _ instead of ???: AFAIK, it's not currently possible to create a value with that type. (Do we plan to support that in the future?)
  • Use some Struct instead of ??? where Struct: FnMut(..) -> _: Because of Unable to manually implement FnOnce #18835/Coherence and blanket impls interact in a suboptimal fashion with generic impls #19032, is not currently possible to do impl FnMut(..) -> _ for Struct.
  • Make Bytes a struct where Bytes: Iterator<u8>, and make StrPrelude::bytes return that struct. This literally means that we won't use closures at all.

StrPrelude::bytes()/Bytes is not the only occurrence. There is also StrPrelude::lines_any()/AnyLines in the same module, and there are probably other cases with other iterator adaptors and in other crates.

My question here is: On what issue should we block "Using unboxed closures in structs field"?

I'll explore this last option, because I want to see if any other bug/ICE pops up in the process.

@japaric
Copy link
Member Author

japaric commented Nov 25, 2014

I'll explore this last option, because I want to see if any other bug/ICE pops up in the process.

Just found out that there are a lot of type aliases to Map in libcollections, that will need to be updated to work with unboxed closures. Namely, all the Keys/Values/MoveItems aliases in each module of libcollections, i.e. btree::Keys, hash_map::Values, etc.

Since libcollections has access to liballoc, I tried to use the Box<FnMut(..) -> _> approach, but the Box needs a lifetime specifier (like Box<FnMut(..) -> _ + 'static), and that doesn't currently parses because of #18772. Thankfully, @nikomatsakis already sent PR #19298 fixing the issue!

I'll update the top comment to mention that issue, because we want it to get snapshot-ed.

@sfackler
Copy link
Member

Don't functions implement the Fn traits? We might be able to work around some of these issues by using them instead of the closure sugar.

@japaric
Copy link
Member Author

japaric commented Nov 25, 2014

Don't functions implement the Fn traits?

Yes! And it almost works:

#![feature(unboxed_closures)]

use std::{iter, slice};

struct Map<A, B, I, F> where I: Iterator<A>, F: FnMut(A) -> B {
    iter: I,
    f: F,
}

fn bytes<'a>(s: &'a str) -> Map<&'a u8, u8, slice::Items<'a, u8>, fn(&u8) -> u8> {
    //~^ error: the trait `core::ops::Fn<(&'a u8,), u8>` is not implemented for the type `fn(&u8) -> u8`
    fn deref(&b: &u8) -> u8 { b }

    Map {
        iter: s.as_bytes().iter(),
        f: deref,
    }
}

fn this_works() -> Map<u8, uint, iter::Range<u8>, fn(u8) -> uint> {
    fn cast(i: u8) -> uint { i as uint }

    Map {
        iter: range(0, 5),
        f: cast,
    }
}

fn main() {}

Except that due to #19126, bare functions that have lifetimes in their arguments/return value currently don't implement the Fn traits. Still, it's a great idea, I'll add it the top comment. Thanks @sfackler!

@AndyShiue
Copy link

Can this be closed?

@japaric
Copy link
Member Author

japaric commented Jan 2, 2015

Can this be closed?

Yes. There are still blockers (mainly for assoc types) but I'm working with @nikomatsakis in a tight feedback loop (on IRC) so this issue is not relevant anymore.

@japaric japaric closed this as completed Jan 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metabug Issues about issues themselves ("bugs about bugs")
Projects
None yet
Development

No branches or pull requests

4 participants