Generalise newAddHandler to any MonadIO m

[ocharles]

This makes it slightly more convenient to call when you're in other
monads (for example, RIO), saving the user a liftIO.

[mitchellwrosen]

LGTM; I tend to like to {-# specialize #-} IO versions of such things but it probably reeeally doesn't matter here :)

[ocharles]

Does that really do anything? The liftIO forces the thing to already be optimized at IO, and the user calls something that uses it, they will be given a MonadIO IO dictionary where liftIO = id.

[mitchellwrosen]

Right; I think it does do something, namely, it generates code that avoids passing that dictionary at runtime.

[HeinrichApfelmus]

Hm. Alas, I'm not entirely sold on the introduction of MonadIO. I understand that it saves a call to liftIO, but at the expense of making the type signature more complicated — the plain data type IO is changed to a more complicated quantified type forall m. MonadIO m => m. I'm in favor of keeping the types simple and only make them more complicated when there is significant gain.

Do you have a specific example where avoiding the liftIO call saves the day?

(I suppose that exception hierarchies à la ExceptT ErrFoo IO benefit the most from
MonadIO, but there are good arguments for not using them in the first place.)

That said, I would consider making the code polymorphic with the help of the io-classes package — in this case, the io-sim package would give us the ability to run the code in a simulated IO environment. Unfortunately, neither package has been released to hackage yet. 🙈

[ocharles]

at the expense of making the type signature more complicated

It's certainly more complicated, but is it more complicated beyond the expected experience for users of this library? I'm not sure!

That said, I would consider making the code polymorphic with the help of the io-classes package

I do find this a little contradictory - we don't want to to MonadIO m => m (), but we are willing to go to a completely new effect system? This seems like a much more significant/unfamiliar change.

Do you have a specific example where avoiding the liftIO call saves the day?

Saves the day is a little strong, I will probably never have such an example as all this change does is allow me to drop a liftIO. But, what I currently have is the following (which is what motivated me to raise these PRs):

data APIEnv = APIEnv
  { getRecentMessages :: MVar (Seq LogMessage) -> IO ()
  , doAOIPocket :: AOIRequest (Text, Int32) -> IO ()
  , -- lots more `a -> IO ()` "fire" functions
  }

data AddHandlers = AddHandlers
  { onUpCameraImageAH :: AddHandler Image
  , onDownCameraImageAH :: AddHandler Image
  , -- lots more `AddHandler`s
  }

main :: IO ()
main = do
  -- not interesting
  ...

  withBatchedHandler defaultBatchingOptions emitJSONMessages \logHandler ->
    flip runLoggingT (liftIO . logHandler) $
    evalContT do
      -- Open both the up and down cameras
      pylon <- ContT withPylon

      onDownCameraImageAH <- openCamera CameraConfig
        { serialNumber = "40123497"
        , imagesDirectory = "/var/lib/pnp/down-camera"
        }

      onUpCameraImageAH <- openCamera CameraConfig
        { serialNumber = "40100047"
        , imagesDirectory = "/var/lib/pnp/up-camera"
        }

      (onSampleStateAH   , getImageDb       ) <- liftIO newAddHandler
      (onSampleMessagesAH, getRecentMessages) <- liftIO newAddHandler

      -- Start part centering
      adsConnection    <- ContT $ withConnection $ adsConnectInfo plcHost
      twinCatVariables <- ContT $ withTwinCatVariables adsConnection

      -- Create AddHandlers for API calls
      (onDoFidFindAH             , doFidFind               ) <- liftIO newAddHandler
      (onLoadProgramAH           , doLoadProgram           ) <- liftIO newAddHandler
      (onAOIReferenceDesignatorAH, doAOIReferenceDesignator) <- liftIO newAddHandler
      (onAOIPointAH              , doAOIPoint              ) <- liftIO newAddHandler
      (onResetAH                 , doReset                 ) <- liftIO newAddHandler
      (onLoadPalletAH            , doLoadPallet            ) <- liftIO newAddHandler
      (onAOIFiducialsAH          , doAOIFiducials          ) <- liftIO newAddHandler
      (onPickAndPlaceAH          , doPickAndPlace          ) <- liftIO newAddHandler
      (onAOIPocketAH             , doAOIPocket             ) <- liftIO newAddHandler

      liftIO $ actuate =<< compile do
        onLogMessages <- mainFRP twinCatVariables AddHandlers{..}
        reactimate $ traverse_ logHandler <$> onLogMessages

      serveAPI APIEnv{ doReset = doReset (), ..}

Note that there's a significant amount of liftIO going on, and I consider it just noise. I'm working in ContT here as I have a ton of bracket-like operations that I want to somewhat hide.

[HeinrichApfelmus]

But, what I currently have is the following

Hm, I see. 🤔 The liftIO in front of the newAddHandler is particularly annoying.

Would you open to a compromise where a new module UnliftIO.Event.Handler exports the MonadIO m => m versions and Control.Event.Handler exports the old newAddHandler? In this way, it would be more clear how and where to get the "unliftio-style" of functions.

we don't want to to MonadIO m => m (), but we are willing to go to a completely new effect system? This seems like a much more significant/unfamiliar change.

My thinking was that if we go into this direction at all, then we might as well go all the way. I tend to think of io-classes not as an effect system, but more of an extension of the mtl style classes. For example, there is a class

class … => MonadFork m where
    forkIO :: m () -> m (ThreadId m)
    …

The beauty (and purpose) of this approach is that the monad does not need to be a stack of monad transformers whose ground type is IO — instead, the ground type can be Identity or any pure data type. This would give us the ability to run/simulate the entire network in a pure manner, completely without IO. In contrast, the MonadIO constraint does require that there is an actual IO somewhere.

---

By the way, your records that contain AddHandler and Handler appears to be susceptible to the trick where one parametrizes a type by a functor. Like this:

import GHC.Generics ((:*:) (..))

data Handlers f = Handlers
  { upCameraImage :: f Image
  , downCameraImage :: f Image
  , …
  }

type APIEnv = Handlers Handler
type AddHandlers = Handlers AddHandler


hoist :: (forall a. f a -> g a) -> Handlers f -> Handlers g
hoist f (Handlers a b c d) = Handlers (f a) (f b) (f c) (f d)

unzipHandlers :: Handlers (f :*: g) -> (Handlers f, Handlers g)
unzipHandlers h = (hoist first h, hoist second h)
  where
    first  (x :*: _) = x
    second (_ :*: y) = y

newAddHandler' :: MonadIO m => m ((AddHandler :*: Handler) a)
newAddHandler' = liftIO $ fmap to newAddHandler
  where to (x,y) = x :*: y


main :: IO ()
main = do
  …
  
  (onHandlersAH, doHandlers) <- fmap unzipHandlers $ Handlers
      <$> newAddHandler'
      <*> newAddHandler'
      <*> newAddHandler'
      <*> newAddHandler'

In this way, you would not have to write down two distinct names for each pair of AddHandler and Handler.

---

I'm working in ContT here as I have a ton of bracket-like operations that I want to somewhat hide.

Ooh! That's a very neat trick. I'm looking at a similar problem right now (long chains of withResource operations), I will need to investigate this one.

[ocharles]

Would you open to a compromise where a new module UnliftIO.Event.Handler exports the MonadIO m => m versions and Control.Event.Handler exports the old newAddHandler? In this way, it would be more clear how and where to get the "unliftio-style" of functions.

I'm not really convinced. My proposed change to newAddHandler subsumes the original IO type, so as far as I know is backwards compatible and keeps the surface area of the API constant. A new module now duplicates this part of the API surface area, and forces users to make a decision - "which of these do I want?". This increases the burden on us, as we need to provide rationale for each module to allow users to make this decision.

Minor, but it also increases the dependency graph if you really want MonadUnliftIO (though it doesn't seem necessary).

My thinking was that if we go into this direction at all, then we might as well go all the way.

It feels like this would be quite a lot of work, and moves reactive-banana into a space that's relatively unoccupied (well, entirely unoccupied as io-classes doesn't even exist on Hackage yet!). Furthermore, the effect system space is constantly changing (or, to put it a bit more bluntly, churning)

This would give us the ability to run/simulate the entire network in a pure manner, completely without IO.

Personally, I think we can all get a bit too caught up in this goal - I know I have. In reality, being in IO is not the end of the world - it just sometimes requires a little more thinking to get good tests. I used to abstract-out my use of a SQL database, for example, but eventually accepted that's fundamental to my application and worked out how to get fast real database tests. For reactive-banana I think there are two things we might want:

1. A way to test the core API. Here being in IO doesn't matter, because it's our library and we get to choose what the IO is.
2. A way to assert properties about the core API. Here something like dejafumight be useful, but that's just speculation.

By the way, your records that contain AddHandler and Handler appears to be susceptible to the trick where one parametrizes a type by a functor.

Yep, I'm aware of this pattern, but generally like to write the simplest Haskell I can. In this case, I couldn't quite justify this pattern. (Somewhere I think it is justified is in my rel8 library, though 😉)

---

We're going a bit off topic now though. I think we should wrap up and make a decision. I think there are ultimately two choices:

1. Go with MonadIO as proposed here.
2. Do nothing, keep things as they are.

I'm just not sure how to make the choice! Personally I much prefer MonadIO everywhere - we did it in sdl2 (https://hackage.haskell.org/package/sdl2-2.5.3.0/docs/SDL-Video-Renderer.html#g:4) and dear-imgui (https://hackage.haskell.org/package/dear-imgui-1.2.2/docs/DearImGui.html), and the gl library also takes the same stance (https://hackage.haskell.org/package/gl-0.9/docs/Graphics-GL-Core45.html).

I can't find much motivation for 2 - it just makes things slightly more annoying. If you're in IO, 1 & 2 are equivalent.

[mitchellwrosen]

I think (1) has small cost and small benefit.

One thing I dislike is how there's not much that is generalizable to MonadIO this way. reactive-banana has many IO actions in negative position, and I think a companion library like reactive-banana-unlifted is a pretty good solution to that problem, which is where the generalized newAddHandler could go, too, even if it could be generalized here to MonadIO m with no additional dependencies.

[HeinrichApfelmus]

I think we should wrap up and make a decision. I think there are ultimately two choices:

1. Go with MonadIO as proposed here.
2. Do nothing, keep things as they are.

Just for reference: The types (forall m. MonadIO m => m a) (choice 1) and IO a (choice 2) are isomorphic. The advantage of the polymorphic version would be that it avoids an extra application of the liftIO function, though the price is that type ambiguities may arise in some contexts (aka the read . show problem).

A new module now duplicates this part of the API surface area, and forces users to make a decision - "which of these do I want?". This increases the burden on us, as we need to provide rationale for each module to allow users to make this decision.

My reason for bringing up the unliftio package is that it's in the business of applying the mentioned isomorphism to many functions in the Haskell ecosystem. For example, the UnliftIO.directory module is a vanilla copy of the System.Directory module, except that all IO a types have been replaced with their isomorphic forall m. MonadIO m => m a counterparts.

My thinking was that I have a slight preference for the monomorphic version (choice 2), but that we could have the best of both worlds by also offering choice 1 in the "de facto canonical" namespace for those who want it.

---

This would give us the ability to run/simulate the entire network in a pure manner, completely without IO.
Personally, I think we can all get a bit too caught up in this goal - I know I have.

Fair enough. I ditch the io-classes idea.