Cell::update

[ghost]

I was wondering why we don't have a method like the following for Cells:

impl<T: Copy> Cell<T> {
    fn update<F: FnMut(T) -> T>(&self, f: F) {
        self.set(f(self.get()));
    }
}

This makes some particularly annoying use cases like updating counters easier (see before and after for comparison):

data.dep_kind.set(cmp::max(data.dep_kind.get(), dep_kind));
data.dep_kind.update(|k| cmp::max(k, dep_kind));

self.has_errors.set(self.has_errors.get() | old_has_errors);
self.has_errors.update(|e| e | old_has_errors);

self.count_imm.set(self.count_imm.get() + 1);
self.count_imm.update(|c| c + 1);

Why require T: Copy instead of T: Default? Because otherwise you might accidentally run into bugs like:

foo.update(|f| {
    let x = bar.set(foo.get()); // Ooops, `foo.get()` returns `0` instead of `f`!
    f + x
});

And why not T: Clone instead? Because it might introduce hidden costs.
For example, to append "..." to a Cell<String>, you might do:

let mut s = foo.take();
s.push_str("...");
foo.set(s);

But if update cloned values, that'd introduce a hidden allocation:

let mut s = foo.update(|mut s| {
    s.push_str("...");
    s
});

Here's a nicer version of the same thing. It still has an allocation, but with T: Default there wouldn't be one, so one might (incorrectly) expect this to be efficient:

foo.update(|mut s| s + "foo");

All in all, T: Default and T: Clone have subtle pitfalls, but Cell::update with T: Copy seems like a safe choice and would be a nice addition to the standard library. After all, T: Copy is by far the most common use of Cell anyway, which can be confirmed by a quick ripgrep through the rustc codebase.

[hanna-kruppe]

And why not T: Clone instead? Because it might introduce hidden costs.

There's a more fundamental reason not to do it: cloning Cell contents is unsound.

[SimonSapin]

T: Copy is by far the most common use of Cell anyway

That’s because it was the only use allowed until recently.

But yeah, Cell::update sounds nice to have, and requiring T: Copy allows the least surprising behavior as you explained.

[SimonSapin]

I think this is small and straightforward enough to open a PR without going through the whole RFC process, with a FCP for libs team review in the PR.

[ghost]

I'd also like to ask what you think about two more ideas:

1. Extending Cell::get by changing T: Copy to T: Clone + Default.
2. A different Cell::update that allows not only T: Copy but any T: Default.

These additions would allow the following code to work:

let count = Cell::new(5);
count.update(|c| *c += 1);
println!("new count: {}", count.get2());

let v = Cell::new(Vec::new());
v.update(|v| v.push(5));
v.update(|v| v.push(6));
println!("a clone of the vector: {:?}", v.get2());

Playground link with a working example.

[SimonSapin]

Extending Cell::get by changing T: Copy to T: Clone + Default.

That’s a breaking change, not an extension. (&T for example implements Copy but not Default.) Cell::get is stable.

A different Cell::update that allows not only T: Copy but any T: Default.

Neither Copy nor Default imply the other, so you have to pick one. Or have two almost-but-not-quite identical methods with different names, which doesn’t seem like good design.

[ghost]

That’s a breaking change, not an extension.

Would there be a way to somehow implement Cell::get for both T: Copy and T: Clone + Default, perhaps using specialization? I tried, but the two implementations keep conflicting. Perhaps specialization is not powerful enough yet?

Or have two almost-but-not-quite identical methods with different names, which doesn’t seem like good design.

Yeah... It's a pity that, in order to push something into a Cell<Vec<T>>, you have to do:

let mut v = cell.take();
v.push(foo);
cell.set(v);

Seems like there's still a lot of room to improve Cell, but hard to come up with the right solution.

[SimonSapin]

Regarding get, I don’t think any implicit .clone() call is desirable (since those can be expensive).

I agree that the ergonomics of Cell<T> for T: !Copy are not great at the moment. Maybe two methods are warranted, with names that reflect their different behaviors:

impl<T> Cell<T> {
    /// This uses `.get()` then `.set()`
    fn get_set<F>(&self, f: F) where T: Copy, F: FnOnce(T) -> T {
        self.set(f(self.get()));
    }

    /// This uses `.take()` to temporarily move out the value
    /// (leaving `Default::default()` behind), mutate it, then move it back.
    /// That default can be observed if the cell is accessed during the callback.
    fn temporary_take<F, R>(&self, f: F) -> R where T: Default, F: FnOnce(&mut T) -> R {
        let mut value = self.take();
        let result = f(&mut value);
        self.set(value);
        result
    }
}

I don’t really like these particular names, but you get the idea.

Taking something and then giving it back… It’s tempting to call that "borrowing", but that term obviously already has a strong meaning in Rust that doesn’t apply here.

Usage examples:

cell.temporary_take(|vec| vec.push(foo));

let vec = cell.temporary_take(|vec| vec.clone());

[SimonSapin]

update and mutate could work as short name to describe these respective methods in isolation, but if both exist I feel they’re not self-explanatory enough in how they differ from each other. Remembering which one is which becomes a memory exercise.

[Centril]

Adding to the bikeshed: modify.

[HeroicKatora]

The addition of temporary_take dilutes the original intention. I would identify two objectives here, which are orthogonal in my point of view. On the one hand we have distinct intents:

• encapsulating a (possibly copyfree) change of internal state
• encapsulating a (possibly copyfree) derivation of output information

While an example for the former was already given by the original author, a motivation for the latter could be retrieving the length of a stored vector. A combination of these is also possible, expressed in the proposed interface of temporary_take by @SimonSapin.

On the other hand, we have to consider zero-cost philosopy and implementation:

• enable non-copy types
• offer convenience for T: Default

Essentially, we have three possibilities of retrieving the input value for the function (get, replace, take) and three different ways of applying the function: as a value function (T -> T), as mutable reference (&mut T -> R) and as normal reference (&T -> R) which is redundant but more expressive. I would propose something along the lines of this:

/// Holds a temporary value that replaces the cells content when
/// it is dropped. A panic in its `map` function will forget about the
/// internal state and not perform the final replacement.
pub struct LendGuard<'a, T: 'a> {
  cell: &'a Cell<T>,
  value: Option<T>, // Panic safety 
}

impl<T> Cell<T> {
  /// Give a copy of the content to the LendGuard
  fn lend_copy<T>(&self) -> LendGuard<T> where T: Copy {
    LendGuard{cell: self, value: Some(self.get())}
  }

  /// Replaces the cells value with T::default() until the LendGuard
  /// is dropped. 
  fn lend<T>(&self) -> LendGuard<T> where T: Default {
    LendGuard{cell: self, value: Some(self.take())}
  }

  /// Replaces the cells value with the supplied value until the
  /// LendGuard is dropped. 
  fn pawn<T>(&self, t: T) -> LendGuard<T> {
    LendGuard{cell: self, value: Some(self.replace(t))}
  }
}

impl<'a, T: 'a> LendGuard<'a, T> {
  /// Replaces the extracted value with the result of applying
  /// the function to it.
  fn map<F>(mut self, f: F) where F: FnOnce(T) -> T {
    self.value = self.value.take().map(f);
  }

  /// Inspects the extracted value through an immutable reference
  /// before moving it back. The result of the inspection is returned
  /// to the caller.
  fn inspect<F, R>(self, f: F) -> R where F: FnOnce(&T) -> R {
    f(self.value.as_ref().unwrap())
  }

  /// Inspects the extracted value through a mutable reference,
  /// possibly modifying it before moving it back. The result of
  /// the inspection is returned to the caller.
  fn mutate<F, R>(mut self, f: F) -> R where F: FnOnce(&mut T) -> R {
    f(self.value.as_mut().unwrap())
  }
}

impl<'a, T> Drop for LendGuard<'a, T> {
  fn drop(&mut self) {
    self.value.take().map(|v| self.cell.set(v));
  }
}

This interface could then be used as follows:

let cell = Cell::new(5);
self.lend_copy().map(|v| v + cell.get()); // cell = Cell(10)
let inspect = cell.lend().inspect(|&v| v + cell.get()); // inspect = 10
cell.pawn(42).mutate(|v: &mut _| *v += cell.get()); // cell = Cell(52)
println!("{}", cell.into_inner() + inspect); //print 62

[HeroicKatora]

In case someone wants to try this out by simple copy&paste, here is an (even slightly more generic) implementation of this proposal: github gist

[ghost]

Cell::update has been added, gated under feature cell_update.
Tracking issue: rust-lang/rust#50186