Add React #191

petertseng · 2016-08-29T02:40:40Z

I felt it would be educational for me to try to implement this in Rust.

Edit: After completing it: Very much so. Lifetimes, closures, storing closure in structs (you have to be generic over the closure type, and give it a lifetime, also understand FnMut vs Fn)

Tests will be taken from the Go track. In case you wonder why there isn't a JSON file... the JSON file for this problem probably at best can only have rough descriptions of how the tests should go.

~~In some instances, I am unsure of the API that should be presented to students. When I am unsure, I will solicit feedback. After that, I'll make a stub file.~~

Edit after having attempted the problem many ways: I'm pretty sure this interface is simple and works. Whereas an interface that gives direct access to the cells is fraught with problems. You can see some failed attempts in the commit history.

Reviewers, please pay attention to the following questions. I am satisfied with my current answer to all of them, but would gratefully accept ideas on them.

API OK, with value and set_value? My ideal is actually get and get_mut as I noted in the stub files, but I attempted and failed to implement such an API. If any can provide an implementation for get and get_mut I strongly gladly using it and making that the API.
Any way to remove the requirement of Copy? I tried and failed, and I honestly don't think it's worth it, but maybe you think differently!
Any way to shorten the amount of time that the test callbacks borrow the vectors they write into? I didn't see any so I'm satisfied with the current implementation, but maybe someone else has one.
Does anyone care whether remove_callback on an already-removed callback should be Ok or Err? I simply didn't test the exact result, only that it doesn't interfere with other callbacks.
Should we keep the reactor generic over the type it reacts on, or go back to just ints like the other tracks? However, even if we go back to ints the interface still has to use generics for the Fn and FnMut, so we can't avoid them completely. But maybe we want to decrease the amount of work required as much as we can.

petertseng · 2016-08-29T02:43:41Z

exercises/react/tests/react.rs

+    let mut reactor = react::Reactor::new();
+    let mut input = reactor.create_input(1);
+    assert_eq!(input.value(), 1);
+    input.set_value(2);


From what I read of https://www.reddit.com/r/rust/comments/2uvfic/why_doesnt_rust_have_properties/, this is slightly un-Rusty, but one point made is "Don't add ways to access/modify data unless it's absolutely necessary." and I believe in this case it is necessary since when a value is set the cell needs to propagate the value to any dependents. This isn't possible if the interface is a simple input.value = 2

petertseng · 2016-08-30T01:36:09Z

I'm going to need to back off from this until I understand lifetimes better, it seems! Maybe I'll implement a different problem in the meantime.

petertseng · 2016-09-01T08:33:10Z

Most of the intermediate commits are useless so I'm going to squash a few of them. The only interesting ones are the ones with my failed attempts, so that reviewers have an idea of what didn't work. And I fully expect that everything will get squashed when merging.

because the value returned by compute only lives for the stack frame, I believe.

This reverts commit 4d95619.

Lifetime, because ComputeCell needs to live at least as long a the Reactor storing it. I don't think this is an insurmountable problem, but I'm not sure where to put the lifetime annotations now

This reverts commit 13886af.

NOPE, then you can't return anything from a compute cell

This reverts commit 88f2463.

petertseng · 2016-09-01T08:46:13Z

I have completed the example implementation (which will probably be the implementation I submit to the site) and the stub so I am removing WIP and saying it's ready for review.

petertseng · 2016-09-01T09:45:16Z

exercises/react/src/lib.rs

+    // Return an Err (and you can change the error type) if the cell does not exist.
+    //
+    // If the callback does not exist (or has already been removed), it is up to you whether to
+    // return Ok(()) or an error, as it cannot be tested.


not true. repeatedly calling remove on the same callback ID can test this. Only question is whether we should:

Test it and expect Ok(())

Test it and expect is_err()

Test it but don't expect any particular value

Not test it.

In any case, change the comment.

petertseng · 2016-09-01T19:32:43Z

Open question about example implementation and the type system: Is it possible to make it so that the callbacks' borrows end when they are removed, which would be earlier than when the reactor goes out of scope?

In general I'm not so sure this is possible in the type system, since the type system has no way of knowing what the remove_callback call is doing.

petertseng · 2016-09-01T20:03:31Z

exercises/react/src/lib.rs

+    dummy: T,
+}
+
+// You are guaranteed that Reactor will only be tested against types that are Copy + PartialEq.


open question: Can the requirement of Copy be removed? It was very difficult for me to see how.

Thinking in terms of what would have to change in the example impl. I don't have rustc to help me verify any of this right now though. I'll verify in a few hours.

If T is not Copy and create_input is made to pass an &T:

then the best Cell can do is to also store the &T

But then, if the compute functions return T, how will that be dealt with? Perhaps https://doc.rust-lang.org/std/borrow/enum.Cow.html ? But that means T has to be ToOwned. Still, maybe that's better

On the other hand if the compute functions return &T... but how is that possible? Where can they store a T that will live long enough? I don't think that's possible.

If T is not Copy and create_input still passes a T:

I think the cell can take ownership for value, and a reference for last_value.

I don't understand how that can work - when value gets replaced, what keeps last_value alive?

I guess I'll see what happens when I try it.

Callbacks and compute functions have to take &T, that should be no problem.

At the very least, Clone is more general than Copy (https://doc.rust-lang.org/std/clone/trait.Clone.html, and see how you must be Clone to Copy: https://doc.rust-lang.org/std/marker/trait.Copy.html), so perhaps the Copy can be relaxed to Clone?

not necessarily what we want, because clone might be expensive and copy not, as https://doc.rust-lang.org/std/clone/trait.Clone.html describes. So maybe Copy is what I want.

The difference is currently academic. We only test against usize right now. I would like to not worry about this too much.

If T is not Copy and create_input still passes a T:

I think the cell can take ownership for value, and a reference for last_value.

Illegal.

src/lib.rs:23:26: 23:33 error: `initial` does not live long enough src/lib.rs:23 last_value: &initial, ^~~~~~~ src/lib.rs:21:60: 30:6 note: reference must be valid for the lifetime 'a as defined on the block at 21:59... src/lib.rs:21 fn new(initial: T, cell_type: CellType<'a, T>) -> Self { ^ src/lib.rs:21:60: 30:6 note: ...but borrowed value is only valid for the scope of function body at 21:59 src/lib.rs:21 fn new(initial: T, cell_type: CellType<'a, T>) -> Self {

Of course. Since initial is the parameter to that function. Doh.

OK in that case I don't care enough. I'm leaving it as Copy.

IanWhitney · 2016-09-02T11:57:10Z

A lot going on in here. Interesting. I probably won't be able to dig into this code for a few days, but my glance at the public API seemed ok.

get and get_mut had too many problems: once you set_value on a cell, how does it know to propagate its changes to its dependents? It either needs references to its dependents or its reactor.

This will make the exercise harder. Students will have to figure out that they need to annotate the Fn and FnMut types. (Or maybe they'll be more clever than I was...)

petertseng · 2016-09-03T09:47:22Z

I had a question about "hey, Reactor is generic over T, so should we have a test that exercises that?" and I answered it myself with "yes!". So a test with booleans was added.

IanWhitney · 2016-09-05T12:56:42Z

What is left on your to-do list for this one, @petertseng?

petertseng · 2016-09-05T16:31:29Z

I have no implementation-related TODOs, so let me review the questions I've asked to see whether they've gotten answered.

petertseng · 2016-09-05T16:47:01Z

Edited questions of interest into the PR description. Summary:

API OK, with value and set_value?
Any way to remove the requirement of Copy?
Any way to shorten the amount of time that the test callbacks borrow the vectors they write into?
Does anyone care whether remove_callback on an already-removed callback should be Ok or Err?
Should we keep the reactor generic over the type it reacts on, or go back to just ints like the other tracks?

If the answer to all these questions is "what's in this PR is OK", then I'd be inclined to merge.

IanWhitney · 2016-09-07T18:14:28Z

API OK, with value and set_value?

I think this is fine.

Any way to remove the requirement of Copy?

Since we're saying that the inputs are always going to be primitives I think Copy is fine.

Any way to shorten the amount of time that the test callbacks borrow the vectors they write into?

I'm not sure what this means. The kinda-weird extra scope in some of the tests?

Does anyone care whether remove_callback on an already-removed callback should be Ok or Err?

Well, we have tests that say we can remove callbacks multiple times, so OK would seem to be the right approach. But if removing a callback more than once isOK, then why is removing a callback on a non-existent cell an Err?

Should we keep the reactor generic over the type it reacts on, or go back to just ints like the other tracks?

I'm good with generic. You've already positioned this problem at the end of the Rust track (which is appropriate), so generics won't be new.

petertseng · 2016-09-07T18:29:27Z

Any way to shorten the amount of time that the test callbacks borrow the vectors they write into?

I'm not sure what this means. The kinda-weird extra scope in some of the tests?

Correct. Currently, I don't see a way to avoid it, so I'm going to leave it.

Well, we have tests that say we can remove callbacks multiple times, so OK would seem to be the right approach. But if removing a callback more than once isOK, then why is removing a callback on a non-existent cell an Err?

What I hear from this comment is "it's inconsistent that one of these is OK and the other Err".

Okay, in that case should the API change to this:

// If both the specified callback and the specified cell exist, removes the callback and returns true.
// Otherwise, returns false.
pub fn remove_callback(&mut self, cell: CellID, callback: CallbackID) -> bool {
    unimplemented!()
}

This seems to make sense to me.

IanWhitney · 2016-09-07T19:27:03Z

What I hear from this comment is "it's inconsistent that one of these is OK and the other Err".

Oh, my opinion's not that strong. Maybe there's a good reason to return an Err in that case. I was just asking.

I'm fine with either approach.

petertseng · 2016-09-08T06:51:48Z

Hmm. HashSet#remove returns just a bool. With a set though you're just passing in the one parameter (the object to remove), versus here where we pass the callback and the cell. Therefore I use Ok/Error instead of bool so that the student can distinguish between the two cases (callback doesn't exist, cell doesn't exist) if desired.

(Aside: We could imagine an API in which you would only need to pass in the Callback ID to remove_callback - the reactor would be required to keep track of what cell is associated with what callback ID. I'm going to not impose the extra bookkeeping on the students, unless someone convinces me otherwise)

Anyway, so I think I'll say it should be an error to remove a nonexistent callback (including if it was already removed).

Why did I think it would be OK for it to be OK? I don't know, something about idempotence. The rationale isn't even fully formed in my head. and now I think it's a bad idea because then someone might reasonably ask what if you send a callback ID that has never been added to that cell? Should it behave differently than a callback ID that was added in the past but was removed? And if it should be different, why?

So, error if the callback doesn't exist.

IanWhitney · 2016-09-08T13:18:55Z

Ok. I think we're good.

petertseng · 2016-09-08T19:24:04Z

Thanks, I think so too. I'll use the squash + merge button for this one.

petertseng · 2016-09-14T04:53:49Z

exercises/react/example.rs

+        let new_id = self.cells.len();
+        for &id in dependencies {
+            match self.cells.get_mut(id) {
+                Some(c) => c.dependents.push(new_id),


Unsafe. If this function gets called with a valid dependency followed by an invalid one, an entry is added to the valid cell's dependents, and then the dependent cell is not created. We have to check all cells' validity first.

petertseng reviewed Aug 29, 2016
View reviewed changes

petertseng changed the title ~~WIP DO NOT MERGE: Add React~~ Add React Sep 1, 2016

petertseng added 9 commits September 1, 2016 01:35

react: add input cells and change their values

427bda5

react: topics

8735dfc

in case you were wondering, this doesn't live long enough

a47d0f1

because the value returned by compute only lives for the stack frame, I believe.

Revert "in case you were wondering, this doesn't live long enough"

00c2101

This reverts commit 4d95619.

Maybe I can have the reactor track all compute cells? NOPE

eff2501

Lifetime, because ComputeCell needs to live at least as long a the Reactor storing it. I don't think this is an insurmountable problem, but I'm not sure where to put the lifetime annotations now

Revert "Maybe I can have the reactor track all compute cells? NOPE"

34fd901

This reverts commit 13886af.

let's have cells and epochs and the cell caches a value...!

b284baf

NOPE, then you can't return anything from a compute cell

Revert "let's have cells and epochs and the cell caches a value...!"

ccda60d

This reverts commit 88f2463.

react: try get/get_mut interface

b257e7e

petertseng reviewed Sep 1, 2016
View reviewed changes

petertseng mentioned this pull request Sep 3, 2016

react: Add JSON test data exercism/problem-specifications#358

Merged

petertseng added 5 commits September 3, 2016 02:32

react: give up on get and get_mut in favor of value and set_value

f0d4de3

get and get_mut had too many problems: once you set_value on a cell, how does it know to propagate its changes to its dependents? It either needs references to its dependents or its reactor.

react: stub

3a64f1f

react: add error tests

18b1a76

react: RFC: remove the input lifetime from the stub???

b4996d9

This will make the exercise harder. Students will have to figure out that they need to annotate the Fn and FnMut types. (Or maybe they'll be more clever than I was...)

react: Add boolean values test

6e7b392

react: it's an error to remove a nonexistent callback

2602332

petertseng merged commit 165e2cc into exercism:master Sep 8, 2016

petertseng mentioned this pull request Sep 11, 2016

Are stub lib.rs files a good idea? #200

Closed

petertseng reviewed Sep 14, 2016
View reviewed changes

petertseng deleted the add-react branch January 4, 2017 12:21

petertseng restored the add-react branch January 4, 2017 12:21

petertseng deleted the add-react branch January 4, 2017 13:07

petertseng mentioned this pull request Jan 18, 2017

react: add to track exercism/kotlin#29

Merged

This was referenced Apr 4, 2018

react: return domain-specific errors #464

Merged

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add React #191

Add React #191

petertseng commented Aug 29, 2016 •

edited

Loading

petertseng Aug 29, 2016

petertseng commented Aug 30, 2016

petertseng commented Sep 1, 2016 •

edited

Loading

petertseng commented Sep 1, 2016

petertseng Sep 1, 2016 •

edited

Loading

petertseng commented Sep 1, 2016

petertseng Sep 1, 2016

petertseng Sep 1, 2016

petertseng Sep 1, 2016

petertseng Sep 1, 2016

petertseng Sep 1, 2016

IanWhitney commented Sep 2, 2016

petertseng commented Sep 3, 2016

IanWhitney commented Sep 5, 2016

petertseng commented Sep 5, 2016

petertseng commented Sep 5, 2016 •

edited

Loading

IanWhitney commented Sep 7, 2016

petertseng commented Sep 7, 2016 •

edited

Loading

IanWhitney commented Sep 7, 2016

petertseng commented Sep 8, 2016 •

edited

Loading

IanWhitney commented Sep 8, 2016

petertseng commented Sep 8, 2016

petertseng Sep 14, 2016

Add React #191

Add React #191

Conversation

petertseng commented Aug 29, 2016 • edited Loading

petertseng Aug 29, 2016

Choose a reason for hiding this comment

petertseng commented Aug 30, 2016

petertseng commented Sep 1, 2016 • edited Loading

petertseng commented Sep 1, 2016

petertseng Sep 1, 2016 • edited Loading

Choose a reason for hiding this comment

petertseng commented Sep 1, 2016

petertseng Sep 1, 2016

Choose a reason for hiding this comment

petertseng Sep 1, 2016

Choose a reason for hiding this comment

petertseng Sep 1, 2016

Choose a reason for hiding this comment

petertseng Sep 1, 2016

Choose a reason for hiding this comment

petertseng Sep 1, 2016

Choose a reason for hiding this comment

IanWhitney commented Sep 2, 2016

petertseng commented Sep 3, 2016

IanWhitney commented Sep 5, 2016

petertseng commented Sep 5, 2016

petertseng commented Sep 5, 2016 • edited Loading

IanWhitney commented Sep 7, 2016

petertseng commented Sep 7, 2016 • edited Loading

IanWhitney commented Sep 7, 2016

petertseng commented Sep 8, 2016 • edited Loading

IanWhitney commented Sep 8, 2016

petertseng commented Sep 8, 2016

petertseng Sep 14, 2016

Choose a reason for hiding this comment

petertseng commented Aug 29, 2016 •

edited

Loading

petertseng commented Sep 1, 2016 •

edited

Loading

petertseng Sep 1, 2016 •

edited

Loading

petertseng commented Sep 5, 2016 •

edited

Loading

petertseng commented Sep 7, 2016 •

edited

Loading

petertseng commented Sep 8, 2016 •

edited

Loading