Updating a closure variable doesn't 'stick' and reverts

[chb0github]

I have this rust code where I am trying to implement observability. And, in my closure add_foo_listener I simply update the called variable and then assert it it's called.

This code compiles, and the closure is invoked, as is evidenced by the println, but the called variable doesn't stick. Everything I read says I should be using FnMut which, I believe I am. But, I don't understand why this boolean isn't being updated

    #[derive(Debug, Clone, Hash, PartialEq, Eq)]
    struct Foo {
        baz: u32
    }
    struct Holder {
        pub foo_observers: Vec<Box<dyn FnMut(Foo)>>,
    }

    impl Holder {

        pub fn add_foo_listener<CB: 'static + FnMut(Foo) + FnMut(Foo)>(&mut self, on_foo: CB) -> &mut Self {
            self.foo_observers.push(Box::new(on_foo));
            self
        }

        pub fn notify_foo_event(&mut self, foo: Foo) -> &mut Self {
            self.foo_observers
                .iter_mut()
                .for_each(|notify| notify(foo.clone()));
            self
        }
    }
    #[test]
    fn test_foo() -> Result<(), String> {
        let mut holder = Holder{
            foo_observers: Vec::new()
        };
        let mut called: bool = false;
        let foo = Foo{
            baz : 10
        };

        holder.add_foo_listener( move |f: Foo| {
            println!("Listener called");
            called = true;
        }).notify_foo_event(foo);
        assert!(called); // called is still false
        Ok(())
    }

Output:

---- notifier::test::test_foo stdout ----
Listener called
thread 'notifier::test::test_foo' panicked at 'assertion failed: called', foo.rs
assert!(called); 
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[kpreid]

The listener closure in test_foo is a move closure, so everything it uses is moved or copied into it. Thus, the called it's assigning to is effectively a new variable independent of the outer called.

Here's a shorter program demonstrating the same problem, and how a non-move closure does change the outer behavior:

fn main() {
    let mut foo = false;
    let mut f_move = move || { foo = true; };
    assert!(!foo);
    f_move();
    assert!(!foo);

    let mut f_borrow = || { foo = true; };
    f_borrow();
    assert!(foo);
}

All of the assertions pass. However, it has a couple of unused-variable warnings, showing that the foo inside f_move is considered separate — the warnings are telling you that foo = true; is not having any effect. When you're confused about your program's behavior, make sure to read the warnings carefully, and fix them (whether or not they're actual problems, decluttering will help).

In your case, where you need to share state between test_foo() and the inner closure, you need explicit shared mutability (interior mutability). An Rc<Cell<bool>> will work:

use std::cell::Cell;
use std::rc::Rc;
...

    let mut called: Rc<Cell<bool>> = Rc::new(Cell::new(false));

    holder
        .add_foo_listener({
            let called = called.clone();  // independent reference to same mutable cell
            move |f: Foo| {
                println!("Listener called");
                called.set(true);
            }
        })
        .notify_foo_event(foo);

    assert!(called.get());

Playground link

[Yandros]

Also, a tip when seeing surprising behavior, is to pay special attention to the warnings firing. Here, ideally, it should lint on capture by Copy of a mutable variable whose original copy is afterwards accessed (alas, it doesn't), but at least it warns against an "unused assignment":

warning: value assigned to `called` is never read
  --> src/lib.rs:35:13
   |
35 |             called = true;
   |             ^^^^^^
   |
   = note: `#[warn(unused_assignments)]` on by default
   = help: maybe it is overwritten before being read?

warning: unused variable: `called`
  --> src/lib.rs:35:13
   |
35 |             called = true;
   |             ^^^^^^
   |
   = note: `#[warn(unused_variables)]` on by default
   = help: did you mean to capture by reference instead?

So, granted, it's not the clearest error message yet, but at least it hints at the issue having something to do with called, the assignment inside the closure, and now that you have read this thread, about how called is captured.

---

While, indeed, not using move will, in cases such as this one, nudge Rust into capturing by &mut, when in doubt, it's actually better to use move but to spell out every variable exactly:

        let mut called: bool = false;
        let foo = Foo {
            baz : 10
        };

+       let called_mut_ref: &mut bool = &mut called;
        holder.add_foo_listener(move |f: Foo| {
            println!("Listener called");
-           called = true;
+           *called_mut_ref = true;
        }).notify_foo_event(foo);

• This way, no matter whether the capture of called_mut_ref: &mut bool happens by move, or by (nested) mut ref, you know that *called_mut_ref refers to / points to the original called variable.

---

The above only applies to the basic closure examples; if you need the closure or future to be 'static, then an owned move will be necessary; if nonetheless you need another (thus, shared) access outside the closure, then you'll need shared ownership, often achieved through ref-counting pointers such as Arc. Due to the shared nature of the access, you won't be able to have &mut / exclusive access, and you'll need shared mutability / interior mutability types, such as an AtomicBool for this instance, or a Mutex/RwLock more generally. Hence leading to @kpreid's suggestion

[steffahn]

@Yandros

While, indeed, not using move will, in cases such as this one, nudge Rust into capturing by &mut , when in doubt, it's actually better to use move but to spell out every variable exactly:

Not using move should be enough to be safe, shouldn't it? If it isn't a Copy type then you can't access it anymore after it has been (implicitly) moved by a non-“move” closure, and I believe if it is a Copy type, then it should never automatically use a capture by move.

---

I agree there should be a warning for the case you described (mutable variable, Copy type, move closure and variable used afterwards).

[Yandros]

True, my phrasing was a bit unclear: yes, no move always does the trick; but sometimes no-move is not an option, when having to also manage other captures: you sometimes need an owned capture for something else and yet still be yielding an FnMut() or Fn() closure, and in that case using move becomes mandatory^[1].

---

1. For a non-Copy type, using a if false { drop(capture); loop {} } inside the closure may do the trick, but it's not the most obvious nor the most readable trick. And edition 2021 may also interact with all this. It doesn't; not even unreachable code can consume an FnMut's captures ↩︎

[steffahn]

@Yandros

For a non-Copy type, using a if false { drop(capture); loop {} } inside the closure may do the trick, but it's not the most obvious nor the most readable trick. And edition 2021 may also interact with all this. It doesn't; not even unreachable code can consume an FnMut's captures

Unsurprising to me that the approach didn't work. E.g. if the loop was a panic, it could be caught and the closure called another time if it was FnMut. And if it's Fn, it could be called a second time in parallel. (And as we all know the constant false condition means nothing to Rust's static analysis (except for optimizations later in compilation).)

[Yandros]

Yeah the heuristic to allow for that would be too complex, and semver brittle (it would have to take panicking into account for FnMut, and just inaccessible for Fn as you pointed out): a fool's thought experiment indeed. I just want explicit closure captures so badly ; the all-or-nothing of move gets very unsatisfactory after a while, to the point I'm testing any possible hack to then let a macro do it (right now macros to achieve this need to be type-annotated)

[chb0github]

this was super helpful. Thanks you!

I am going to take this a step further - in that lambda, I also want to assert that the foo being passed into the closure is equal to the foo that was created - the idea being that you have to tell me about the correct foo in this event handler

So,

    let mut called: Rc<Cell<bool>> = Rc::new(Cell::new(false));

    holder
        .add_foo_listener({
            let called = called.clone();  // independent reference to same mutable cell
            move |f: Foo| {
                assert_eq!(foo, f); // 
                called.set(true);
            }
        })
        .notify_foo_event(foo);

And, depending on what I do, I either get:

110 |             .notify_foo_event(foo.clone());
    |                                         ^^^^^^^^^^^^ value borrowed here after move

or I get

110 |             .notify_foo_event(foo);
    |                                            ^^^ value used here after move

I don't get what it means to "move" something - nothing is actually moving that I can see

[kpreid]

@chb0github

I don't get what it means to "move" something - nothing is actually moving that I can see

The closure is a value itself, which is owned by the foo_observers vector in Holder. Per the type, it has no lifetime bound — it is allowed to be kept alive as long as the enclosing Holder wants it to. Therefore, everything it needs must be owned by it. In an ordinary struct, we might write a new() function which constructs or moves in everything needed; in a closure, this role is served by the enclosing code and by the presence or absence of the move option.

In particular, the Cell<bool> has two reference-counted pointers to it — one that stays in the outside scope/stack frame, and one that was created for the use of the closure by let called = called.clone(); and then moved into it.

Similarly, if you want to assert equality, the thing that you are comparing equality against must also be moved into the closure. Foo implements Clone, so you can do exactly the same thing with it:

    holder
        .add_foo_listener({
            let called = called.clone();
            let foo = foo.clone();
            move |f: Foo| {
                assert_eq!(foo, f);
                called.set(true);
            }
        })
        .notify_foo_event(foo);

You can also give the clones different variable names, which might help to understand the difference:

    holder
        .add_foo_listener({
            let called_for_closure = called.clone();
            let foo_for_closure = foo.clone();
            move |f: Foo| {
                assert_eq!(foo_for_closure, f);
                called_for_closure.set(true);
            }
        })
        .notify_foo_event(foo);

Notice that within the body of the closure, everything it mentions is one of:

• a moved variable, thus owned by the closure
• an argument to the function, thus owned by the closure
• a literal or global, thus static

This is the rule for 'static closures (Box<dyn FnMut(Foo)> has an implicit 'static bound): they must not borrow anything from an outer scope. By cloning and moving all the things it captures, we obey this rule.