Helper closures which modify a state: Use RefCell or pass mutable references?

I sometimes use little helper closures which perform certain tasks on local variables for me. Consider the following simple example where I have a sum that is initialized with 0 and a closure add, which allows me to add values to it. Let's assume I also have a second closure double, which will double the stored sum.

The naive approach doesn't work:

fn main() {
    let mut sum = 0;
    let mut add = |addend| sum += addend;
    //let mut double = || sum *= 2;
    add(4);
    add(5);
    //double();
    add(1);
    //double();
    println!("{}", sum);
}

(Playground)

Because if I uncomment the lines involving doubling, sum is both borrowed mutably by the add and by the double closure. (You can try by uncommenting the lines above.)

There are several solutions:

  • using RefCell,
  • passing a mutable reference to sum when calling each closure,
  • avoid having two closures doing the operation by using an enum to branch.

These three approaches would look as follows:

use std::cell::RefCell;

fn using_cell() {
    let sum = RefCell::new(0);
    let add = |addend| *sum.borrow_mut() += addend;
    let double = || *sum.borrow_mut() *= 2;
    add(4);
    add(5);
    double();
    add(1);
    double();
    let sum = sum.into_inner();
    println!("{}", sum);
}

fn using_mut() {
    let mut sum = 0;
    let add = |sum: &mut _, addend| *sum += addend;
    let double = |sum: &mut _| *sum *= 2;
    add(&mut sum, 4);
    add(&mut sum, 5);
    double(&mut sum);
    add(&mut sum, 1);
    double(&mut sum);
    println!("{}", sum);
}

fn using_enum() {
    let mut sum = 0;
    enum Action { Add(i32), Double() }
    use Action::*;
    let mut doit = |act: Action| match act {
        Add(addend) => sum += addend,
        Double() => sum *= 2,
    };
    doit(Add(4));
    doit(Add(5));
    doit(Double());
    doit(Add(1));
    doit(Double());
    println!("{}", sum);
}

fn main() {
    using_cell();
    using_mut();
    using_enum();
}

(Playground)

Output:

38
38
38

I wonder, which of the three ways do you think is most idiomatic?

If you need mutation, then just pass a mutable reference. Both alternatives seem strictly worse.

In particular, I don't feel that the use of interior mutability (RefCell) is warranted here at all, since interior mutability circumvents compile-time borrow checking, and generally leads to tangled, harder to understand code.

Similarly, matching on an enum to do completely different things seems wrong because it is really just lumping together functions that should have been separate due to a technical detail. It also makes the code less readable in my opinion.

2 Likes

Normally, I would interpret this as my code saying there's a deeper abstraction trying to get out.

fn main() {
  let mut sum = Counter::zero();
  sum.add(4);
  sum.add(5);
  sum.double();
  sum.add(1);
  sum.double();
  println!("{}", sum.value());
}

struct Counter(u32);

impl Counter {
  fn zero() -> Self { Counter(0) }
  fn add(&mut self, delta: u32) { self.0 += delta; }
  fn double(&mut self) { self.0 *= 2; }
  fn value(&self) -> u32 { self.0 }
}

So I'd remove the whole "group of closures that modify local variables" thing and promote it to an object with a name.

It doesn't make sense to me to use RefCell because there is nothing that intrinsically requires shared mutation in this function.

The doit() enum reminds me of a story where - in the name of security and "quality" - a company required their developers to go through an elaborate approval process before they could add a new endpoint to their website's backend. What ended up happening was the developers added a single generic run endpoint and depending on which query parameter you passed it, there would be a switch statement that call into the "actual" endpoint logic... Which just ended up making things worse, not better.

3 Likes

I like that idea. Maybe I tend to use closures too quickly where "objects" (I mean: structs with methods; is there a name for it to distinguish it from dyn objects?) are the better approach.

It's possible to define the struct inside the function too, to make it more concise. And I would probably use pattern matching instead of providing a constructor. Something like this:

fn main() {
    struct Counter(u32);
    impl Counter {
      fn add(&mut self, delta: u32) { self.0 += delta; }
      fn double(&mut self) { self.0 *= 2; }
    }
    let mut cnt = Counter(0);
    cnt.add(4);
    cnt.add(5);
    cnt.double();
    cnt.add(1);
    cnt.double();
    let Counter(res) = cnt;
    println!("{}", res);
}

(Playground)

Output:

38

:thinking: However, this easy approach fails when my functions depend on outside parameters. In that case, the whole thing will become more complex:

fn foo(multiplier: u32) {
    struct Counter<'a> {
        multiplier: &'a u32,
        acc: u32,
    }
    impl<'a> Counter<'a> {
      fn add(&mut self, delta: u32) { self.acc += delta; }
      fn mult(&mut self) { self.acc *= *self.multiplier; }
    }
    let mut cnt = Counter { multiplier: &multiplier, acc: 0 };
    cnt.add(4);
    cnt.add(5);
    cnt.mult();
    cnt.add(1);
    cnt.mult();
    let Counter { acc: res, .. } = cnt;
    println!("{}", res);
}

fn main() {
    foo(2);
}

(Playground)

I deliberately didn't use Copy for u32 here for purposes of demonstrating possible complexity.

So closures sometimes do make things much easier.

This reminds me of "functions returning closures" and currying and feeling unsure too whether working with closures is a good thing or a bad pattern. Consider the following:

use std::f64::consts::TAU;

fn osc(amp: f64, freq: f64, phase: f64) -> impl Fn(f64) -> f64 {
    move |x| amp * f64::sin(x * TAU * freq + phase)
}

struct Osc(f64, f64, f64);

impl Osc {
    fn new(amp: f64, freq: f64, phase: f64) -> Self {
        Osc(amp, TAU * freq, phase)
    }
    fn calc(&self, x: f64) -> f64 {
        self.0 * f64::sin(x * self.1 + self.2)
    }
}

fn main() {
    let osc_closure  = osc(2.0, 3.0, 0.0);
    let osc_obj = Osc::new(2.0, 3.0, 0.0);
    println!("{}, {}, {}",  osc_closure(0.0),  osc_closure(0.1),  osc_closure(0.2));
    println!("{}, {}, {}", osc_obj.calc(0.0), osc_obj.calc(0.1), osc_obj.calc(0.2));
}

(Playground)

Which is the better way here? Use a struct or work with impl-closure?


Note that on Nightly using #![feature(unboxed_closures, fn_traits)], we can also write:

fn main() {
    let osc = Osc::new(2.0, 3.0, 0.0);
    println!("{}, {}, {}", osc(0.0), osc(0.1), osc(0.2));
}

But this comes with some clunky syntax when implementing the Fn trait, see Nightly Playground.

You can also use macros

fn main() {
    let mut sum = 0;
    macro_rules! add {
        ($addend:expr) => {
            sum += $addend
        };
    }
    macro_rules! double {
        () => {
            sum *= 2
        };
    }

    add!(4);
    add!(5);
    double!();
    add!(1);
    double!();
    println!("{}", sum);
}

There was some discussion about this issue on internals awhile back.

2 Likes

Thanks for the reference to the other discussion. So other people have stumbled upon the ergonomics too. I feel like macros are a good way here, I should perhaps consider using them more often.