Skip to content

react: Investigate more type safety for input vs compute cells #473

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.

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

Closed
petertseng opened this issue Mar 17, 2018 · 4 comments
Closed

react: Investigate more type safety for input vs compute cells #473

petertseng opened this issue Mar 17, 2018 · 4 comments

Comments

@petertseng
Copy link
Member

A Reactor may issue CellIDs in response to either a create_input or create_compute.

A Reactor accepts CellIDs in the following circumstances:

  • create_compute so that the caller specifies the dependencies of the compute cell to be created. These dependencies may be either input or compute cells.
  • value. May be either input or compute cell.
  • set_value. Only valid for an input cell.
  • add_callback/remove_callback. Only valid for a compute cell. There is no technical barrier against an input cell having callbacks, but the specification does not call for it and it is never tested.

I believe it is technically feasible to have slightly more safety, as follows:

  • create_input issues only InputCellIDs.
  • create_compute issues only ComputeCellIDs
  • add_callback/remove_callback accept ComputeCellID only.
  • set_value accepts InputCellID only.
  • create_compute and value need to accept enum CellID { Input(InputCellID), Compute(ComputeCellID) }

Should this be done? Is this a reasonable API and/or any other reasons it's a good/bad idea?

If we do this, is it further possible to test at compile-time that InputCellID and ComputeCellID cannot be assigned to each other? Even if it is possible, should we do that? I encourage making them unassignable to each other for more safety, but I don't want students to feel too restricted.

If I do not hear objections or think of any myself, I will investigate what the code looks like. I will not be offended if there were initially no objections but objections come to light after the seeing the code.

@petertseng
Copy link
Member Author

Initial investigation of what the code looks like present in https://github.com/petertseng/exercism-rust/commits/react-cell-ids . Note that I based it off a post-#434 and post-#464 state, since there would otherwise be too many merge conflicts that I'm not willing to write two versions of.

Also note that as of this writing, now set_value should consider returning something other than SetValueError since there is only one failure mode (cell does not exist); the other failure mode (setting a compute cell) is eliminated by the type system. That has not yet been decided.

@coriolinus
Copy link
Member

A somewhat more ergonomic API IMO would be to define a trait HasValue and require that both ComputeCellID and InputCellID implement HasValue. This is useful pedagogically, as I believe this would become the only place in the Rust track in which students need to deal with defining their own trait types. it would also mean that create_compute and value could be defined to accept a T: HasValue, which makes actual use of the reactor a bit nicer.

It adds complexity, but also makes this code somewhat more like real production code. For a difficulty-10 exercise, I believe this to be an appropriate tradeoff.

@coriolinus
Copy link
Member

Having now actually attempted to implement an example of my API suggestion, I see that:

  • HasValue makes more sense applied to Cell than to CellID
  • CellID probably needs to remain an enum distinguishing the two cell types
  • There is already a comment on Reactor.value to the effect that putting the value on the cell type was investigated and discarded for reasons of implementation complexity.

I retain a suspicion that there exists an elegant API characterized by pub trait Cell<T: Copy> { fn value(&self) -> T; ... }, and that this API could operate in the type-safe manner proposed here without necessitating use of the CellID enum. However, it would involve completely redesigning the API expected of this exercise, and I do not currently have the time required to actually do it.

I therefore rescind my suggestion about using a HasValue trait; I don't see any clear ways to improve on the API suggested here without redesigning the entire thing.

@petertseng
Copy link
Member Author

One may see my original failed attempts to get value on cells in the commit history of #191 . I can't guarantee that they were good attempts and/or I tried everything that is possible, but there they are. As I learn more about what's possible in the language, I may try other things. For example, it is possible that std::cell::Cell may help here. I'm not yet convinced I want to force every implementation to use Cell, but I'll have to see the possibilities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants