Skip to content

Commit

Permalink
auto merge of rust-lang#19467 : japaric/rust/uc, r=alexcrichton
Browse files Browse the repository at this point in the history
This PR moves almost all our current uses of closures, both in public API and internal uses, to the new "unboxed" closures system.

In most cases, downstream code that *only uses* closures will continue to work as it is. The reason is that the `|| {}` syntax can be inferred either as a boxed or an "unboxed" closure according to the context. For example the following code will continue to work:

``` rust
some_option.map(|x| x.transform_with(upvar))
```

And will get silently upgraded to an "unboxed" closure.

In some other cases, it may be necessary to "annotate" which `Fn*` trait the closure implements:

```
// Change this
|x| { /* body */}
// to either of these
|: x| { /* body */}  // closure implements the FnOnce trait
|&mut : x| { /* body */}  // FnMut
|&: x| { /* body */}  // Fn
```

This mainly occurs when the closure is assigned to a variable first, and then passed to a function/method.

``` rust
let closure = |: x| x.transform_with(upvar);
some.option.map(closure)
```

(It's very likely that in the future, an improved inference engine will make this annotation unnecessary)

Other cases that require annotation are closures that implement some trait via a blanket `impl`, for example:

- `std::finally::Finally`
- `regex::Replacer`
- `std::str::CharEq`

``` rust
string.trim_left_chars(|c: char| c.is_whitespace())
//~^ ERROR: the trait `Fn<(char,), bool>` is not implemented for the type `|char| -> bool`
string.trim_left_chars(|&: c: char| c.is_whitespace())  // OK
```

Finally, all implementations of traits that contain boxed closures in the arguments of their methods are now broken. And will need to be updated to use unboxed closures. These are the main affected traits:

- `serialize::Decoder`
- `serialize::DecoderHelpers`
- `serialize::Encoder`
- `serialize::EncoderHelpers`
- `rustrt::ToCStr`

For example, change this:

``` rust
// libserialize/json.rs
impl<'a> Encoder<io::IoError> for Encoder<'a> {
    fn emit_enum(&mut self,
                 _name: &str,
                 f: |&mut Encoder<'a>| -> EncodeResult) -> EncodeResult {
        f(self)
    }
}
```

to:

``` rust
// libserialize/json.rs
impl<'a> Encoder<io::IoError> for Encoder<'a> {
    fn emit_enum<F>(&mut self, _name: &str, f: F) -> EncodeResult where
        F: FnOnce(&mut Encoder<'a>) -> EncodeResult
    {
        f(self)
    }
}
```

[breaking-change]

---

### How the `Fn*` bound has been selected

I've chosen the bounds to make the functions/structs as "generic as possible", i.e. to let them allow the maximum amount of input.

- An `F: FnOnce` bound accepts the three kinds of closures: `|:|`, `|&mut:|` and `|&:|`.
- An `F: FnMut` bound only accepts "non-consuming" closures: `|&mut:|` and `|&:|`.
- An `F: Fn` bound only accept the "immutable environment" closures: `|&:|`.

This means that whenever possible the `FnOnce` bound has been used, if the `FnOnce` bound couldn't be used, then the `FnMut` was used. The `Fn` bound was never used in the whole repository.

The `FnMut` bound was the most used, because it resembles the semantics of the current boxed closures: the closure can modify its environment, and the closure may be called several times.

The `FnOnce` bound allows new semantics: you can move out the upvars when the closure is called. This can be effectively paired with the `move || {}` syntax to transfer ownership from the environment to the closure caller.

In the case of trait methods, is hard to select the "right" bound since we can't control how the trait may be implemented by downstream users. In these cases, I have selected the bound based on how we use these traits in the repository. For this reason the selected bounds may not be ideal, and may require tweaking before stabilization.

r? @aturon
  • Loading branch information
bors committed Dec 13, 2014
2 parents 567b90f + b8e0b81 commit 444fa1b
Show file tree
Hide file tree
Showing 194 changed files with 2,534 additions and 1,761 deletions.
9 changes: 5 additions & 4 deletions src/libarena/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
html_root_url = "http://doc.rust-lang.org/nightly/")]

#![feature(unsafe_destructor)]
#![feature(unboxed_closures)]
#![allow(missing_docs)]

extern crate alloc;
Expand Down Expand Up @@ -209,7 +210,7 @@ impl Arena {
}

#[inline]
fn alloc_copy<T>(&self, op: || -> T) -> &mut T {
fn alloc_copy<T, F>(&self, op: F) -> &mut T where F: FnOnce() -> T {
unsafe {
let ptr = self.alloc_copy_inner(mem::size_of::<T>(),
mem::min_align_of::<T>());
Expand Down Expand Up @@ -263,7 +264,7 @@ impl Arena {
}

#[inline]
fn alloc_noncopy<T>(&self, op: || -> T) -> &mut T {
fn alloc_noncopy<T, F>(&self, op: F) -> &mut T where F: FnOnce() -> T {
unsafe {
let tydesc = get_tydesc::<T>();
let (ty_ptr, ptr) =
Expand All @@ -287,7 +288,7 @@ impl Arena {
/// Allocates a new item in the arena, using `op` to initialize the value,
/// and returns a reference to it.
#[inline]
pub fn alloc<T>(&self, op: || -> T) -> &mut T {
pub fn alloc<T, F>(&self, op: F) -> &mut T where F: FnOnce() -> T {
unsafe {
if intrinsics::needs_drop::<T>() {
self.alloc_noncopy(op)
Expand Down Expand Up @@ -339,7 +340,7 @@ fn test_arena_destructors_fail() {
arena.alloc(|| { [0u8, 1u8, 2u8] });
}
// Now, panic while allocating
arena.alloc::<Rc<int>>(|| {
arena.alloc::<Rc<int>, _>(|| {
panic!();
});
}
Expand Down
44 changes: 32 additions & 12 deletions src/libcollections/bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,14 @@ use std::rand;
use std::rand::Rng;
use test::Bencher;

pub fn insert_rand_n<M>(n: uint, map: &mut M, b: &mut Bencher,
insert: |&mut M, uint|,
remove: |&mut M, uint|) {
pub fn insert_rand_n<M, I, R>(n: uint,
map: &mut M,
b: &mut Bencher,
mut insert: I,
mut remove: R) where
I: FnMut(&mut M, uint),
R: FnMut(&mut M, uint),
{
// setup
let mut rng = rand::weak_rng();

Expand All @@ -31,9 +36,14 @@ pub fn insert_rand_n<M>(n: uint, map: &mut M, b: &mut Bencher,
})
}

pub fn insert_seq_n<M>(n: uint, map: &mut M, b: &mut Bencher,
insert: |&mut M, uint|,
remove: |&mut M, uint|) {
pub fn insert_seq_n<M, I, R>(n: uint,
map: &mut M,
b: &mut Bencher,
mut insert: I,
mut remove: R) where
I: FnMut(&mut M, uint),
R: FnMut(&mut M, uint),
{
// setup
for i in range(0u, n) {
insert(map, i * 2);
Expand All @@ -48,9 +58,14 @@ pub fn insert_seq_n<M>(n: uint, map: &mut M, b: &mut Bencher,
})
}

pub fn find_rand_n<M, T>(n: uint, map: &mut M, b: &mut Bencher,
insert: |&mut M, uint|,
find: |&M, uint| -> T) {
pub fn find_rand_n<M, T, I, F>(n: uint,
map: &mut M,
b: &mut Bencher,
mut insert: I,
mut find: F) where
I: FnMut(&mut M, uint),
F: FnMut(&M, uint) -> T,
{
// setup
let mut rng = rand::weak_rng();
let mut keys = Vec::from_fn(n, |_| rng.gen::<uint>() % n);
Expand All @@ -70,9 +85,14 @@ pub fn find_rand_n<M, T>(n: uint, map: &mut M, b: &mut Bencher,
})
}

pub fn find_seq_n<M, T>(n: uint, map: &mut M, b: &mut Bencher,
insert: |&mut M, uint|,
find: |&M, uint| -> T) {
pub fn find_seq_n<M, T, I, F>(n: uint,
map: &mut M,
b: &mut Bencher,
mut insert: I,
mut find: F) where
I: FnMut(&mut M, uint),
F: FnMut(&M, uint) -> T,
{
// setup
for i in range(0u, n) {
insert(map, i);
Expand Down
24 changes: 16 additions & 8 deletions src/libcollections/bit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ impl<'a> Iterator<(uint, u32)> for MaskWords<'a> {

impl Bitv {
#[inline]
fn process(&mut self, other: &Bitv, op: |u32, u32| -> u32) -> bool {
fn process<F>(&mut self, other: &Bitv, mut op: F) -> bool where F: FnMut(u32, u32) -> u32 {
let len = other.storage.len();
assert_eq!(self.storage.len(), len);
let mut changed = false;
Expand Down Expand Up @@ -816,7 +816,7 @@ pub fn from_bytes(bytes: &[u8]) -> Bitv {
/// let bv = from_fn(5, |i| { i % 2 == 0 });
/// assert!(bv.eq_vec(&[true, false, true, false, true]));
/// ```
pub fn from_fn(len: uint, f: |index: uint| -> bool) -> Bitv {
pub fn from_fn<F>(len: uint, mut f: F) -> Bitv where F: FnMut(uint) -> bool {
let mut bitv = Bitv::with_capacity(len, false);
for i in range(0u, len) {
bitv.set(i, f(i));
Expand Down Expand Up @@ -1182,7 +1182,7 @@ impl BitvSet {
}

#[inline]
fn other_op(&mut self, other: &BitvSet, f: |u32, u32| -> u32) {
fn other_op<F>(&mut self, other: &BitvSet, mut f: F) where F: FnMut(u32, u32) -> u32 {
// Expand the vector if necessary
self.reserve(other.capacity());

Expand Down Expand Up @@ -1277,10 +1277,12 @@ impl BitvSet {
#[inline]
#[unstable = "matches collection reform specification, waiting for dust to settle"]
pub fn union<'a>(&'a self, other: &'a BitvSet) -> TwoBitPositions<'a> {
fn or(w1: u32, w2: u32) -> u32 { w1 | w2 }

TwoBitPositions {
set: self,
other: other,
merge: |w1, w2| w1 | w2,
merge: or,
current_word: 0u32,
next_idx: 0u
}
Expand All @@ -1306,11 +1308,13 @@ impl BitvSet {
#[inline]
#[unstable = "matches collection reform specification, waiting for dust to settle"]
pub fn intersection<'a>(&'a self, other: &'a BitvSet) -> Take<TwoBitPositions<'a>> {
fn bitand(w1: u32, w2: u32) -> u32 { w1 & w2 }

let min = cmp::min(self.capacity(), other.capacity());
TwoBitPositions {
set: self,
other: other,
merge: |w1, w2| w1 & w2,
merge: bitand,
current_word: 0u32,
next_idx: 0
}.take(min)
Expand Down Expand Up @@ -1343,10 +1347,12 @@ impl BitvSet {
#[inline]
#[unstable = "matches collection reform specification, waiting for dust to settle"]
pub fn difference<'a>(&'a self, other: &'a BitvSet) -> TwoBitPositions<'a> {
fn diff(w1: u32, w2: u32) -> u32 { w1 & !w2 }

TwoBitPositions {
set: self,
other: other,
merge: |w1, w2| w1 & !w2,
merge: diff,
current_word: 0u32,
next_idx: 0
}
Expand All @@ -1373,10 +1379,12 @@ impl BitvSet {
#[inline]
#[unstable = "matches collection reform specification, waiting for dust to settle"]
pub fn symmetric_difference<'a>(&'a self, other: &'a BitvSet) -> TwoBitPositions<'a> {
fn bitxor(w1: u32, w2: u32) -> u32 { w1 ^ w2 }

TwoBitPositions {
set: self,
other: other,
merge: |w1, w2| w1 ^ w2,
merge: bitxor,
current_word: 0u32,
next_idx: 0
}
Expand Down Expand Up @@ -1614,7 +1622,7 @@ pub struct BitPositions<'a> {
pub struct TwoBitPositions<'a> {
set: &'a BitvSet,
other: &'a BitvSet,
merge: |u32, u32|: 'a -> u32,
merge: fn(u32, u32) -> u32,
current_word: u32,
next_idx: uint
}
Expand Down
14 changes: 10 additions & 4 deletions src/libcollections/btree/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,12 @@ pub struct MoveEntries<K, V> {
}

/// An iterator over a BTreeMap's keys.
pub type Keys<'a, K, V> = iter::Map<'static, (&'a K, &'a V), &'a K, Entries<'a, K, V>>;
pub type Keys<'a, K, V> =
iter::Map<(&'a K, &'a V), &'a K, Entries<'a, K, V>, fn((&'a K, &'a V)) -> &'a K>;

/// An iterator over a BTreeMap's values.
pub type Values<'a, K, V> = iter::Map<'static, (&'a K, &'a V), &'a V, Entries<'a, K, V>>;
pub type Values<'a, K, V> =
iter::Map<(&'a K, &'a V), &'a V, Entries<'a, K, V>, fn((&'a K, &'a V)) -> &'a V>;

/// A view into a single entry in a map, which may either be vacant or occupied.
pub enum Entry<'a, K:'a, V:'a> {
Expand Down Expand Up @@ -1207,7 +1209,9 @@ impl<K, V> BTreeMap<K, V> {
/// ```
#[unstable = "matches collection reform specification, waiting for dust to settle"]
pub fn keys<'a>(&'a self) -> Keys<'a, K, V> {
self.iter().map(|(k, _)| k)
fn first<A, B>((a, _): (A, B)) -> A { a }

self.iter().map(first)
}

/// Gets an iterator over the values of the map.
Expand All @@ -1226,7 +1230,9 @@ impl<K, V> BTreeMap<K, V> {
/// ```
#[unstable = "matches collection reform specification, waiting for dust to settle"]
pub fn values<'a>(&'a self) -> Values<'a, K, V> {
self.iter().map(|(_, v)| v)
fn second<A, B>((_, b): (A, B)) -> B { b }

self.iter().map(second)
}

/// Return the number of elements in the map.
Expand Down
34 changes: 23 additions & 11 deletions src/libcollections/btree/set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ pub struct BTreeSet<T>{
pub type Items<'a, T> = Keys<'a, T, ()>;

/// An owning iterator over a BTreeSet's items.
pub type MoveItems<T> = iter::Map<'static, (T, ()), T, MoveEntries<T, ()>>;
pub type MoveItems<T> =
iter::Map<(T, ()), T, MoveEntries<T, ()>, fn((T, ())) -> T>;

/// A lazy iterator producing elements in the set difference (in-order).
pub struct DifferenceItems<'a, T:'a> {
Expand Down Expand Up @@ -87,7 +88,9 @@ impl<T> BTreeSet<T> {
/// Gets an iterator for moving out the BtreeSet's contents.
#[unstable = "matches collection reform specification, waiting for dust to settle"]
pub fn into_iter(self) -> MoveItems<T> {
self.map.into_iter().map(|(k, _)| k)
fn first<A, B>((a, _): (A, B)) -> A { a }

self.map.into_iter().map(first)
}
}

Expand Down Expand Up @@ -600,22 +603,31 @@ mod test {
assert!(hash::hash(&x) == hash::hash(&y));
}

fn check(a: &[int],
b: &[int],
expected: &[int],
f: |&BTreeSet<int>, &BTreeSet<int>, f: |&int| -> bool| -> bool) {
struct Counter<'a, 'b> {
i: &'a mut uint,
expected: &'b [int],
}

impl<'a, 'b> FnMut(&int) -> bool for Counter<'a, 'b> {
extern "rust-call" fn call_mut(&mut self, (&x,): (&int,)) -> bool {
assert_eq!(x, self.expected[*self.i]);
*self.i += 1;
true
}
}

fn check<F>(a: &[int], b: &[int], expected: &[int], f: F) where
// FIXME Replace Counter with `Box<FnMut(_) -> _>`
F: FnOnce(&BTreeSet<int>, &BTreeSet<int>, Counter) -> bool,
{
let mut set_a = BTreeSet::new();
let mut set_b = BTreeSet::new();

for x in a.iter() { assert!(set_a.insert(*x)) }
for y in b.iter() { assert!(set_b.insert(*y)) }

let mut i = 0;
f(&set_a, &set_b, |x| {
assert_eq!(*x, expected[i]);
i += 1;
true
});
f(&set_a, &set_b, Counter { i: &mut i, expected: expected });
assert_eq!(i, expected.len());
}

Expand Down
20 changes: 9 additions & 11 deletions src/libcollections/dlist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -351,18 +351,16 @@ impl<T> DList<T> {
/// println!("{}", e); // prints 2, then 4, then 11, then 7, then 8
/// }
/// ```
pub fn insert_when(&mut self, elt: T, f: |&T, &T| -> bool) {
{
let mut it = self.iter_mut();
loop {
match it.peek_next() {
None => break,
Some(x) => if f(x, &elt) { break }
}
it.next();
pub fn insert_when<F>(&mut self, elt: T, mut f: F) where F: FnMut(&T, &T) -> bool {
let mut it = self.iter_mut();
loop {
match it.peek_next() {
None => break,
Some(x) => if f(x, &elt) { break }
}
it.insert_next(elt);
it.next();
}
it.insert_next(elt);
}

/// Merges `other` into this `DList`, using the function `f`.
Expand All @@ -371,7 +369,7 @@ impl<T> DList<T> {
/// put `a` in the result if `f(a, b)` is true, and otherwise `b`.
///
/// This operation should compute in O(max(N, M)) time.
pub fn merge(&mut self, mut other: DList<T>, f: |&T, &T| -> bool) {
pub fn merge<F>(&mut self, mut other: DList<T>, mut f: F) where F: FnMut(&T, &T) -> bool {
{
let mut it = self.iter_mut();
loop {
Expand Down
Loading

0 comments on commit 444fa1b

Please sign in to comment.