Request for comment: EventM state monad refactor

[jtdaugherty]

This ticket is a place for discussing the impact of work that is happening on the refactor/event-state-monad branch. The objective of this branch is essentially to refactor EventM to make event handler code more modular, easier to write, and less verbose. This branch introduces several breaking API changes.

If you write/maintain brick applications, I'd love your thoughts on how this will impact your applications (positively and negatively) if this were to be merged and released. I'd also love feedback if you actually try using the branch as well. Thanks!

Examples of the impact of these changes in action can be found in the demo programs. Here are a few examples:

• MouseDemo: before, after
• ListDemo: before, after
• EditDemo: before, after

Here's a summary of the changes and their impacts:

• The EventM type is changing from EventM n a to EventM n s a. EventM n s a is now a MonadState over s. (The state monad introduced in this change is strict in s.) This means that you can either use lenses or the mtl state monad API to manage your state. Therefore, event handler functions that previously took an explicit state argument and returned a Next are now state-monadic over that state type instead. For example, a function that was previously written as handleMyEvent :: s -> BrickEvent n e -> EventM n (Next s) will now be written handleMyEvent :: BrickEvent n e -> EventM n s ().
• The Next type is going away from the user-facing API. This means EventM blocks in event handlers no longer need to end with continue, halt, etc. Instead, the behavior previously provided by the continue function is now the default behavior for any EventM block, and halt etc. need to be called explicitly to indicate what the library's main event loop should do once the event handler terminates. Functions like halt do not change control flow of EventM (i.e. do not cause early aborting).
• continue itself was removed from the API. A practical consequence of this is that if an EventM block now calls halt, it cannot undo that by subsequently calling continue since continue no longer exists.
• handleEventLensed went away and has been replaced with support for the zoom function from microlens-mtl. This works with both lenses and traversals. Brick.Types now re-exports zoom for convenience.
• The library's event handler functions (e.g. for List, Edit, etc.) are now scoped to just the states they manage; e.g. handleEditorEvent :: BrickEvent n e -> EventM n (Editor t n) (). This means they are now used with zoom, i.e. zoom someLens $ handleEditorEvent e. This also impacted the Form type whose field event handlers correspondingly changed to be scoped to their fields' state types.
• Brick now depends on mtl rather than transformers.

To me, the biggest practical consequences of these changes are that:

• Positive: EventM code will be much cleaner, particularly if you like to use lenses to manage your state values. Event handlers can now use lens updates with the microlens-mtl API, e.g., someLens .= someValue etc.
• Positive: Nested event handlers can now change state types, leading to more focused and more modular event handler implementations.
• Positive: event handlers will no longer have continue noise since that is the default behavior of any EventM block unless otherwise specified by halt etc.
• Positive: brick application authors who want to use lenses more often in their event handlers can now do so.
• Positive: even brick application authors who don't want to use lenses more often can still benefit from the move to make EventM a state monad over the application state type.
• Negative: Application authors will need to update their event handlers to use the new API. Depending on the application, this may be quite a bit of work. If this gets released, I'll provide upgrade guidance documentation.
• Negative: Applications that previously used their own event handler wrapper monads around EventM will have to contend with these changes as well. (I suspect this will be rare.)

[simonmichael]

I'm all for easier APIs, hledger would adapt. Thank you !

[jtdaugherty]

Okay, thanks, @simonmichael - that's great to hear.

[jtdaugherty]

@simonmichael I guess I'm wondering whether you think the specific changes described above would make things easier for hledger.

[simonmichael]

I'll know more after porting to the new API but I expect it would be more of a help when first coding something - not now, when we are already coded for the old API.

[frasertweedale]

I will attempt to find time to update purebred this week, and provide feedback about the change.

[jtdaugherty]

@frasertweedale Okay, great!

[kquick]

It sounds reasonable to me. I'll try to refactor my application to the feature branch sometime this week and will be able to give more specific feedback then.

[jtdaugherty]

Thanks, @kquick!

[jtdaugherty]

FYI: I also keep thinking of ways to update and clarify the summary comment at the top of this ticket, too.

[byorgey]

Yeah, although refactoring to use the new API would be a bit of work, I'm pretty convinced (as far as I can be without actually trying it) that this would improve the code in swarm a lot. In particular, not having to put continue everywhere, and being able to use state monad + lens stuff to manage our application state will be great.

[jtdaugherty]

@byorgey okay, great.

Another thing I wanted to mention that I will add to the summary comment, but that I wanted to make very explicit here: EventM is a strict state monad. I can't think of a reason why it would be important for it to be lazy, but I want to point this out in case anyone is planning on relying on laziness in some fancy way.

[thomasjm]

Thanks for reaching out! I guess my main concern is how to migrate in such a way that I can support the 2 major versions of Brick as easily as possible, since my project will need to keep supporting older code for a while. I guess I can more or less use CPP to define continue = put for the new Brick API, plus update a few type signatures?

[jtdaugherty]

@thomasjm I'm afraid that will probably not be practical. Migrating to this version of brick will likely require a lot of small changes, which in your example means a lot of annoying CPP noise. Migrating to this version of brick will be a one-way process; since it introduces breaking changes, it will likely be very difficult if not impossible to support older versions of brick in your build. That's something to be aware of.

[thomasjm]

Okay understood. I'll try to find some time to try out the new branch. In general any shims to ease the possibility of making a CPP monstrosity would be appreciated, since I try to support the last several Stackage LTSes and don't really want to maintain multiple branches. Aside from migration issues the new API sounds very nice!

[simonmichael]

I'll be one trying to support both APIs, since I need to support a range of ghc versions and all stackage snapshots.

[jtdaugherty]

For folks wanting to support older versions of brick, I'm not sure there is much I can say other than that you'd just use whatever release came before this one (if it goes out). Supporting multiple versions of a library that introduces breaking changes would be no small task in general, and in this case the changes introduced could have a significant impact on your application. If this gets merged and released, it would be the supported (by me) version of brick going forward since I don't have the resources to maintain two versions of the library (i.e. backport bugfixes). I would strongly recommend that you commit to upgrading, and particularly for application builds I would question the wisdom in supporting arbitrary builds of older versions of library dependencies anyway. I just want to be really explicit about of this since I wouldn't recommend supporting the current version of brick at all once this one is released. In short, this kind of issue is one of the reasons I wanted to have a discussion on the impact of this upgrade.

I also want to mention that I do not explicitly participate in Stackage as a thing in the Haskell ecosystem, so any compatibility of brick or vty with Stackage or its releases is incidental. I do respond to requests to update bounds to keep Stackage builds working, but that's the extent of my involvement in Stackage-related matters. So far that seems to have worked reasonably well, but I wanted to be explicit about this, too.

[Cj-bc]

I've tried new API with few of my own apps (Cj-bc/brick-3d, Cj-bc/brick-shgif), and it was easy to migrate!
Also, I didn't find anything negative by this modification.

As I don't know much about API design, I'm not sure about pros and cons. But I felt confortable to write EventM with MonadState functions.

I've struggled a bit to unedrstand how event handler works when I first learnt brick because of its (a little bit) complecated type signature, so I think it's good idea to use well-known 'MonadState' to construct it.

So I agree with this update!

[jtdaugherty]

@Cj-bc that's very cool! And I'm glad to hear that the update was straightforward in your case.

[sullyj3]

I find that my state ends up looking something like this:

data MainScreenState
data SecondScreenState
data ThirdScreenState

data ScreenState = MSS MainScreenState | SSS SecondScreenState | TSS ThirdScreenState

data AppState = AppState { _field1 :: CommonState1, _field2 :: CommonState2, _screenState :: ScreenState }

handleEvent :: AppState -> BrickEvent n e -> EventM n (Next AppState)
handleEvent (AppState field1 field2 screenState) ev = case screenState of
  MSS mss -> handleEventMainScreen mss field1 field2 ev
  SSS sss -> handleEventSecondScreen sss ev
  TSS tss -> handleEventThirdScreen tss field2 ev

handleEventMainScreen
  :: MainScreenState -> CommonState1 -> CommonState2 -> BrickEvent n e -> EventM n (Next AppState)

handleEventSecondScreen
  :: SecondScreenState -> BrickEvent n e -> EventM n (Next AppState)

handleEventThirdScreen
  :: ThirdScreenState -> CommonState2 -> BrickEvent n e -> EventM n (Next AppState)

Note that each of the screen specific event handlers take in the state specific to their screen, so that they don't have to do redundant partial pattern matches on ScreenState in each of their bodies. However, they need to return a full AppState, because they need to be able to switch screens, or modify state common to the whole program. This means they don't match the form of a state monad, s -> (a, s). Do you have any suggestions on how a program like this with multiple screens could be modularized using a state monad approach?

[jtdaugherty]

If it were me, I'd just rewrite everything to be in terms of AppState as follows, and trust that the implementation of the screen-specific event handlers mutated only the bits of AppState that they needed to mutate.

One challenge with your current data types that is exacerbated by the move to a state monad is that you can't easily use lenses over the different constructors of ScreenState. If it were me, I'd write three unsafe lenses (one per ScreenState constructor) to use in the screen-specific event handlers. (They'd be unsafe in that they'd raise an exception if used on the wrong constructor, but that would be a bug anyway.)

data MainScreenState
data SecondScreenState
data ThirdScreenState

data ScreenState = MSS MainScreenState | SSS SecondScreenState | TSS ThirdScreenState

data AppState = AppState { _field1 :: CommonState1, _field2 :: CommonState2, _screenState :: ScreenState }

handleEvent :: BrickEvent n e -> EventM n AppState ()
handleEvent ev = do
  s <- use screenState
  case s of
    MSS _ -> handleEventMainScreen ev
    SSS _ -> handleEventSecondScreen ev
    TSS _ -> handleEventThirdScreen ev

handleEventMainScreen :: BrickEvent n e -> EventM n AppState ()

handleEventSecondScreen :: BrickEvent n e -> EventM n AppState ()

handleEventThirdScreen :: BrickEvent n e -> EventM n AppState ()

[jtdaugherty]

In case you haven't used something like microlens-mtl, it lets you then write things like

handleEventMainScreen :: BrickEvent n e -> EventM n AppState ()
handleEventMainScreen ev = do
    field1 .= someNewCommonState -- Direct assignment
    screenState.unsafeMSS %= func -- Modification by a pure function, func
    screenState.unsafeMSS .= newMSSValue -- Direct assignment

(This assumes the existence of a lens like unsafeMSS. If it were me, I wouldn't call it that, but you get the idea.)

[sullyj3]

I've used and enjoy microlens-mtl, it's very ergonomic. Yeah, I think that structure makes sense. It feels a bit icky to be writing partial lenses, but it might be the least bad option. Could you give an example or point to a resource on how to write a partial lens like that? I've only ever used TH or generics to generate them.

[frasertweedale]

@jtdaugherty from the purebred project, we have a few problems to solve before we can properly analyse this proposed brick change. There are already several other users providing feedback. So if you are ready to proceed before we are able to give feedback, our default position is "no objection".

[jtdaugherty]

@sullyj3 I don't know of a better way than to use lens. Here's an example for the MSS constructor:

mss :: Lens' ScreenState MainScreenState
mss =
      lens (\ss -> case ss of
                 MSS mss -> mss
                 _ -> error "BUG: mss: tried to get MainScreenState from incorrect constructor")
           (\ss mss -> case ss of
              MSS _ -> MSS mss
              _ -> error "BUG: mss: tried to write MainScreenState to incorrect constructor field")

[jtdaugherty]

@frasertweedale no hurry, I'm not going to merge this for a while and not until the feedback suggests the change could be weathered.

[xsebek]

Hi, does anyone here know how to run an action on the inner state? There is bound to be a lens for that, but I do not see it.

Original code:

s' <- s & uiState . uiModal . _Just . modalDialog %%~ handleDialogEvent ev

Work in progress:

-- Something like this would be nice:
-- uiState . uiModal . _Just . modalDialog %%= handleDialogEvent ev
modal <- use $ uiState . uiModal . _Just . modalDialog
d <- fst <$> nestEventM modal (handleDialogEvent ev)
uiState . uiModal . _Just . modalDialog .= d

But in most cases, the change is easy and more readable, I should have swarm updated over the weekend. 👍

[jtdaugherty]

s' <- s & uiState . uiModal . _Just . modalDialog %%~ handleDialogEvent ev

becomes

withLens (uiState.uiModal.singular _Just.modalDialog) (handleDialogEvent ev)

[jtdaugherty]

In talking with another developer about the impact of these changes, the question arose: what is the performance overhead of withLens, and in particular, multiple levels of nested use of withLens? Here's the implementation:

https://github.com/jtdaugherty/brick/blob/refactor/event-state-monad/src/Brick/Types.hs#L115

I haven't done any performance measurement on the new branch and frankly I'm not sure how to do so. If anyone has thoughts on this, I'd love to know!

[jtdaugherty]

In particular, what I'll need to investigate is whether the fact that EventM is strict in its state, combined with the overhead of the machinery in withLens (nestEventM), results in a measurable latency increase in event handler cost.

[xsebek]

@jtdaugherty just to bikeshed a bit on the name withLens, lens libraries already use that name to mean the reversal of lens.

Maybe this could be named mutateWithLens or something shorter like mutateVL? The lens library uses all the good names. 😅

EDIT: or maybe it should be a Zoom instance?

Also, so far I have not used the second component of nestEventM result and I wrote a helper function:

nestEventM' :: a -> EventM n a b -> EventM n s a
nestEventM' a em = fst <$> nestEventM a em

[xsebek]

But thanks a lot @jtdaugherty for that withLens example, it worked in almost all cases!

The exception is when we were inside Maybe and I think I need Traversal':

mutateFirst :: Traversal' s a -> EventM n a () -> EventM n s ()
mutateFirst t em = do
  ma <- preuse t
  case ma of
    Nothing -> return ()
    Just a -> do
      newA <- nestEventM' a em
      t .= newA

For use case like this:

- s' <- s & uiState . uiModal . _Just . modalDialog %%~ handleDialogEvent ev
+ mutateFirst (uiState . uiModal . _Just . modalDialog) (handleDialogEvent ev)

Maybe there is a more generic implementation (and better name) for that function, but hopefully it will be useful to other people porting magic lenses to the new API. 🙂

[jtdaugherty]

EDIT: or maybe it should be a Zoom instance?

@xsebek Yes, this would be great if it's possible. I looked into it when I did the refactor and I got the impression that it wasn't, but I'd be happy to be wrong about that! But if a Zoom instance can't be written then I'd like to try to stick with withLens unless we can come up with another name that is 1) concise/short, 2) naturally named, and 3) readable in context.

Also, so far I have not used the second component of nestEventM result and I wrote a helper function:

Okay, it's helpful to know that you needed a different version of nestEventM.

The exception is when we were inside Maybe and I think I need Traversal':

This is really interesting to me because while I can see the value in working with a Traversal', having such a variant means you can write event handlers that silently do nothing if you mistakenly invoke them when your traversal yields no elements. That could be deliberate (and I think that's reason enough to provide it) but I'd want to put a big warning in the documentation for such a function saying "You probably want withLens unless you are really sure you want to silently do nothing if your traversal fails to find anything."

[jtdaugherty]

@xsebek The branch now provides nestEventM' and withFirst (a different name for your mutateFirst function).

[xsebek]

Thanks a lot @jtdaugherty! 👍 I investigated the Zoom instance in the refactor/zoom-event branch, but got stuck.

On a moral level, I do not think the instance is impossible. But given that the current functions should take care of 99% of use cases, it is not a big deal. 🙂

[jtdaugherty]

Thanks, @xsebek. I do want to look into it more, and thanks for your work on it. I think zoom would be a more natural API than withLens, so I feel motivated to see if there's a way we can make that work.

[jtdaugherty]

In other news, in an attempt to clean up nestEventM I realized there was a much better way to refactor the internals that also led to the improvement of suspendAndResume. I made that change on this branch as well; it is e0c70f0.

[jtdaugherty]

Great news, @xsebek - I just got zoom working and pushed a patch to the branch. The change removes both withLens and withFirst; zoom should work for the cases where either of those was needed. Also paging @byorgey @kquick

[jtdaugherty]

I also just updated the ticket description to incorporate the zoom-related changes.

[byorgey]

BTW, we have now finished updating the swarm codebase to work on top of this branch, and the code is so much nicer. (See swarm-game/swarm#583 for the changes). We definitely don't want to go back. 😄

[jtdaugherty]

Thanks, @byorgey - I'm glad to hear that the changes led to improvements!

[jtdaugherty]

Reading over the comments again, I am now planning on merging this to master and getting it ready for release. Quite a bit of work still needs to be done so the release won't happen for a week or two, but when it gets released, it will be released as Brick 1.0 and will also include some other breaking API changes and improvements. The release will include an updated User Guide as well as migration guidance specific to 0.7x -> 1.0 updates.

Thank you so much to everyone here for sharing your thoughts and trying out the branch!

[jtdaugherty]

Hi everyone, Brick 1.0 is out and includes the changes discussed here! https://github.com/jtdaugherty/brick/blob/master/CHANGELOG.md#10

[simonmichael]

hledger 1.27 will use (and require) brick 1.0. The new code seems more straightforward and understandable - thanks @jtdaugherty !

The migration notes were very helpful. The one thing I didn't entirely figure out was how to compile with MonadFail across all GHC versions (so I just avoided it. For the record: it came to Prelude in ghc 8.8/base 4.13.. it's leaving Prelude in ghc 9.4/base 4.17.. it came to brick's EventM in 0.52.. [and was removed in 1.0, but only when building with base >= 4.13].. it shows in the hackage haddock for brick 0.73, but not for brick 1.0.)

brick 1.0 seems to build well with old stackage snapshots/ghc versions, I tested back to lts-14.27/ghc-8.6 which required these extra deps:

- brick-1.0
- bimap-0.5.0
- text-zipper-0.12
- vty-5.36
- ansi-terminal-0.10.3

[jtdaugherty]

Ah, sorry about the MonadFail thing. I forgot to mention that removal in the changelog. I ran into some irritation with MonadFail and decided it wasn't worth trying to deal with it since the MonadFail instance was not really being used for anything important.

[simonmichael]

Ah, so it was removed. I think I can see that now.. except it seems to still be there when building with base < 4.13 / ghc < 8.8 ? Is that intended ?

1.0 doc: https://hackage.haskell.org/package/brick-1.0/docs/Brick-Types.html#t:EventM
1.0 source: https://hackage.haskell.org/package/brick-1.0/docs/src/Brick.Types.EventM.html#EventM

[jtdaugherty]

Thanks, I'm getting mixed up. I remember now that I made the derivation conditional. I forget why but on newer GHCs there was some issue with the derivation, so I just bailed and made it conditional. I'd be okay with reinstating it once I can look into it more (or someone knows what the proper fix would be).

[simonmichael]

FWIW in hledger I also used only nestEventM', not nestEventM.

[ecthiender]

Hi I am running into a problem with this migration and thought I'd ask here. Sorry if it's a dumb question.

I have the following pattern for my event handlers -

 let newList = insertNewItem (t,m) _asMessages -- _asMessages is part of my AppState
 in continue $ state { _asMessages = newList }

For simple things like this I don't like to use lens. IMO, lens makes is more complicated to read, adds a layer of indirection and abstraction, and also takes up compile time. I would rather stick to simple record updates, atleast for simple things.

Now in the migration guide it is written I should replace continue $ s & someLens .~ value with someLens .= value. What if I don't have the someLens? What if I have my updated state and just want to return it? Is there any way? Is there any way where I can keep my simple record updates and return back the updated state?

[ecthiender]

Ah okay, I figured something out. Not sure if this preferable or "officially recommended", but I'll note it down anyway if anyone else is wondering like me.

I did not know .= operates in MonadState. It has the type -

MonadState s m => ASetter s s a b -> b -> m ()

So that seems like it modifies the state. So I tried with the following and it seems to work

let newList = insertNewItem (t,m) _asMessages
in modify $ const $ state { _asMessages = newList }

Admittedly, this is not nicer than just writing asMessages .= newList, but I prefer to write non-lens code as much as I can. So from that pov, I'd prefer the continue primitive.

[jtdaugherty]

Hi @ecthiender - yes, some version of what you wrote is going to be required. If you want to avoid using lenses, then you'll need to do something like what you wrote, or abstract out the modification process a bit (which you'd have needed to do anyway, I think).

If it were me, I'd probably write

modifyMessages :: (Messages -> Messages) -> State -> State
modifyMessages f s = s { _asMessages = f (_asMessages s) }

and then in the event handler,

modify $ modifyMessages (insertNewItem (t, m))

That avoids the need for const (which could actually avoid bugs since then it's unclear which state is being modified, and some modifications could accidentally get lost).

Incidentally, modifyMessages is essentially a manually-written lens; after writing a few more of those it might become apparent why lenses are going to save a lot of time in the long run.

[jtdaugherty]

@ecthiender - I should also mention that the pattern you're encountering has an even nicer lens representation with %= if asMessages is a lens:

asMessages %= insertNewItem (t, m)

[sullyj3]

@ecthiender You can also use put instead of modify with const

let newList = insertNewItem (t,m) _asMessages
in put $ state { _asMessages = newList }

But I also prefer %=