`Signal::set` needs to panic instead of silently failing on double-borrow

In this draft PR https://github.com/maciejhirsz/kobold/pull/55, I was trying to see if I could use the get function implementation of Hook https://docs.rs/kobold/latest/kobold/stateful/struct.Hook.html#method.get just for fun. It states that its used to "Get the value of state if state implements Copy". I used a state that is just a struct State that has a my_state property with a bool type so it can implement the Copy trait, since if the State was more complex and had properties of types that don't automatically implement the Copy trait (i.e. Vec, String, Range, Box) then you get errors like the following, which were unnecesassarily complicated for me to resolve when I just wanted to a simple example of using the Hook's get function :

error[E0204]: the trait `Copy` may not be implemented for this type
  --> examples/invoice/src/state.rs:33:32
   |
33 | #[derive(PartialEq, Eq, Clone, Copy, Debug, Serialize, Deserialize)]
   |                                ^^^^
...
39 |     pub qr_code: String,
   |     ------------------- this field does not implement `Copy`
   |
   = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0204]: the trait `Copy` may not be implemented for this type
  --> examples/invoice/src/state.rs:41:32
   |
41 | #[derive(PartialEq, Eq, Clone, Copy, Debug, Serialize, Deserialize)]
   |                                ^^^^
42 | pub struct Entry {
43 |     pub description: String,
   |     ----------------------- this field does not implement `Copy`
   |
   = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0204]: the trait `Copy` may not be implemented for this type
  --> examples/invoice/src/state.rs:47:32
   |
47 | #[derive(PartialEq, Eq, Clone, Copy, Debug, Serialize, Deserialize)]
   |                                ^^^^
...
50 |     pub columns: Vec<Text>,
   |     ---------------------- this field does not implement `Copy`
51 |     pub rows: Vec<Vec<Text>>,
   |     ------------------------ this field does not implement `Copy`
   |
   = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0204]: the trait `Copy` may not be implemented for this type
  --> examples/invoice/src/state.rs:54:32
   |
54 | #[derive(PartialEq, Eq, Clone, Copy, Debug, Serialize, Deserialize)]
   |                                ^^^^
55 | pub enum Text {
56 |     Insitu(Range<usize>),
   |            ------------ this field does not implement `Copy`
57 |     Owned(Box<str>),
   |           -------- this field does not implement `Copy`
   |
   = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0204]: the trait `Copy` may not be implemented for this type
   --> examples/invoice/src/state.rs:204:32
    |
204 | #[derive(PartialEq, Eq, Clone, Copy, Debug, Serialize, Deserialize)]
    |                                ^^^^
205 | pub struct TextSource {
206 |     pub source: String,
    |     ------------------ this field does not implement `Copy`

So my first thought was, is the intention for developers whose State has properties that don't automatically implement the Copy trait to try to get them to implement the Copy trait if they want to use the Hook's public get function (even though it might require changes to the Kobold library itself)? Or is the intention for it to just be used by developers with simple projects where the State has properties that automatically implement the Copy trait?

Then when I was playing with trying different parts of the docs, I tried to use stateful::Signal public function .set https://docs.rs/kobold/latest/kobold/stateful/struct.Signal.html#method.set, which says it's used to "Replace the entire state with a new value and trigger an update.", but when I tried using it to change the state, it actually didn't change the state at all as expected, i thought it would have set the my_state value to true. See screenshot below of UI output:

Note: I also tried using update, but that doesn't appear to have worked either for some reason even though I thought I'd been using it previously without issues

The set and get are working somewhat similar to std::cell::Cell here. The only time you really need the get method is if your entire state is something like count: &Hook<u32> and you want to get the u32 by value. Since Hook<T> implements Deref<Target = T> you can copy a value out of it by doing **count (one * to deref the &, the other to deref the Hook), count.get() is just a convenience method to make that slightly cleaner and to show the intent clearer.

You can't implement Copy on a type that has Strings or Vecs in it as that would violate ownership rules: String is a smart pointer that owns the memory it points to, when you clone it a new allocation is made, the content is copied to the new allocation, and then you get a new String with a new pointer. This is different from, say, &str which is a shared reference so the pointer can be just copied on stack with no issues.

If you have a state: &Hook<State> where State has a my_state field that has a bool in it, you can get that bool just by invoking state.my_state and Deref will take care of the rest. This derefencing is free after optimizations (the references &Hook<State> and dereferenced &State are the same pointer to the compiler).

As to the Signal::set: do you have a link to where you use it? Chances are it fails silently because you're trying to set state while already having a mutable reference to it. This needs to panic (at least in debug mode), I just haven't gotten to it yet.

As to the Signal::set: do you have a link to where you use it?

yes i created this draft PR to illustrate how i encountered the Signal::set issue https://github.com/maciejhirsz/kobold/pull/55/files

i mock the state here here so state.my_state has a value false when injected into the Hello component here.

and i pass a prop new_state with value true into here

it correctly gets state.my_state as false here,

but then i run signal.update(|state| state.toggle());, which should toggle the value of state.my_state to be true here, but that didn't work, as it remained false

then i also tried signal.set(State { my_state: new_state }); here, which should set the value of state.my_state to be true, but that didn't work either, it remained false

Ye, that will be a panic on runtime, it just happens to fail silently atm. You cannot set a state inside a view, stateful effectively borrows the state internally* to give you the immutable &Hook<State> reference, and you cannot have a mutable reference while an immutable reference exists at the same time, that violates Rust memory safety.

Even if it were safe, it's still a bad practice and a common source of bugs in other languages. If you try this in React you would either get an exception or an infinite loop / stack overflow in analog scenario (setting a state triggers a render which sets a state which triggers a render...)

I might remove the method to get a Signal inside the view altogether, and just give you a way to bind async event handlers that use a Signal internally so you can't shoot yourself in the foot like this even if you try to.

*) That's only partially true. On top of borrowing rules, on the first render the Hook contains a weak pointer to yet-to-be-initialized memory that it needs to actually do the mutable borrow from.

maciejhirsz / kobold

`Signal::set` needs to panic instead of silently failing on double-borrow #56