Copy/Clone Closures

[cramertj]

Implement Clone and Copy for closures where possible:

// Many closures can now be passed by-value to multiple functions:
fn call<F: FnOnce()>(f: F) { f() }
let hello = || println!("Hello, world!");
call(hello);
call(hello);

// Many `Iterator` combinators are now `Copy`/`Clone`:
let x = (1..100).map(|x| x * 5);
let _ = x.map(|x| x - 3); // moves `x` by `Copy`ing
let _ = x.chain(y); // moves `x` again
let _ = x.cycle(); // `.cycle()` is only possible when `Self: Clone`

// Closures which reference data mutably are not `Copy`/`Clone`:
let mut x = 0;
let incr_x = || x += 1;
call(incr_x);
call(incr_x); // ERROR: `incr_x` moved in the call above.

// `move` closures implement `Clone`/`Copy` if the values they capture
// implement `Clone`/`Copy`:
let mut x = 0;
let print_incr = move || { println!("{}", x); x += 1; };

fn call_three_times<F: FnMut()>(mut f: F) {
    for i in 0..3 {
        f();
    }
}

call_three_times(print_incr); // prints "0", "1", "2"
call_three_times(print_incr); // prints "0", "1", "2"

Rendered

Note: this RFC was made possible by the recent change to make Clone a lang item.

[eddyb]

FWIW I keep seeing people refer to that change as "making Clone a lang item" but that is neither sufficient nor necessary (if you really wanted to you could just extract it from Copy's only supertrait).

What happened was automagic Clone::clone shimming in the compiler, like syntactic deriving but done on semantic type information and producing MIR, which makes it more powerful.

[kennytm]

Does this include function types and function pointers as well? Currently Clone is manually implemented for:

• fn(A, B, C, D, E, F, G, H, I, J, K, L) -> Ret
• unsafe fn(A, B, C, D, E, F, G, H, I, J, K, L) -> Ret
• extern "C" fn(A, B, C, D, E, F, G, H, I, J, K, L) -> Ret
• extern "C" fn(A, B, C, D, E, F, G, H, I, J, K, L, ...) -> Ret
• unsafe extern "C" fn(A, B, C, D, E, F, G, H, I, J, K, L) -> Ret
• unsafe extern "C" fn(A, B, C, D, E, F, G, H, I, J, K, L, ...) -> Ret

[cramertj]

@kennytm This RFC as written doesn't cover those, but I think those impls should definitely added. I think, though, that they probably belong in #2133 rather than this RFC.

[eddyb]

@cramertj Can you update the description to refer to #2133 instead?

[kennytm]

How will this interact with generators (#2033)?

[cramertj]

@kennytm I'd like to think that generators would follow the same rules, but as no formal RFC for generators has been merged, I think that's outside the scope of this RFC.

[aturon]

This is a pretty straightfoward and very welcome RFC (eliminating a small but painful papercut). I'm going to go ahead and get the wheels turning toward FCP review:

@rfcbot fcp merge

[rfcbot]

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

• @aturon
• @eddyb
• @nikomatsakis
• @nrc
• @pnkfelix
• @withoutboats

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[nikomatsakis]

I remember being sort of happy in the implementation that closures were just never Copy; I'm trying to think precisely why. But I guess if there's a big problem it will surface during the implementation. Probably it just let me cut some corner or other.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[SimonSapin]

@nikomatsakis Is it because it could be confusing to accidentally make a copy, resume the copy, and not see side effects on the original? (Same as iterators.) Maybe it makes sense to deliberately make generators not Copy, but they should definitely be Clone whenever possible.

[nikomatsakis]

@SimonSapin Hmm. I don't think that issue is specific to generators per se, it applies to any closure that owns mutable state. (After all, at a deep level at least, a generator is just a function.) Example:

fn main() {
    let mut i = 0;
    let mut get_value = move || -> u32 { i += 1; return i; }; 
    println!("{}", get_value()); // prints 1
    let mut get_value2 = get_value; // currently errors, but wouldn't with this RFC.
    println!("{}", get_value()); // prints 2
    println!("{}", get_value2()); // prints 1
}

I'm not sure if you consider surprising, or just expected behavior. It certainly behaves the same as any other value would, but it depends on how precise your "mental model" is for what state is owned by the closure versus being referenced from the surrounding stack frame, I suppose.

[cramertj]

@SimonSapin I have an example of this in the RFC-- check out how call_three_times is used in the summary.

[ExpHP]

Is it because it could be confusing to accidentally make a copy, resume the copy, and not see side effects on the original? (Same as iterators.) Maybe it makes sense to deliberately make generators not Copy, but they should definitely be Clone whenever possible.

IIRC, this is basically the reason Range isn't Copy.

...ah, in fact, I think we're both remembering the same discussion.

[KamilaBorowska]

Considering Range is not Copy, would it be feasible to implement Copy only when closure implements Fn (i.e. doesn't mutate anything). The table of possible autotraits would look like this.

Closure trait	Can automatically implement
Fn	Copy
FnMut	Clone
FnOnce	none

Note that this still requires that objects contained in closure are copyable/cloneable. For instance a closure owning &mut T is never cloneable, so this is not an issue - for all practical purposes, to have a closure implementing only FnMut and FnOnce that could be cloned with those rules, it's required to use move keyword.

Also out of curiosity I consider the behaviour of call_three_times with &Cell instead of moved variable, and it seems reasonable to me even if a bit confusing - it returns 0, 1, 2, 3, 4, 5 - matching the behaviour of reference cell.

Here is a reference version to be called with call_three_times:

let cell = Cell::new(0);
let print_incr = &|| {
    println!("{}", cell.get());
    cell.set(cell.get() + 1);
};

And here is Copy version.

#![feature(fn_traits, unboxed_closures)]

use std::cell::Cell;

#[derive(Copy, Clone)]
struct Incrementer<'a> {
    cell: &'a Cell<i32>,
}

impl<'a> Fn<()> for Incrementer<'a> {
    extern "rust-call" fn call(&self, _args: ()) {
        let cell = self.cell;
        println!("{}", cell.get());
        cell.set(cell.get() + 1)
    }
}

fn main() {
    let cell = Cell::new(0);
    let incrementer = Incrementer { cell: &cell };

    fn call_three_times<F: FnMut()>(mut f: F) {
        for _ in 0..3 {
            f();
        }
    }

    call_three_times(incrementer);
    call_three_times(incrementer);
}

// Noise needed for technical reasons
impl<'a> FnMut<()> for Incrementer<'a> {
    extern "rust-call" fn call_mut(&mut self, args: ()) {
        self.call(args)
    }
}

impl<'a> FnOnce<()> for Incrementer<'a> {
    type Output = ();
    extern "rust-call" fn call_once(self, args: ()) {
        self.call(args)
    }
}

[nikomatsakis]

It's a nit pick, but -- strictly speaking -- this is not always true. A non-move closure can take ownership of values that it (in turn) moves, and hence might not be copy or clone (if those values are not copy or clone).

[nikomatsakis]

(Notably, those closures would be FnOnce.)

[nikomatsakis]

This is relatively easy. I don't think it has to be considered an unresolved question. However, I do think it would be valuable to give an example of the sort of message you would expect. =)

[nikomatsakis]

I think I would propose separately feature-gating "Copy" and "Clone" on closures -- in particular, I don't think there is much objection to closures being clone, but it'd be interesting to see if copy closures cause any kinds of problems.

But really I think a lot of these concerns come back to the desire to have a kind of "Copy" lint that warns you when you are implicitly copying values that you may not want to implicitly copy. (Which could then be silenced by invoking clone() explicitly.) We've always planned to have it, but never done it.

[rfcbot]

The final comment period is now complete.

[aturon]

This RFC has been merged! Tracking issue.

[dead10ck]

Forgive me if I'm just totally wrong here, but wouldn't this break boxed closures? I've had issues before with not being able to box a trait because it implemented Clone.

[kennytm]

@dead10ck Do you have any code the reproduce the error? ("not being able to box a trait because it implemented Clone")

[sfackler]

@dead10ck Clone isn't object safe itself, but it's fine to have a type that implements Clone and some object safe trait like Fn turn into a Fn trait object.

[dead10ck]

@kennytm yeah, e.g. https://play.rust-lang.org/?gist=cc5e61ee6a37a21dae1e145911bb620f&version=stable

@sfackler right, but if I'm understanding this RFC proposal, wouldn't it make Fn: Clone, which would make it object-unsafe?

[kennytm]

@dead10ck Ok, the problem in your code is that Foo has a supertrait Clone. The proposal doesn't make Fn: Clone, but that some instances of Fn now implements Clone. There can still be non-Clone closures, like what OP demonstrated:

// Closures which reference data mutably are not `Copy`/`Clone`:
let mut x = 0;
let incr_x = || x += 1;
call(incr_x);
call(incr_x); // ERROR: `incr_x` moved in the call above.

[dead10ck]

@kennytm I see, so it's not a blanket impl; but what about the cases that do : Clone? The ones from this case, for example, couldn't be boxed, right? And consequently, we wouldn't be able to return them from a function.

// Many `Iterator` combinators are now `Copy`/`Clone`:
let x = (1..100).map(|x| x * 5);
let _ = x.map(|x| x - 3); // moves `x` by `Copy`ing
let _ = x.chain(y); // moves `x` again
let _ = x.cycle(); // `.cycle()` is only possible when `Self: Clone`

[kennytm]

@dead10ck It is fine to box and return it as Box<Iterator<Item=i32>>, because Iterator<Item=i32> itself is an object-safe trait. It doesn't matter if .cycle() relies on Clone. You can't return Box<Foo> because Foo reait is not object-safe.

[dead10ck]

@kennytm I see, thank you! Sorry for the confusion. I'm still pretty inexperienced with Rust.