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:

https://github.com/revery-ui/reason-reactify/blob/1f4801a8acda4da549239dad2cd958467bccff5c/lib/Reactify.re#L165

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 <[email protected] 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 https://github.com/revery-ui/reason-reactify/issues/47#issuecomment-451740958, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG2Ks2coNXRhyghnYZC3F88s5iB9vZsks5vAfc_gaJpZM4ZnGC_ .

[jaredly]

As for obj.magic - reason reactify needs it in "pushNewState" to handle hooks data. My impl doesn't use it anywhere.

On Sun, 6 Jan 2019, 7:00 am Jared Forsyth <[email protected] wrote:

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 <[email protected] 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 https://github.com/revery-ui/reason-reactify/issues/47#issuecomment-451740958, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG2Ks2coNXRhyghnYZC3F88s5iB9vZsks5vAfc_gaJpZM4ZnGC_ .

[jchavarri]

As for obj.magic - rreason reactify needs it in "pushNewState" to handle hooks data

I am aware. This is the whole reason why we're exploring the slots approach 😄 I was asking because of this comment:

I tried a version that didn't use continuations, but it required Obj.magic.

But if you had everything figured out, great!

[jchavarri]

@cristianoc Thinking about it more, one potential advantage of the continuation-passing style is that we could return the latest state value from the use* calls. This would kind of "close the circle", as we could read that value from the reconciler and pass it back in the next render. I believe this would allow to remove all side effects from Slots.use making it maybe more easily testable.

I haven't still figured out all the details, but here's the current state of this idea: https://gist.github.com/jchavarri/e90f1f49df51922989b200f81d925202

Although, this might be possible as well with the direct style...

[cristianoc]

@jchavarri Not sure I understand the comment, but slots gets filled the first time foo(slots) is called. And the next time foo(slots) is called, all the slots are already filled so foo gets access to them. In the case of state: any values can go into the slot, and will be available in the next invocation. Eg for a simple state implementation where state is set immediately, the slot is a reference. Next time foo(slots) is called, the same reference is found. The contents of the reference can be mutated.

[jchavarri]

I'm running into an issue with the slots polymorphism. When reconciling, we need a way to set the "initial slot" to None for new instances, or pass the previous slots for existing instance. This should happen around here.

But we don't want the instance to know the full type of the slots as that would complicate things. So I'm wrapping the slots type in a GADT opaqueSlots, but I can't make the render function compile when I try to unwrap it:

  let render = (id: ComponentId.t, lazyElement, ~children) => {
    ignore(children);
    let ret =
      Component(
        id,
        (OpaqueSlot(slots)) => {
          Effects.resetEffects(__globalEffects);
          let childElement = lazyElement(slots); /* This fails: "The type constructor $OpaqueSlot_'slot would escape its scope" */
          let children = [childElement];
          let effects = Effects.getEffects(__globalEffects);
          let renderResult: elementWithChildren = (
            children,
            effects,
            __globalContext^,
          );
          renderResult;
        },
      );
    ret;
  };

The alternative (not using GADT and storing the slots types "as is" in the instance) is prob not an option as that polymorphism would propagate across the instances which is problematic for children etc.

Is there a way to store the slots without exposing the polymorphism, and pass them down unpacked to the user defined function? I see @jaredly managed to do it by creating a record with an init function. In that case the unpacking of GADT seems to work, what is different in that case?

[jchavarri]

Hm, I was trying to create an isolated case and ran into a different issue 😅 When trying to add the slots the function used in createComponent there is an error The type of this packed module contains variables:

https://gist.github.com/jchavarri/0455be2984480313bd1800b14b01251c

[jaredly]

@jchavarri ah now I better understand your question. With the non-cps implementation, there's no way to "unwind" the slots after they've been created, so I wasn't able to get the "final" state of the slots to match the "initial" state of the slots that was expected. CPS made it so that the final state was the same type as the initial state

[jaredly]

@jchavarri as far as polymorphism, I pack the slots along with the render function, so that the render function is allowed to know about the slots type https://github.com/jaredly/fluid/blob/master/src/Fluid.re#L86

[jchavarri]

Thanks a lot @jaredly ! Your suggestions and the Fluid code (amazing progress btw! 😮 ), plus some hints from @cristianoc –who helped me offline– brought me back to the right path. The trick as both of you mentioned was to pack the slots with the render function. (aside: I wish this kind of "advanced OCaml folk knowledge" could be learnt upfront by reading some manual, book or docs that explain how these types work, but I guess this is the most efficient way to learn? 🤔 )

I have an isolated example with a similar setup to the one in reason-reactify, that shows how instantiation and rendering could work with these packed types: https://gist.github.com/jchavarri/470e94adc174e7a452711337d6324635

Now it is a matter of integrating these ideas into the existing instance and element types of reason-reactify 😄

With the non-cps implementation, there's no way to "unwind" the slots after they've been created, so I wasn't able to get the "final" state of the slots to match the "initial" state of the slots that was expected. CPS made it so that the final state was the same type as the initial state

@jaredly I'm not sure I understand this. With the slots implementation that @cristianoc shared in https://gist.github.com/cristianoc/88d28074c86c276c5e501b1cb61b0ce2, one doesn't need to unwind the slots, the type of the slots value passed to the render function is inferred "backwards" based on subsequent usages of hooks in that same render function. Because the slots are ultimately an option, one just needs to make sure to create a new slots on instantiation with a None value –as you're doing in Fluid already– and the rest should take care of itself. The "accumulation of slots types" happens then in the slots value, that gets passed around (instead of being returned by the CPS functions). The gist I pasted above has an example without CPS that reads and sets some state and then renders twice to make sure the state is updated normally.

[jaredly]

Oh you're totally right. I was missing having each slot be a ref. Looks great!