Skip to content

react: Distinguish cell IDs #499

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 20, 2018
Merged

react: Distinguish cell IDs #499

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
52e622d
react: Distinguish cell IDs
petertseng Mar 17, 2018
a1cf22d
react: Use newtype instead of typedef
petertseng Apr 18, 2018
169ea90
react: set_value: Return bool
petertseng Apr 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 103 additions & 55 deletions exercises/react/example.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,32 @@
use std::collections::HashMap;

pub type CellID = usize;
pub type CallbackID = usize;
/// `InputCellID` is a unique identifier for an input cell.
#[derive(Clone, Copy, Debug, PartialEq)]
pub struct InputCellID(usize);
/// `ComputeCellID` is a unique identifier for a compute cell.
/// Values of type `InputCellID` and `ComputeCellID` should not be mutually assignable,
/// demonstrated by the following tests:
///
/// ```compile_fail
/// let mut r = react::Reactor::new();
/// let input: react::ComputeCellID = r.create_input(111);
/// ```
///
/// ```compile_fail
/// let mut r = react::Reactor::new();
/// let input = r.create_input(111);
/// let compute: react::InputCellID = r.create_compute(&[react::CellID::Input(input)], |_| 222).unwrap();
/// ```
#[derive(Clone, Copy, Debug, PartialEq)]
pub struct ComputeCellID(usize);
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
pub struct CallbackID(usize);

#[derive(Clone, Copy, Debug, PartialEq)]
pub enum CellID {
Input(InputCellID),
Compute(ComputeCellID),
}

#[derive(Debug, PartialEq)]
pub enum SetValueError {
Expand All @@ -15,77 +40,97 @@ pub enum RemoveCallbackError {
NonexistentCallback,
}

struct Cell<'a, T: Copy> {
struct Cell<T: Copy> {
value: T,
last_value: T,
dependents: Vec<CellID>,
cell_type: CellType<'a, T>,
callbacks_issued: usize,
callbacks: HashMap<CallbackID, Box<FnMut(T) -> () + 'a>>,
dependents: Vec<ComputeCellID>,
}

enum CellType<'a, T: Copy> {
Input,
Compute(Vec<CellID>, Box<Fn(&[T]) -> T + 'a>),
struct ComputeCell<'a, T: Copy> {
cell: Cell<T>,

dependencies: Vec<CellID>,
f: Box<Fn(&[T]) -> T + 'a>,
callbacks_issued: usize,
callbacks: HashMap<CallbackID, Box<FnMut(T) -> () + 'a>>,
}

impl <'a, T: Copy> Cell<'a, T> {
fn new(initial: T, cell_type: CellType<'a, T>) -> Self {
impl <T: Copy> Cell<T> {
fn new(initial: T) -> Self {
Cell {
value: initial,
last_value: initial,
dependents: Vec::new(),
cell_type: cell_type,
}
}
}

impl <'a, T: Copy> ComputeCell<'a, T> {
fn new<F: Fn(&[T]) -> T + 'a>(initial: T, dependencies: Vec<CellID>, f: F) -> Self {
ComputeCell {
cell: Cell::new(initial),

dependencies,
f: Box::new(f),
callbacks_issued: 0,
callbacks: HashMap::new(),
}
}
}

pub struct Reactor<'a, T: Copy> {
cells: Vec<Cell<'a, T>>,
inputs: Vec<Cell<T>>,
computes: Vec<ComputeCell<'a, T>>,
}

impl <'a, T: Copy + PartialEq> Reactor<'a, T> {
pub fn new() -> Self {
Reactor{
cells: Vec::new(),
inputs: Vec::new(),
computes: Vec::new(),
}
}

pub fn create_input(&mut self, initial: T) -> CellID {
self.cells.push(Cell::new(initial, CellType::Input));
self.cells.len() - 1
pub fn create_input(&mut self, initial: T) -> InputCellID {
self.inputs.push(Cell::new(initial));
InputCellID(self.inputs.len() - 1)
}

pub fn create_compute<F: Fn(&[T]) -> T + 'a>(&mut self, dependencies: &[CellID], compute_func: F) -> Result<CellID, CellID> {
pub fn create_compute<F: Fn(&[T]) -> T + 'a>(&mut self, dependencies: &[CellID], compute_func: F) -> Result<ComputeCellID, CellID> {
// Check all dependencies' validity before modifying any of them,
// so that we don't perform an incorrect partial write.
if let Some(&invalid) = dependencies.iter().find(|&dep| *dep >= self.cells.len()) {
return Err(invalid);
for &dep in dependencies {
match dep {
CellID::Input(InputCellID(id)) => if id >= self.inputs.len() { return Err(dep) },
CellID::Compute(ComputeCellID(id)) => if id >= self.computes.len() { return Err(dep) },
}
}
let new_id = self.cells.len();
for &id in dependencies {
self.cells.get_mut(id).unwrap().dependents.push(new_id);
let new_id = ComputeCellID(self.computes.len());
for &dep in dependencies {
match dep {
CellID::Input(InputCellID(id)) => self.inputs[id].dependents.push(new_id),
CellID::Compute(ComputeCellID(id)) => self.computes[id].cell.dependents.push(new_id),
}
}
let inputs: Vec<_> = dependencies.iter().map(|&id| self.value(id).unwrap()).collect();
let initial = compute_func(&inputs);
self.cells.push(Cell::new(initial, CellType::Compute(dependencies.iter().cloned().collect(), Box::new(compute_func))));
self.computes.push(ComputeCell::new(initial, dependencies.to_vec(), compute_func));
Ok(new_id)
}

pub fn value(&self, id: CellID) -> Option<T> {
self.cells.get(id).map(|c| c.value)
match id {
CellID::Input(InputCellID(id)) => self.inputs.get(id).map(|c| c.value),
CellID::Compute(ComputeCellID(id)) => self.computes.get(id).map(|c| c.cell.value),
}
}

pub fn set_value(&mut self, id: CellID, new_value: T) -> Result<(), SetValueError> {
match self.cells.get_mut(id) {
Some(c) => match c.cell_type {
CellType::Input => {
c.value = new_value;
Ok(c.dependents.clone())
},
CellType::Compute(_, _) => Err(SetValueError::ComputeCell),
pub fn set_value(&mut self, id: InputCellID, new_value: T) -> Result<(), SetValueError> {
let InputCellID(id) = id;
match self.inputs.get_mut(id) {
Some(c) => {
c.value = new_value;
Ok(c.dependents.clone())
},
None => Err(SetValueError::NonexistentCell),
}.map(|deps| {
Expand All @@ -100,16 +145,19 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> {
})
}

pub fn add_callback<F: FnMut(T) -> () + 'a>(&mut self, id: CellID, callback: F) -> Option<CallbackID> {
self.cells.get_mut(id).map(|c| {
pub fn add_callback<F: FnMut(T) -> () + 'a>(&mut self, id: ComputeCellID, callback: F) -> Option<CallbackID> {
let ComputeCellID(id) = id;
self.computes.get_mut(id).map(|c| {
c.callbacks_issued += 1;
c.callbacks.insert(c.callbacks_issued, Box::new(callback));
c.callbacks_issued
let cbid = CallbackID(c.callbacks_issued);
c.callbacks.insert(cbid, Box::new(callback));
cbid
})
}

pub fn remove_callback(&mut self, cell: CellID, callback: CallbackID) -> Result<(), RemoveCallbackError> {
match self.cells.get_mut(cell) {
pub fn remove_callback(&mut self, cell: ComputeCellID, callback: CallbackID) -> Result<(), RemoveCallbackError> {
let ComputeCellID(cell) = cell;
match self.computes.get_mut(cell) {
Some(c) => match c.callbacks.remove(&callback) {
Some(_) => Ok(()),
None => Err(RemoveCallbackError::NonexistentCallback),
Expand All @@ -118,29 +166,28 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> {
}
}

fn update_dependent(&mut self, id: CellID) {
fn update_dependent(&mut self, id: ComputeCellID) {
let ComputeCellID(id) = id;

let (new_value, dependents) = {
// This block limits the scope of the self.cells borrow.
// This is necessary becaue we borrow it mutably below.
let (dependencies, f, dependents) = match self.cells.get(id) {
Some(c) => match c.cell_type {
CellType::Input => panic!("Input cell can't be a dependent"),
CellType::Compute(ref dependencies, ref f) => (dependencies, f, c.dependents.clone()),
},
let (dependencies, f, dependents) = match self.computes.get(id) {
Some(c) => (&c.dependencies, &c.f, c.cell.dependents.clone()),
None => panic!("Cell to update disappeared while querying"),
};
let inputs: Vec<_> = dependencies.iter().map(|&id| self.value(id).unwrap()).collect();
(f(&inputs), dependents)
};

match self.cells.get_mut(id) {
match self.computes.get_mut(id) {
Some(c) => {
if c.value == new_value {
if c.cell.value == new_value {
// No change here, we don't need to update our dependents.
// (It wouldn't hurt to, but it would be unnecessary work)
return;
}
c.value = new_value;
c.cell.value = new_value;
},
None => panic!("Cell to update disappeared while updating"),
}
Expand All @@ -150,19 +197,20 @@ impl <'a, T: Copy + PartialEq> Reactor<'a, T> {
}
}

fn fire_callbacks(&mut self, id: CellID) {
let dependents = match self.cells.get_mut(id) {
fn fire_callbacks(&mut self, id: ComputeCellID) {
let ComputeCellID(id) = id;
let dependents = match self.computes.get_mut(id) {
Some(c) => {
if c.value == c.last_value {
if c.cell.value == c.cell.last_value {
// Value hasn't changed since last callback fire.
// We thus shouldn't fire the callbacks.
return
}
for cb in c.callbacks.values_mut() {
cb(c.value);
cb(c.cell.value);
}
c.last_value = c.value;
c.dependents.clone()
c.cell.last_value = c.cell.value;
c.cell.dependents.clone()
},
None => panic!("Callback cell disappeared"),
};
Expand Down
41 changes: 32 additions & 9 deletions exercises/react/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,30 @@
// Because these are passed without & to some functions,
// it will probably be necessary for these two types to be Copy.
pub type CellID = ();
pub type CallbackID = ();
/// `InputCellID` is a unique identifier for an input cell.
#[derive(Clone, Copy, Debug, PartialEq)]
pub struct InputCellID();
/// `ComputeCellID` is a unique identifier for a compute cell.
/// Values of type `InputCellID` and `ComputeCellID` should not be mutually assignable,
/// demonstrated by the following tests:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if cargo doc flags to the user that the code under test here should fail to compile. If it does, no changes are required. If it does not, it's probably worth mentioning in the text that both of the examples here should fail to compile.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did manage to confirm this. There's a red ⓘ and hovering over it shows "this example deliberately fails to compile"

Given this, I'll be merging it. Just not now, since it is inconvenient to now.

///
/// ```compile_fail
/// let mut r = react::Reactor::new();
/// let input: react::ComputeCellID = r.create_input(111);
/// ```
///
/// ```compile_fail
/// let mut r = react::Reactor::new();
/// let input = r.create_input(111);
/// let compute: react::InputCellID = r.create_compute(&[react::CellID::Input(input)], |_| 222).unwrap();
/// ```
#[derive(Clone, Copy, Debug, PartialEq)]
pub struct ComputeCellID();
#[derive(Clone, Copy, Debug, PartialEq)]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the tests are going to need all of these. The reason why Debug and PartialEq should be obvious.

The reason for Copy (and therefore why Clone) is:

error[E0382]: use of moved value: `callback`
   --> tests/react.rs:203:52
    |
201 |     assert!(reactor.remove_callback(output, callback).is_ok());
    |                                             -------- value moved here
202 |     for _ in 1..5 {
203 |         assert_eq!(reactor.remove_callback(output, callback), Err(RemoveCallbackError::NonexistentCallback));
    |                                                    ^^^^^^^^ value used here after move
    |
    = note: move occurs because `callback` has type `react::CallbackID`, which does not implement the `Copy` trait

pub struct CallbackID();

#[derive(Clone, Copy, Debug, PartialEq)]
pub enum CellID {
Input(InputCellID),
Compute(ComputeCellID),
}

#[derive(Debug, PartialEq)]
pub enum SetValueError {
Expand All @@ -28,7 +51,7 @@ impl <T: Copy + PartialEq> Reactor<T> {
}

// Creates an input cell with the specified initial value, returning its ID.
pub fn create_input(&mut self, _initial: T) -> CellID {
pub fn create_input(&mut self, _initial: T) -> InputCellID {
unimplemented!()
}

Expand All @@ -45,7 +68,7 @@ impl <T: Copy + PartialEq> Reactor<T> {
// Notice that there is no way to *remove* a cell.
// This means that you may assume, without checking, that if the dependencies exist at creation
// time they will continue to exist as long as the Reactor exists.
pub fn create_compute<F: Fn(&[T]) -> T>(&mut self, _dependencies: &[CellID], _compute_func: F) -> Result<CellID, CellID> {
pub fn create_compute<F: Fn(&[T]) -> T>(&mut self, _dependencies: &[CellID], _compute_func: F) -> Result<ComputeCellID, CellID> {
unimplemented!()
}

Expand All @@ -71,7 +94,7 @@ impl <T: Copy + PartialEq> Reactor<T> {
// a `set_value(&mut self, new_value: T)` method on `Cell`.
//
// As before, that turned out to add too much extra complexity.
pub fn set_value(&mut self, _id: CellID, _new_value: T) -> Result<(), SetValueError> {
pub fn set_value(&mut self, _id: InputCellID, _new_value: T) -> Result<(), SetValueError> {
unimplemented!()
}

Expand All @@ -87,7 +110,7 @@ impl <T: Copy + PartialEq> Reactor<T> {
// * Exactly once if the compute cell's value changed as a result of the set_value call.
// The value passed to the callback should be the final value of the compute cell after the
// set_value call.
pub fn add_callback<F: FnMut(T) -> ()>(&mut self, _id: CellID, _callback: F) -> Option<CallbackID> {
pub fn add_callback<F: FnMut(T) -> ()>(&mut self, _id: ComputeCellID, _callback: F) -> Option<CallbackID> {
unimplemented!()
}

Expand All @@ -96,7 +119,7 @@ impl <T: Copy + PartialEq> Reactor<T> {
// Returns an Err if either the cell or callback does not exist.
//
// A removed callback should no longer be called.
pub fn remove_callback(&mut self, cell: CellID, callback: CallbackID) -> Result<(), RemoveCallbackError> {
pub fn remove_callback(&mut self, cell: ComputeCellID, callback: CallbackID) -> Result<(), RemoveCallbackError> {
unimplemented!(
"Remove the callback identified by the CallbackID {:?} from the cell {:?}",
callback,
Expand Down
Loading