`UseStateHandle` can become stale

[mc1098]

It was mentioned in #2109 about how the UseStateHandle can contain stale data - this is the case because in use_state we replace the Rc<T> on each set which means any Rcs that handles are holding are now stale. The handle keeps the Rc in scope so it will always be valid but this isn't what most people would expect.

The reason this goes somewhat unnoticed is that we often re-create Callbacks so will move in the updated UseStateHandle so that the value is valid, however, when you pass it to something like an Agent or the router listener callback then it is a single Callback that never gets updated and whatever the state was when that handle was passed in is what it will remain to be.

So solving this issue is actually bit difficult without causing ergonomic pain (unless I'm missing something, and I hope I am). If we used a Rc<RefCell<T>> with use_state and then encapsulated that in the UseStateHandle then our state could remain in sync which is good.
We would have to make sure that users of handle could not mutate the state other than the set fns. UseStateHandle could no longer impl Deref and would have to have a borrow function like RefCell so we could enforce the borrow lifetime constraints for exclusive mutability when we actually run the setter. I think if we uphold the lifetime constraints we can guarantee that we won't panic when trying to get a mutable borrow of state.

We could dip our toes into some unsafe 👀 and use UnsafeCell in order to still impl Deref and avoid the RefCell runtime borrow checks - I haven't thought too much on the safety argument but I think it would be safe to do this as we'd only give out references to T with a lifetime that is bound to the UseStateHandle so would prevent someone from deref -ing state then passing it to a Callback as they all require 'static lifetimes. We can then happily mutably borrow the state in the setter because this is within the scheduler so we can't have outstanding borrows to state because we require 'static lifetimes to store anything.

I'd love to hear from others regarding the above and whether I missed something or if there is a better way to it all :) We can either try and resolve it or document it - but the latter would be a shame because I think this behaviour will catch people out.

[voidpumpkin]

@mc1098 I would love to attempt to solve this but I'm not sure how to reproduce this.

Could you provide a small example that i could use as a base?

[mc1098]

@voidpumpkin small and contrived example :)

Funny enough, I actually think someone might have come across this before because in the docs for Custom Hooks the example uses a use_effect which will update the Callback each time. I will say, that I have touched that example a couple of times while messing around with the docs but never quite noticed it till now 🤦

[voidpumpkin]

@mc1098 so then perhaps this issue can be closed?

[mc1098]

@voidpumpkin Nope 🙃.

Using the use_effect is not a great solution because it keeps building bridges on each render. When #2116 is resolved then you will have the same issue with the router callback. I also think this will pop up more and more as we try and integrate with JS APIs that are event based.

I'd like to keep this issue open to explore how we could change use_state to allow passing in state handles that can remain in sync with the hook state so that you don't have to keep refreshing the Callbacks after each set.

[futursolo]

I guess we need to add a separate use_cell hook to solve this.

pub struct UseCellHandle<T> {
    value: Rc<RefCell<Rc<T>>>,
    setter: Rc<dyn Fn(T)>,
}

impl UseCellHandle<T> {
    fn set(&self, value: T) {
        (self.setter)(value)
    }

    fn get(&self) -> Rc<T> {
        (*self.value.borrow()).clone()
    }

    // Maybe also get_owned() and take() ?
}

impl UseCellHandle<T>
where
    T: PartialEq
{
    fn set_if_neq(&self, value: T) {
        if (**self.value.borrow()) != value {
            self.set(value)
        }
    }
}

This hook should behave like Rc<Cell<Rc<T>>> but using a RefCell<Rc<T>> to make sure that a fresh value is always available.

I guess it's better to make get() to return Rc<T> because it very easy to write code that holds a Ref<T> in spawn_local or destructor of use_effect that causes the application to panic if other handles is trying to borrow in the meantime.

Also I am not sure if we should rename set_if_neq to set_if_ne or set_ne since in Rust the counterpart of eq is ne not neq.

In addition, I have read the implementation of useState in React, and it's implemented internally with useReducer which means that it dispatched as an action back into the reducer function and hence can update reliably and a reducer won't trigger re-render if the state is equal.

React guarantees that dispatch function identity is stable and won’t change on re-renders. This is why it’s safe to omit from the useEffect or useCallback dependency list.

If you return the same value from a Reducer Hook as the current state, React will bail out without rendering the children or firing effects.

https://reactjs.org/docs/hooks-reference.html#usereducer

However, Yew's use_reducer will always trigger a re-render which I guess might also cause issues in the future.

I guess the proper way to solve this is:

• Remove use_reducer
• Rename use_reducer_with_init to use_reducer and remove InitialState so the API is consistent with use_state
  (You can move surrounding values into the closure for initial values if you need one.)
  (Also not sure why InitFn is not FnOnce🤔.)
• Add a use_reducer_eq hook that requires the state to implement PartialEq and only updates when the state changes.
• Create dispatch only once so dispatch function won't change between re-renders.

And reimplement the following hooks with use_reducer or use_reducer_eq with a single setter set:

• use_cell (re-render is always triggered when set.)
• use_cell_eq (re-render is only triggered when new value differs from the old one)
• use_state
• use_state_eq

_eq variant is separated from the non-_eq'ed variant so we don't have to box 2 fns and don't have to worry about how the _eq dispatch / setter can be typed if state does not implement PartialEq.

Make a distinction between use_state and use_cell that:

• handles of use_state will notify a render and dereferences to the value at the time of rendering / handle creation.
• handles of use_cell will notify a render when a new value is set, a value can be retrieved using .get() and it always returns a reference of current value.

In addition, add documentation to discourage manual checking when setting values of use_state and prefer implementing PartialEq on state or use use_cell or use_reducer_eq instead.

Optionally, add a get_setter() that clones the setter function, should one prefer to register the setter as a dependency with use_effect_with_deps? (Use Rc::ptr_eq for equality)

I am not very decided on the naming, the ones I can come up with: use_cell_eq, use_eq_cell, use_equal_cell, use_ref_state, use_state_ref

I guess this implementation should address:

• Cost of having to box 2 setters
• state values going stale
• Setter is not functioning consistently
• Reducer will always re-render

[futursolo]

@mc1098 I have file a two-part PR that intends to solve this issue. Could you please take a look and let me know the thoughts?