Skip to content

Proposal: Move to a single index for hook types #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Sep 6, 2020

Conversation

thomashoneyman
Copy link
Owner

@thomashoneyman thomashoneyman commented May 15, 2020

I’ve had some time to work with an alternate Hooks implementation which only uses a single index for hook types instead of two (as is done with the indexed free monad).

Compare:

-- New approach
foreign import kind HookType

newtype Hook m (h :: HookType) a = Hook (Free (UseHookF m) a)

foreign import data Hooked :: HookType -> HookType -> HookType
infixr 1 type Hooked as <>

bind 
  :: forall h h' m a b
   . Hook m h a 
  -> (a -> Hook m h' b) 
  -> Hook m (h <> h') b

In this approach a Hook is a free monad of the algebra UseHookF and bind appends hooks with <>. The new approach models hooks like a list: as a series of appends.

But the old approach had more going on:

-- Old approach
type Hook m (newHooks :: Type -> Type) a =
  forall hooks. Hooked m hooks (newHooks hooks) a

newtype Hooked m hooks newHooks a = 
  Hooked (Indexed (Free (UseHookF m)) hooks newHooks a)

bind 
  :: forall a b x y z m
   . Hooked m hooks hooks' a 
  -> (a -> Hooked m hooks' newHooks b) 
  -> Hooked m hooks newHooks b

Improvements

Switching to a free monad + phantom type instead of an indexed free monad makes a few improvements to how Hooks are written.

Hooks are now written in the order they occur

I find it irritating to define Hooks in the reverse order from how they appear in the Hook definition, and the same is true of type errors — Hooks get parsed backwards, so you have to flip the error in your head.

Compare:

-- UseState, then UseEffect, then UseRef in the code

-- this reads backwards, as state transitions 'away from' the
-- hooks type variable
UseRef Int 
  (UseEffect 
  (UseState Int hooks))

-- this reads in the order hooks are applied in the code, where
-- `Nil` represents the call to `pure`
UseState Int
  <> UseEffect
  <> UseRef Int
  <> Hooks.Nil

You can now use Hooks.pure in place of a particular list of Hooks

As @Vladciobanu noted on the functional programming Slack, it seems like you ought to be able to use Hooks.pure to return a Hook’s result, regardless of the HookType of the hook.

nothing :: Hook m (UseAPI T) (Maybe T)
nothing = Hooks.pure Nothing

This ought to compile, but it doesn’t under the indexed monad. With the simple free monad approach this is now accepted.

Fewer type variables to understand

Hooks are now just a free monad with a phantom type variable, instead of being a full-on indexed monad. Previously, you had to leave a hooks variable free when defining any Hooks:

type UseX' hooks = UseState Int (UseEffect hooks)

newtype UseX hooks = UseX (UseX' hooks)

derive instance newtypeUseX :: Newtype (UseX hooks) _

Under the hood, the Hook m hookType a was actually Hooked m hooks (hookType hooks) a, which was confusing to see in error messages instead of seeing the Hook type. But all this underlying type was ever used for was to apply Unit as the hooks type when you turned your Hook into a component.

It was easy to forget the hooks type and see obscure errors, or not be sure where the hooks type would go in the nested parentheses, and so on (see JordanMartinez/learn-halogen#78 (comment) as well).

 while checking that expression wrap
    has type Hooked m0 hooks1 (UseInitializer hooks1) Unit

  in value declaration useInitializer

Now, there is no type synonym — Hook is just Hook, without the indexed monad. Rather than nested parentheses you can use the <> type operator to append hook types, where you can use Nil to represent a call to pure.

type UseX' = UseEffect <> UseState Int <> Hooks.Nil

foreign import data UseX :: Hooks.HookType

instance newtypeUseX :: HookEquals UseX' h => HookNewtype UseX h

If you don’t need the synonym it gets a bit smaller:

foreign import data UseX :: Hooks.HookType

instance newtypeUseX 
  :: HookNewtype UseX (UseEffect <> UseState Int <> Hooks.Nil)

Drawbacks

The new approach does have some drawbacks, mostly in that it introduces a few concepts users wouldn’t otherwise have to learn about.

More unfamiliar concepts

foreign import data ... will be a new concept for a lot of people in a way that newtype alone is not. Luckily this is essentially boilerplate — you don’t need a deep understanding of the topic, you just need to recognize the pattern. Still, it’s yet more unfamiliar stuff to learn.

HookEquals will be new for anyone who doesn’t know about TypeEquals. HookNewtype is hopefully clear to anyone used to Newtype, but it’s less clear than just using Newtype.

More complicated definitions

This is how you’d define a Hook under the indexed monad — you need to know about leaving the hooks variable free and to nest it at the deepest level, but otherwise this is all ordinary PureScript code:

-- with type synonyms
type UseX' hooks = UseState Int (UseEffect hooks)

newtype UseX hooks = UseX (UseX' hooks)

derive instance newtypeUseX :: Newtype (UseX hooks) _

-- without
newtype UseX hooks = UseX (UseState Int (UseEffect hooks))

derive instance newtypeUseX :: Newtype (UseX hooks) _

This is how you’d define the same thing with the free monad + phantom type — now you need to know about the Hooked type / (<>) type operator, and the HookType kind, and the HookEquals and HookNewtype type classes. You need to know (or at least you’ll soon find out) that type synonyms are disallowed in instance heads, which is why HookEquals is needed.

I don’t think any of this is hard, especially because HookEquals should be unnecessary soon, but it’s definitely a few new things to learn about where the newtype approach has none of them.

-- with type synonyms
type UseX' = UseEffect <> UseState Int <> Hooks.Nil

foreign import data UseX :: Hooks.HookType

instance newtypeUseX :: HookEquals UseX' h => HookNewtype UseX h

-- without
foreign import data UseX :: Hooks.HookType

instance newtypeUseX 
  :: HookNewType UseX (UseEffect <> UseState Int <> Nil)

Improvements to look forward to

The main thing I dislike about the new implementation is that users will now have to use the HookEquals class to get around not being able to use type synonyms in instance heads.

However, I’m ultimately still open to making this change because there are upcoming changes to the PureScript compiler that will make this nicer.

First, with PolyKinds in 0.14 the HookEquals class is no longer necessary because TypeEquals will work on the HookType kind:

foreign import data UseX :: HookType

type UseX' = UseState Int <> Nil

-- before
instance newtypeUseX
  :: HookEquals UseX' h => HookNewtype UseX h

-- with polykinds
instance newtypeUseX
  :: TypeEquals UseX' h => HookNewtype UseX h

Even better, if purescript/purescript#3539 gets merged for 0.14 (which I hope happens!) then the type synonym workaround via TypeEquals won’t even be necessary:

-- before
instance newtypeUseX
  :: HookEquals h UseX' => HookNewtype UseX h

-- with type synonms allowed
instance newtypeUseX :: HookNewtype UseX UseX'

Some example type errors

Here are a few comparisons of type errors between the two implementations. I’m happy to add more.

Used too many hooks The HookType version does a better job of reporting that it expected a `UseEffect` hook but instead received 3:
[1/1 TypesDoNotUnify] examples/Example/Hooks/UseInitializer.purs:18:30

                                   v
  18  useInitializer initializer = Hooks.wrap Hooks.do
  19    Hooks.useLifecycleEffect (initializer *> pure Nothing)
  20    Hooks.useLifecycleEffect (initializer *> pure Nothing)
  21    Hooks.useLifecycleEffect (initializer *> pure Nothing)
                                                             ^
  
  Could not match type
  
    UseEffect
  
  with type
  
    Hooked UseEffect (Hooked UseEffect UseEffect)
  
  while solving type class constraint
  
    Halogen.Hooks.Hook.HookNewtype 
      UseInitializer
      (Hooked UseEffect (Hooked UseEffect UseEffect))
  
  while applying a function wrap
    of type HookNewtype t1 t2 => Hook t3 t2 t4 -> Hook t3 t1 t4

  while checking that expression wrap
    has type Hook m0 UseInitializer Unit

  in value declaration useInitializer

As compared to the current indexed monad, which reports it can’t match hooks1 with UseEffect (UseEffect hooks1) first, which is hard to parse, before a better error message that shows all the hooks. I find the newtype part of the error message easier to parse as well.

[1/1 TypesDoNotUnify] examples/Example/Hooks/UseInitializer.purs:19:30

                                   v
  19  useInitializer initializer = Hooks.wrap Hooks.do
  20    Hooks.useLifecycleEffect (initializer *> pure Nothing)
  21    Hooks.useLifecycleEffect (initializer *> pure Nothing)
  22    Hooks.useLifecycleEffect (initializer *> pure Nothing)
                                                             ^
  
  Could not match type
  
    hooks1
  
  with type
  
    UseEffect (UseEffect hooks1)
  
  while trying to match type UseEffect hooks1
    with type UseEffect (UseEffect (UseEffect hooks1))
  while solving type class constraint
  
    Data.Newtype.Newtype
      (UseInitializer hooks1)
      (UseEffect (UseEffect (UseEffect hooks1)))
  
  while checking that expression wrap
    has type Hooked m0 hooks1 (UseInitializer hooks1) Unit
  in value declaration useInitializer
Newtype differences Any error which pertains to the use of `wrap` is better with the new version, because the old version would spit out `Hooked` out of nowhere when users would typically have never seen it before.

The wrap function in the HookType version shows a move from Hook _ a _ -> Hook _ b _ and checks that the resulting type is Hook m UseInitializer Unit.

  while applying a function wrap
    of type HookNewtype t1 t2 => Hook t3 t2 t4 -> Hook t3 t1 t4

  while checking that expression wrap
    has type Hook m0 UseInitializer Unit

  in value declaration useInitializer

The indexed monad version, though, is hard to understand:

 while checking that expression wrap
    has type Hooked m0 hooks1 (UseInitializer hooks1) Unit

  in value declaration useInitializer
Used the wrong hook
[1/1 TypesDoNotUnify] examples/Example/Hooks/UseInitializer.purs:18:30

                                   v
  18  useInitializer initializer = Hooks.wrap Hooks.do
  19    _ <- Hooks.useState 0
  20    pure unit
                ^
  
  Could not match type
  
    UseEffect
  
  with type
  
    Hooked (UseState Int) t5
  
  while solving type class constraint
  
    Halogen.Hooks.Hook.HookNewtype 
      UseInitializer
      (Hooked (UseState Int) t5)
  
  while applying a function wrap
    of type HookNewtype t1 t2 => Hook t3 t2 t4 -> Hook t3 t1 t4

  while checking that expression wrap
    has type Hook m0 UseInitializer Unit

  in value declaration useInitializer

The old version was a bit better, but the wrap function reveals “Hooked” with extra types out of nowhere, which is worse.

[1/1 TypesDoNotUnify] examples/Example/Hooks/UseInitializer.purs:19:30

                                   v
  19  useInitializer initializer = Hooks.wrap Hooks.do
  20    a <- Hooks.useState 0
  21    Hooks.pure unit
                      ^
  
  Could not match type
  
    UseEffect
  
  with type
  
    UseState Int
  
  while trying to match type UseEffect hooks1
    with type UseState Int hooks1
  while solving type class constraint
  
    Data.Newtype.Newtype
      (UseInitializer hooks1)
      (UseState Int hooks1)
  
  while checking that expression wrap
    has type Hooked m0 hooks1 (UseInitializer hooks1) Unit

  in value declaration useInitializer

@thomashoneyman thomashoneyman added the breaking change Change will require a major version bump label May 15, 2020
@thomashoneyman thomashoneyman self-assigned this May 15, 2020
@JordanMartinez
Copy link
Contributor

Does it still prohibit running hooks conditionally?

One question I have about this. Is it possible to subvert the guarantees of this code by using regular do notation rather than the qualified do notation you are supposed to use? For example, let's say I forget to use Hooks.do and instead use do. Does this code still compile?

foo = do -- notice, no Hooks.do here
  state /\ stateId <- useState 0
  useLifecycleEffect do
    liftEffect $ log "foo"
  --

Now, let's say I reorder the hooks so that the effect occurs first, does the code still compile? I imagine the indexed monad approach would not compile in this second version because the Monad instance for an indexed monad would require the "state" type to not change when using normal bind.

data IxMonad x y a = IxMonad a

-- The `state` types here don't change
instance bindIxMonad :: Bind (IxMonad x x) where
  bind :: IxMonad x x a -> (a -> IxMonad x x b) -> IxMonad x x b

I thought preventing this state change was one of the primary reasons for using indexed monads over something else. Otherwise, what stops you from writing if condition then oneHook else aDifferentHook, which would invalidate the guarantee that a given index always refers to the same element in the underlying array? Granted, you could probably fix this by defining your own version of the indexed monad type class hierarchy:

-- `k` would be `HookType`, but I guess this would be easier
class IxAppendMonad :: forall k. (k -> Type -> Type) -> Constraint
class IxAppendMonad m where
  iabind 
    :: forall a b state1 state2
     . m state1 a 
    -> (a -> m state2 b) 
    -> m (state1 <> state2) b

Related...

nothing :: Hook m (UseAPI T) (Maybe T)
nothing = Hooks.pure Nothing

This ought to compile, but it doesn’t under the indexed monad. With the simple free monad approach this is now accepted.

I can't recall the full context of this situation and why that was actually a "bad" thing. If you make it possible to do this, do you also make it possible to run hooks conditionally?

On Naming Things

UseState Int
 <> UseEffect
 <> UseRef Int
 <> Hooks.Nil

I feel like Nil is a bit misleading because of its connotations with List's Nil, which signals the end of a list. Couldn't I do this, too?

Hooks.Nil <> UseState Int <> UseEffect <> Hooks.Nil <> Hooks.Nil

Nil seems to be your way of saying "no hook was used here". In which case, why not use a different word for this, such as NoHook, instead?

foreign import data Hooked :: HookType -> HookType -> HookType
infixr 1 type Hooked as <>

Could this be something more meaningful like AppendHookTypes? <> is already strongly associated with Semigroup's append and yet you'd be calling this type Hooked. Why?

PS 0.14 and its features

First, with PolyKinds in 0.14 the HookEquals class is no longer necessary because TypeEquals will work on the HookType kind
...
Even better, if #3539 gets merged for 0.14 (which I hope happens!) then the type synonym workaround via TypeEquals won’t even be necessary:

Except 0.14 hasn't been released yet, and we don't know when it will be released (e.g. registry issues will slow down people from updating the ecosystem; role inference issues have slowed down the release; other issues may appear over time). I've been waiting for a few months since Nate's PolyKinds PR was merged in, and I can't do any work on my repo without invalidating all the syntax work I did in preparation for this release.
Would it be wiser to make this change after 0.14 gets released and you have the full language features you need to support this well rather than what you're currently doing?

Just clarifying my before-and-after understanding

I don't think your before-and-after examples above provided a full implementation for a real hook. They showed how to define a hook, but not the full example. Here's my current understanding based on what you've written thus far. Is this correct?

-- current approach: hooks appear in reverse
newtype UseX hooks = UseX (UseState Int (UseEffect hooks))

derive instance newtypeUseX :: Newtype (UseX hooks) _

{-
useUseX :: Hooked m hooks (UseX hooks) Return -}
useUseX :: Hook   m       UseX        Return
useUseX = Hooks.wrap Hooks.do
  useLifecycleEffect do
    -- do stuff here
  state /\ stateId <- useState 1
  pure returnValue

-- future approach: hooks appear in their written order
-- but lack of type synonyms in head requires
-- using HookEquals; limitation will be lifted
foreign import data UseX :: Hooks.HookType

instance newtypeUseX 
  :: HookEquals (UseEffect <> UseState Int <> Nil) h => HookNewType UseX h

useUseX :: Hook m UseX Return
useUseX = Hooks.wrap Hooks.do
  useLifecycleEffect do
    -- do stuff here
  state /\ stateId <- useState 1
  pure returnValue

@thomashoneyman thomashoneyman changed the title Move to a single index for hook types Proposal: Move to a single index for hook types May 15, 2020
@thomashoneyman
Copy link
Owner Author

I clarified the title of this PR to note it as a proposal rather than a definite path forward. I won't merge this for a while so the idea can sit and be discussed. To answer your questions:

Does this still prevent running hooks conditionally?

Is it possible to subvert the guarantees of this code by using regular do notation rather than the qualified do notation you are supposed to use? For example, let's say I forget to use Hooks.do and instead use do. Does this code still compile?

Now, let's say I reorder the hooks so that the effect occurs first, does the code still compile?

The code in question was this:

foo = do -- notice, no Hooks.do here
  state /\ stateId <- useState 0
  useLifecycleEffect do
    liftEffect $ log "foo"
  --

The code won't compile either way without Hooks.do. You'll get an error like this one:

Could not match type
  
    UseEffect
  
  with type
  
    UseState Int
  
  while trying to match type Hook t2 UseEffect
    with type Hook t0 (UseState Int)

You still need Hooks.do to write several hooks in a row.

On naming things

I feel like Nil is a bit misleading because of its connotations with List's Nil, which signals the end of a list

That actually is the reason to use Nil; typically pure ends the Hook block, so it's like writing a list with Cons / Nil. It's possible to throw some pures in the middle, but not normal.

Could this be something more meaningful like AppendHookTypes? <> is already strongly associated with Semigroup's append and yet you'd be calling this type Hooked.

That's a good point. It would be better named HookAppend.

PS 0.14

Would it be wiser to make this change after 0.14 gets released and you have the full language features you need to support this well rather than what you're currently doing?

Yes, perhaps it would be best to let this sit until 0.14, and see if it can be cleaned up even more by that time. The HookEquals part isn't terrible, but it'd certainly be nicer without it.

Clarifying before-and-after

I don't think your before-and-after examples above provided a full implementation for a real hook. They showed how to define a hook, but not the full example. Here's my current understanding based on what you've written thus far. Is this correct?

You can also look at the examples folder, which has everything converted to the new style. I'd recommend taking a look there too. As far as your code example, a few notes on the new approach:

-- future approach: hooks appear in their written order but lack of type synonyms in 
-- instance head requires using HookEquals; limitation will be lifted
foreign import data UseX :: Hooks.HookType

instance newtypeUseX 
  :: HookEquals (UseEffect <> UseState Int <> Nil) h => HookNewType UseX h

The code you've written here doesn't use a type synonym so you don't need HookEquals. You could write this instead:

foreign import data UseX :: Hooks.HookType
instance newtypeUseX :: HookNewtype UseX (UseEffect <> UseState Int <> Nil)

However, if you do use a type synonym, then you need HookEquals:

foreign import data UseX :: Hooks.HookType
type UseX' = UseEffect <> UseState Int <> Nil
instance newtypeUseX :: HookEquals UseX' h => HookNewtype UseX h

@natefaubion
Copy link
Collaborator

Note, you don't have to use hook newtypes at all. You can just use synonyms everywhere if you want. Type errors can get crazy though for large blocks.

@thomashoneyman
Copy link
Owner Author

thomashoneyman commented May 15, 2020

Yea, that's true for both approaches.

-- old
type UseX hooks = UseState Int (UseEffect hooks)

useX :: Hook m UseX Unit
useX = ...

-- new
type UseX = UseEffect <> UseState Int <> Nil

useX :: Hook m UseX Unit
useX = ...

@JordanMartinez
Copy link
Contributor

You still need Hooks.do to write several hooks in a row.

Great! That was my only real concern with this.

That actually is the reason to use Nil; typically pure ends the Hook block, so it's like writing a list with Cons / Nil. It's possible to throw some pures in the middle, but not normal.

Ok, but we don't use Cons here either. I get that this has a stack-like feeling to it, which is what List is, but it seems like we're unnecessarily throwing connotations at this by using Nil. If this is always used with pure, I feel like there's a more appropriate name we should use. A few ideas: NoHook or PureHook.

HookAppend

Yeah, that makes more sense.

You can also look at the examples folder, which has everything converted to the new style. I'd recommend taking a look there too.

🤦‍♂️ Wow... I completely forgot about those..

foreign import data UseX :: Hooks.HookType
instance newtypeUseX :: HookNewtype UseX (UseEffect <> UseState Int <> Nil)

Looks like the advantage of this approach over the type one is that it's easier to write. However, when looking at its docs, one will need to be directed to look at its Newtype instance to understand what its corresponding hooks are...

Note, you don't have to use hook newtypes at all. You can just use synonyms everywhere if you want. Type errors can get crazy though for large blocks.

True, but who wants to deal with crazy type errors?

Yes, perhaps it would be best to let this sit until 0.14, and see if it can be cleaned up even more by that time. The HookEquals part isn't terrible, but it'd certainly be nicer without it.

Ok, so I had somewhat of an overreaction caused mainly by my fear that the code would compile using regular do notation. I think this PR's approach is better than what we currently have, and it would be more future-focused. Otherwise, we'll build an entire ecosystem based on the current approach, knowing that we'll break it all in the future anyways.

Also, we likely wouldn't want to use TypeEquals, right? Since it's not an empty dictionary (i.e. it has to and from), it wouldn't get optimized away, correct? HookEquals doesn't have any members, so it should be optimized like that, right?

@thomashoneyman
Copy link
Owner Author

If this is always used with pure, I feel like there's a more appropriate name we should use. A few ideas: NoHook or PureHook.

Or UsePure!

However, when looking at its docs, one will need to be directed to look at its Newtype instance to understand what its corresponding hooks are...

It also doesn't lend itself well to longer hooks -- you'd end up with weird indentation like this:

instance newtypeUseX 
  :: HookNewtype 
      UseX
      (UseEffect 
        <> UseState MyReallyLongType 
        <> UseRef MyReallyLongType
        <> UseRef MyReallyLongType
        <> Nil)

@JordanMartinez
Copy link
Contributor

Or UsePure!

I thought about that, but didn't want to take up the namespace in case there was a hook sometime in the future that wanted that name.

It also doesn't lend itself well to longer hooks -- you'd end up with weird indentation like this:

Ah... yeah. But, even the current approach suffers from this in its own way.

@thomashoneyman
Copy link
Owner Author

I've renamed Hooked to HookAppend and Nil to Pure.

@thomashoneyman
Copy link
Owner Author

As a note: this can't be merged until the README and other documentation have been updated as well.

@thomashoneyman
Copy link
Owner Author

I'm ready to merge this, though I'm still a little uncertain about using Pure to represent a call to Hooks.pure. Otherwise, the documentation has been updated and there's nothing else currently blocking this.

@JordanMartinez
Copy link
Contributor

Other potential names:

  • NoHook
  • NoOpHook
  • Hookless
  • Use_ (as in "use blank/nothing")

@thomashoneyman thomashoneyman changed the base branch from master to main August 5, 2020 19:38
@thomashoneyman thomashoneyman added enhancement New feature or request and removed idea labels Aug 24, 2020
@thomashoneyman
Copy link
Owner Author

@milesfrain @JordanMartinez I'm merging this PR now, but not yet making a release (I still have a few other changes that I'd like to go into the next release, and I need to write a short migration guide). However, I wanted to ping you as an early heads up on halogen-hooks-extra / the Cookbook. Thanks!

@thomashoneyman thomashoneyman merged commit c2c8bfd into main Sep 6, 2020
@thomashoneyman thomashoneyman deleted the no-indexed-monad branch September 6, 2020 21:36
@JordanMartinez
Copy link
Contributor

Thanks for the headsup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Change will require a major version bump enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants