react: Distinguish cell IDs

[petertseng]

Closes #473

I strongly advise that this should not be merged yet, for at least the following reason:

set_cell_value now can only fail in one way (cell does not exist) since it is no longer possible to designate an input cell to it. Therefore, it no longer needs to return SetValueError. We can just return a boolean, even.

Except for that point, I guess everything else is how I expect it to be.

I'm also happy to hear ideas about being able to move set_value onto input cells directory, in which case I don't see the need for this PR.

[petertseng]

Well, I guess this is it.

[coriolinus]

I note that these are typedefs, not newtypes. As such, the compiler treats InputCellID, ComputeCellID, and usize interchangeably. If we want it to enforce real type safety for us, we'll need to use newtypes:

pub struct InputCellID(usize);
pub struct ComputeCellID(usize);
// the above two types _cannot_ be interchanged

[coriolinus]

It might be worth declaring these as

pub struct InputCellID();
pub struct ComputeCellID();

to suggest the newtype pattern instead of the typedef pattern.

Interestingly, I believe that the tests don't care; they should work whether these are type aliases or newtypes. Still, if we're in this for the type safety, we may as well do it right.

[petertseng]

I would like to experiment with adding a compile_fail test (docmented in https://doc.rust-lang.org/stable/rustdoc/documentation-tests.html) to ensure IDs aren't mutually assignable.

[petertseng]

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

[coriolinus]

Sorry for the delay reviewing this. It looks good!

Exercises such as this form a useful jumping-off point for students into the kind of code they should expect to read and write in the real world, so I am very much in favor of the improved type safety here.

[coriolinus]

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.

[petertseng]

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.

[petertseng]

Thanks for reviewing!