Unexpected partial move in closure for `Copy` field

[wenym1]

Hi! Recently we have encountered an issue for metrics reporting. We are using futures_async_stream::try_stream to convert our iterators into stream, and we implement a struct Stats to collect useful metrics and report the metrics when Stats gets dropped.

The try_stream macro generates the internal code of the stream by using rust Generator, and the Stats is mutably moved in the generator closure, and will get modified while collecting the metrics. Under our expectation, we are able to collect some correct metrics before the Stats gets dropped. However, in our observation, the Stats is not modified at all, and none of the metrics is collected.

After some experiment, we figure out that there is a confusing behavior in the rust code. We have minimized our code, and the following code is a minimum reproducible code for the unexpected behavior.

I tried this code:

#![feature(generators, generator_trait)]

use std::ops::Generator;
use std::pin::Pin;

#[derive(Default, Debug)]
struct SomethingElse {
    num: usize,
}

#[derive(Default, Debug)]
struct TestStats {
    total_items: usize,
    some: SomethingElse,
}

impl Drop for TestStats {
    fn drop(&mut self) {
        println!("dropped: {} {:?}", self.total_items, self.some);
    }
}

pub fn main() {
    let mut stats = TestStats::default();

    let mut generator = Box::pin(move || {
        // println!("{:?}", stats);
        stats.total_items += 1;
        yield 2;
    });

    Pin::new(&mut generator).resume(());

}

We expected to see this output:

dropped: 1 SomethingElse { num: 0 }

which means that stats.total_items += 1 gets executed once.

Instead, we saw

dropped: 0 SomethingElse { num: 0 }

which means that stats.total_items += 1 did not get executed on stats.total_items.

After we uncomment the line println!("{:?}", stats); and reference the whole stats,

#![feature(generators, generator_trait)]

use std::ops::Generator;
use std::pin::Pin;

#[derive(Default, Debug)]
struct SomethingElse {
    num: usize,
}

#[derive(Default, Debug)]
struct TestStats {
    total_items: usize,
    some: SomethingElse,
}

impl Drop for TestStats {
    fn drop(&mut self) {
        println!("dropped: {} {:?}", self.total_items, self.some);
    }
}

pub fn main() {
    let mut stats = TestStats::default();

    let mut generator = Box::pin(move || {
        println!("{:?}", stats); // Previously commented !!!
        stats.total_items += 1;
        yield 2;
    });

    Pin::new(&mut generator).resume(());

}

, the output gets as expected.

TestStats { total_items: 0, some: SomethingElse { num: 0 } }
dropped: 1 SomethingElse { num: 0 }

Meta

The code can be run on Rust Playground.

<version>
1.70.0-nightly 2023-03-05 7820b62

Backtrace

``` ```

[wenym1]

We have tried inspecting the MIR generated by the rust compiler under whether the println!("{:?}", stats); is commented or not.

• When println!("{:?}", stats); is not commented

  let mut generator = Box::pin(move || {
  println!("{:?}", stats);
  stats.total_items += 1;
  yield 2;
  });

  the stats is referenced as a whole, and the generated closure holds the whole stats as seen in the MIR _3 = [generator@[src/main.rs:26:34: 26:41](...) (#0)] { stats: move _1 } in

  bb0: {
      _1 = <TestStats as Default>::default() -> bb1; // scope 0 at [src/main.rs:24:21: 24:41](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021#)
                                       // mir::Constant
                                       // + span: [src/main.rs:24:21: 24:39](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021#)
                                       // + literal: Const { ty: fn() -> TestStats {<TestStats as Default>::default}, val: Value(<ZST>) }
  }
  
  bb1: {
      _3 = [generator@[src/main.rs:26:34: 26:41](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021#) (#0)] { stats: move _1 }; // scope 1 at [src/main.rs:26:34: 30:6](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021#)
                                       // generator
                                       // + def_id: DefId(0:8 ~ playground[c2be]::main::{closure#0})
                                       // + substs: [
                                       //     (),
                                       //     i32,
                                       //     (),
                                       //     {i32, ()},
                                       //     (TestStats,),
                                       // ]
                                       // + movability: Movable

• After we commented println!("{:?}", stats);,

  let mut generator = Box::pin(move || {
  // println!("{:?}", stats);
  stats.total_items += 1;
  yield 2;
  });

  only the field total_items of usize is used, and therefore the closure only takes stats.total_items as seen from the MIR _3 = [generator@[src/main.rs:26:34: 26:41](...) (#0)] { stats: (_1.0: usize) }. And since usize is Copy, the original total_items is untouched, and left unmodified.

  bb0: {
      _1 = <TestStats as Default>::default() -> bb1; // scope 0 at [src/main.rs:24:21: 24:41](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021#)
                                       // mir::Constant
                                       // + span: [src/main.rs:24:21: 24:39](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021#)
                                       // + literal: Const { ty: fn() -> TestStats {<TestStats as Default>::default}, val: Value(<ZST>) }
  }
  
  bb1: {
      _3 = [generator@[src/main.rs:26:34: 26:41](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021#) (#0)] { stats: (_1.0: usize) }; // scope 1 at [src/main.rs:26:34: 30:6](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021#)
                                       // generator
                                       // + def_id: DefId(0:8 ~ playground[c2be]::main::{closure#0})
                                       // + substs: [
                                       //     (),
                                       //     i32,
                                       //     (),
                                       //     {i32, ()},
                                       //     (usize,),
                                       // ]
                                       // + movability: Movable

[wenym1]

I even tried running with function closure, and same thing happens.

pub fn main() {
    let mut stats = TestStats::default();

    let mut generator = Box::pin(move || {
        // println!("{:?}", stats);
        stats.total_items += 1;
        // yield 2;
        2
    });

    // Pin::new(&mut generator).resume(());
    generator();

}

The result is the same

dropped: 0 SomethingElse { num: 0 }

In the code above, if stats is not fully referenced, when stats gets dropped, its total_items is untouched.

Is this an expected behavior in rust?

From my understanding, the total_items is accessed from the closure via stats.total_items, so we shall treat the stats as being moved into the closure. However, in the code above, only the Copy field used in the closure is moved into the closure, which is confusing.

[zirconium-n]

It's expected and it's a change between edition 2018/2021. See doc.

[wenym1]

It's expected and it's a change between edition 2018/2021. See doc.

Thanks for pointing out.

Would this behavior be too counter-intuitive? In intuition the statement stats.total_items += 1 should be an in place update on the field of stats. The current behavior is confusing and misleading.

[zirconium-n]

It's a joint result of partial capture / move capture / field being Copy.

pub fn main() {
    let mut i = 0;

    (|| i += 1 )();
    dbg!(i);  // i = 1

    (move || i += 1 )();
    dbg!(i);  // i still 1
}

If you remove move then it would be an in-place update. But if you specify move capture, then Copy type are actually copied. I think we should have some lint against this specific case to warn the user. Something like Captured stats.total_items is not read would help.

[hzxa21]

I think we should have some lint against this specific case to warn the user.

Strong +1. Otherwise it is very hard for developer to spot potential bugs as mentioned in this issue.