Closure returned as impl FnOnce incorrectly borrows captured used-by-value iff it is Copy

[CAD97]

Given the following code: [playground] (with the other cases as well)

#[derive(Clone, Copy)]
pub struct Unit;

fn returns(it: Unit) -> impl FnOnce() -> Unit {
    || it
}

The current output is:

error[E0373]: closure may outlive the current function, but it borrows `it`, which is owned by the current function
 --> src/lib.rs:5:5
  |
5 |     || it
  |     ^^ -- `it` is borrowed here
  |     |
  |     may outlive borrowed value `it`
  |
note: closure is returned here
 --> src/lib.rs:5:5
  |
5 |     || it
  |     ^^^^^
help: to force the closure to take ownership of `it` (and any other referenced variables), use the `move` keyword
  |
5 |     move || it
  |     ++++

If you remove the Copy implementation, then this compiles without error.

APIT also compiles without error, but TAIT also causes an error.

Without Copy

#[derive(Clone)]
pub struct Unit;

fn returns(it: Unit) -> impl FnOnce() -> Unit {
    || it
}

---

APIT

#[derive(Clone, Copy)]
pub struct Unit;

fn apit(it: Unit) {
    fn takes(_: impl FnOnce() -> Unit) {}
    takes(|| Unit)
}

---

TAIT

#[derive(Clone, Copy)]
pub struct Unit;

fn tait(it: Unit) {
    type F = impl FnOnce() -> Unit;
    let f: F = || it;
}

---

I checked, and this has been the case since RPIT was first stabilized.

@rustbot label +A-impl-trait

Technically, this makes adding Copy to a type a breaking change 🙃

I expected to see this happen: explanation

Instead, this happened: explanation

Meta

rustc version: 1.65.0-nightly (2022-08-16 86c6ebe)

[CAD97]

I suspect this is due to how closures "sufficiently" capture the captured variables; for a Copy type, it is sufficient to capture it by-reference to use it by-copy.

However, this behavior is imho nondesirable, as it makes the semantics visibly (because of lifetimes) change to treat the type differently because it is known to be Copy.

[veber-alex]

This doesn't just happens when returning the closure, I encountered this with the new scoped thread API.
For example:

pub fn test(n: usize) {
    let sum = Mutex::new(0);
    std::thread::scope(|s| {
        for i in 0..n {
            s.spawn(|| {
                *sum.lock().unwrap() += i;
            });
        }
    });
}

This doesn't compile because "closure may outlive the current function, but it borrows i, which is owned by the current function"
Adding a move doesn't fix the issue though, because it moves sum into the first thread and you get "use of moved value: sum"
So to fix it you need to declare sum as &Mutex::new(0) or introduce another variable which is a reference to the mutex and borrow that.

If I change the code to use another type for i that is not Copy, it works as expected.

struct W(usize);

impl AddAssign<W> for usize {
    fn add_assign(&mut self, rhs: W) {
        *self += rhs.0;
    }
}

pub fn test2(n: usize) {
    let sum = Mutex::new(0);
    std::thread::scope(|s| {
        for i in (0..n).map(W) {
            s.spawn(|| {
                *sum.lock().unwrap() += i;
            });
        }
    });
}

Of course adding #[derive(Copy)] to W breaks the code again.

[obi1kenobi]

👋 Hi, maintainer of cargo-semver-checks here. Since I'm only minimally familiar with the context, I'd love your input on whether it's worth making cargo-semver-checks treat adding Copy as a breaking change. For example, reasons to do it would be if this issue is tricky to fix or unlikely to be prioritized, and is therefore likely to remain open for a while.

Related: obi1kenobi/cargo-semver-checks#5

[nikomatsakis]

The behavior is "by design", for better or worse.

Without the move keyword, closures take references wherever possible. This includes reads of a Copy value -- but values that are not Copy must be moved.

With the move keyword, closures capture by value always. This includes reads of a Copy value. =)

I don't believe we did a thorough write-up of the reasoning here -- though I have a vague memory of an internals thread -- but it helps avoid some surprising behavior when combined with mut. For example, the following code fails to compile today:

let mut i = 0;
let c = || i;
i += 1;
println!("{}", c());

but if you add the move keyword, then it would compile and print 0.

The intent was that plain || x are "attached" to the stack frame that created them and not meant to escape, and move closures are meant to be used for closures that are returned. But of course, the way the system is setup, this is not a hard requirement. (If I had it to do over again, or if we were to tweak the system, I'd be inclined to rethink the move-vs-default setup to be focused more about escaping vs not escaping.)

[nikomatsakis]

Given all this, though, what's the fate of this bug?

The system is working as designed. It'd be good to verify that this is documented in the reference (I think I've had "Document capture rules better" on my to-do list for a while, actually, now that I think about it...), and it's good to document this in terms of the our semver recommendations, but changing the language is more like "RFC territory" (I assume it'd be backwards incompatible, though I don't have an example off hand I suppose, maybe that's not the case).

[CAD97]

If non-move closures always borrowed from the parent environment, then I'd agree with the current behavior. As is, though, it's "capture by example", so if a value is used by-value it's captured by-value... except if it's Copy.

I believe changing this would be nonbreaking, because the only ways to observe the difference between by-ref and by-copy captures are

• the size of the closure, and
• failing to compile due to lifetimes.

I accept that this is more in the territory of a language change than a bugfix, but I've more often seen closure captures explained as "by example" than "as needed" (even when the words "as needed" are used) and never once seen this corner case mentioned.

I'm pretty sure if you asked a lot of programmers whether || x captures x by value or by reference, fewer than 1% would say anything other than an unconditional by-value.

It seems painful to say you have to spell out every capture you do want to capture by reference in a pseudo captures list in order to capture Copy values by value. (The opposite of spelling by-value-but-only-used-by-ref captures up front in a closure is somewhat common.) This is necessary for something which is deliberately borrowing from input but captures function-local Copy values.

To note, async {} also has this same pitfall.

[traviscross]

@rustbot labels +AsyncAwait-Triaged +WG-async

We reviewed this today in WG-async triage.

As @nikomatsakis said here, this behavior is by design, for better or worse. While we'd like to see better documentation about this, we can probably go ahead and close this particular issue.