Unexpected behavior when calling hooks a different # of times on a conditional branch

[jchavarri]

While #43 made some progress on solidifying hooks types to make their usage safer, there is still a case that @cristianoc brought up today that is not covered. For example, if one replaces the renderCounter function by:

let renderCounter = () =>
  if (Random.bool()) {
    useReducer(reducer, 0, ((count, dispatch)) =>
      <view>
        <button title="Decrement" onPress={() => dispatch(Decrement)} />
        <text> {"Counter: " ++ str(count)} </text>
        <button title="Increment" onPress={() => dispatch(Increment)} />
      </view>
    );
  } else {
    ignore(useReducer(reducer, 0, ((_count, _dispatch)) =>
      <view></view>
    ));
    useReducer(reducer, 0, ((count, dispatch)) =>
      <view>
        <button title="Decrement" onPress={() => dispatch(Decrement)} />
        <text> {"Counter: " ++ str(count)} </text>
        <button title="Increment" onPress={() => dispatch(Increment)} />
      </view>
    );
  };

(note the additional useState call on the second branch wrapped by ignore())

This code will compile, because in both branches we return hook((unit, reducer((int, action) => int))). But at runtime, the behavior becomes unexpected, because of the differences "state stacks" that are created between branches. In some cases (like if the ignored useState using a string-typed state) leading to runtime exceptions because of the different types of state[0] for that component.

@cristianoc also came up with this (amazing) idea that now that we know the shape of the state of each component in the form of nested tuples, we could change the underlying implementation to support this kind of behavior by moving away from the state stack model, into a new model where each hook has a reserved "slot" with the shape of the state needed.

If I understood correctly, the change would be that the hooks would return a "getter" and a "setter" for that individual piece of state, which means there would be no linear restriction about hooks anymore, as one would always get the "right paths" to read and write from them.

I believe that the implementation of these ideas could also lead to the removal of the Obj.magic that is currently used in State.re, but I have to noodle a bit more around this 🍜.

@bryphe Would you be open to more API changes to enforce a better safety? (even at the cost of diverging a bit from ReactJS hooks semantics...)

[jchavarri]

Adding here the great implementation ideas that @cristianoc proposed: pass a slots value around with some types + functions to allow each hook to read or write state atomically without the need of a "state stack":

https://gist.github.com/cristianoc/0cb8afcfca272f17bb001ffa26983126

[jchavarri]

@cristianoc One of the first questions that I have about your proposal is that the initial value of slots that needs to be passed to each component render function somehow needs to mimic the resulting shape from the usage of hook functions inside that render function.

For example, if a component does (very simplified):

let comp = slots =>
  useEffect(
    slots,
    () => print_endline("hello"),
    (nextSlots, ()) =>
      useState(
        nextSlots,
        3,
        (_nextSlots, (count, setCount)) => {
          setCount(count + 1);
          ();
        },
      ),
  );

(i.e. use useEffect and then a nesteduseStatecall)

Then the slots passed would need to have this shape:

let compSlot = 
  State.createSlot(slot2 => {
    let slot1 = Effect.createSlot();
    (slot1, (slot2, ()));
  });

One idea I had was that we could include another creation function createSlot and then match the type hook(h) in createComponent:

reason-reactify/lib/Reactify.re

Line 165 in 1f4801a

((unit => hook(h), ~children: list(emptyHook)) => emptyHook) => c,

But this would impose some extra burden on users, as they'd need to replicate the hooks function calls nesting structure into this createSlot function.

Is there a way to solve this "slot creation" problem automatically through some type system mechanism?

[cristianoc]

@jchavarri the issue with creating the initial value of slots is that one would need to know the type in advance, and the type is only known by the type checker.

An analogue

Forgetting about types for a second. If we were in an untyped world, and the data structure were an array, the natural solution would be to create an extensible array, which gets extended while we walk through it (without losing the identity, so next time the same array can be passed back to the functions). And we would pass the empty extensible array at the beginning.

Extensible slots

Now back to the current situation: the data structure is nested pairs (x, (y, (z, ...))) where the depth is known statically (to the type checker) but it is not known to the program. What we want here are extensible nested pairs, where new pairs are created dynamically as needed while we traverse the structure. Essentially, a heterogeneously typed linked list.

Initial values

There's the question of how to initialize slots, and the simplest way is to ask slot handlers to provide default values. Alternatively, one would make all slots be of option type, and initialize with None. Using defaults seems more general, as each slot handler can decide what to do, and using None is one specific decision.

Here's a gist, which covers the simplest possible runnable examples: slots of type int and string:
https://gist.github.com/cristianoc/8f69c833a94e13a8ed659c76c97b8b20

[jchavarri]

Woah @cristianoc this is awesome!! I think there is now a clear path to add a initialSlot property to each instance, that will be stored and passed every time there needs to be a render, and then we'd be good to go I guess 🎉

I will take a stab at implementing this solution in reactify in the next days 🔨 Thanks @cristianoc !

[bryphe]

This sounds like a great approach! Thanks @cristianoc for the proposal and gist, and @jchavarri for implementing it 😄 Looking forward to it.

I think this actually will solve a variety of 'technical debt' we have around our current-hacky-__global-state-solution. Wasn't happy with the way we shim state in and out of __globalState, and the entire State.re module we have today felt like a messy hack - this proposed approach is much better/cleaner across the board 🙏

[jchavarri]

@cristianoc @bryphe I've started implementing something in the slots branch (not much to see yet...), and I have one thought I'd like to share:

After the latest changes towards the "extensible slots model", what'd be the purpose of the continuation-based approach?

If we have to thread the slots value around the different calls to hooks, then I don't see the upsides on keeping the continuation passing style, which had as original goal to increase the type safety while replacing the need to add that precise threading of slots.

The accumulated tuple types generated by hooks usages would then live in the slots values passed around, and we could simplify a lot the different functions as we wouldn't need to return hook('t), just an element.

Do you see any downsides of removing the continuation?

[cristianoc]

@jchavarri indeed one can go direct style, and do something like:

  let (x, slots) = slots |> useInt;
  let (s, slots) = slots |> useString;

Still need to return slots to get access to the next slots.

Here's an updated gist: https://gist.github.com/cristianoc/88d28074c86c276c5e501b1cb61b0ce2.

[jaredly]

Ok so I've been banging on this today, and I've got something working!
Here's an example component https://github.com/jaredly/fluid/blob/master/src/Hooks.re#L145 (below it is the "un-ppx'd" version.
And there's no Obj.magic involved! I tried a version that didn't use continuations, but it required Obj.magic.
Also, I think having a ppx makes sense anyway, to enforce things like "don't put hooks inside of conditionals"

[jchavarri]

The main problem that I'm finding with the new slots-based model is that the value of nextSlots is always left as variable.

This is failing when trying to unify types when calling createComponent because the remaining nextSlots are left as variable, which leads to a compilation error: Error: The type of this packed module contains variables.

Here's a gist with an isolated case for it:
https://gist.github.com/jchavarri/898232f3568f86b0a593269efb8461a0

I was thinking maybe about having a useStateFinal? So in that case there would be no nextSlots returned, but maybe there is a better way 🤔

[cristianoc]

@jchavarri not sure about the general context, but for that example, this takes care of the type error:

/* SlotsType:   type empty = t(unit,unit); */
let (state, _nextSlots : Slots.empty) = useState(3, slots);

Or, useStateFinal would also do it.

[jchavarri]

Aaah figured it out, it was exactly the second case you mentioned @cristianoc, I just had to ignore that type on the function signature, this example with direct style compiles! https://gist.github.com/jchavarri/ca0ec4f791c399fa3fba6bf8587b8755 🚀

@jaredly I am curious why the case without continuation doesn't work for fluid, in theory it should work just fine in both cases (or not at all in both). Where did you have to use Obj.magic in the non-continuation case?

[cristianoc]

@jaredly: I was wondering about this

to enforce things like "don't put hooks inside of conditionals"

That restriction is not actually needed when hooks are handled in a type-safe way.
Specifically, I'm thinking about:

if(showCounter) { renderCounter(hooks) }

instead of retrieving the state outside the conditional and do nothing with it.

[jaredly]

I don't think putting hooks in conditionals makes logical sense, even though we can make sure it works type-wise

…

On Sun, 6 Jan 2019, 6:16 am Cristiano Calcagno ***@***.*** wrote: @jaredly <https://github.com/jaredly>: I was wondering about this to enforce things like "don't put hooks inside of conditionals" That restriction is not actually needed when hooks are handled in a type-safe way. Specifically, I'm thinking about: if(showCounter) { renderCounter(hooks) } instead of retrieving the state outside the conditional and do nothing with it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#47 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAG2Ks2coNXRhyghnYZC3F88s5iB9vZsks5vAfc_gaJpZM4ZnGC_> .