agrafix / Spock

Another Haskell web framework for rapid development
https://www.spock.li
678 stars 56 forks source link

Add instance of MonadTransControl, MonadBase, and MonadBaseControl for ActionCtxT #117

Closed cdepillabout closed 7 years ago

cdepillabout commented 7 years ago

This PR adds an instance of MonadTransControl, MonadBase, and MonadBaseControl for ActionCtxT. This closes #116.

Commit 0aa24d4 adds an instance of MonadTransControl for ActionCtxT. Aside from some hairy wrapping and unwrapping, it is pretty straight-forward.

Commit 9d83386 adds an instance of MonadBase and MonadBaseControl for ActionCtxT. The main problem with this is that it requires UndecidableInstances.

Here are three different courses of action I feel like we could take:

  1. Just merge in this PR. monad-control and transformers-base both use UndeciableInstances when defining their instances of MonadBase and MonadBaseControl for the standard transformers like ExceptT, ReaderT, etc. If it's good enough for them, it's good enough for us.

    Realistically, it's unlikely that a user would want to define an alternative instance of MonadBase or MonadBaseControl for ActionCtxT.

  2. Right now, the MonadBase and MonadBaseControl instances look like this:

    instance MonadBase b m => MonadBase b (ActionCtxT ctx m) where
        liftBase = liftBaseDefault
    
    instance MonadBaseControl b m => MonadBaseControl b (ActionCtxT ctx m) where
        type StM (ActionCtxT ctx m) a = ComposeSt (ActionCtxT ctx) m a
        liftBaseWith = defaultLiftBaseWith
        restoreM = defaultRestoreM

    Trying to compile this without UndecidableInstances produces the following warning:

    /home/illabout/git/spock/Spock-core/src/Web/Spock/Internal/Wire.hs:275:10: error:
        • Illegal instance declaration for ‘MonadBase b (ActionCtxT ctx m)’
            The coverage condition fails in class ‘MonadBase’
              for functional dependency: ‘m -> b’
            Reason: lhs type ‘ActionCtxT ctx m’ does not determine rhs type ‘b’
            Un-determined variable: b
            Using UndecidableInstances might help
        • In the instance declaration for ‘MonadBase b (ActionCtxT ctx m)’
    
    /home/illabout/git/spock/Spock-core/src/Web/Spock/Internal/Wire.hs:278:10: error:
        • Illegal instance declaration for
            ‘MonadBaseControl b (ActionCtxT ctx m)’
            The coverage condition fails in class ‘MonadBaseControl’
              for functional dependency: ‘m -> b’
            Reason: lhs type ‘ActionCtxT ctx m’ does not determine rhs type ‘b’
            Un-determined variable: b
            Using UndecidableInstances might help
        • In the instance declaration for
            ‘MonadBaseControl b (ActionCtxT ctx m)’
    
    /home/illabout/git/spock/Spock-core/src/Web/Spock/Internal/Wire.hs:279:10: error:
        • Illegal nested type family application ‘StM
                                                    m (StT (ActionCtxT ctx) a)’
          (Use UndecidableInstances to permit this)
        • In the type instance declaration for ‘StM’
          In the instance declaration for
            ‘MonadBaseControl b (ActionCtxT ctx m)’

    If we heed the warning, we could define specific instances of MonadBase and MonadBaseControl for IO. It would look something like this:

    instance MonadBase IO m => MonadBase IO (ActionCtxT ctx m) where
        liftBase = liftBaseDefault
    
    instance MonadBaseControl IO m => MonadBaseControl IO (ActionCtxT ctx m) where
        type StM (ActionCtxT ctx m) a = ...
        liftBaseWith = ...
        restoreM = ...

    This means there won't be an instance of MonadBase and MonadBaseControl for users running ActionCtxT in some other base monad (like ST or Maybe). But realistically, there probably aren't many users that actually do that. And even if there are, it's unlikely that they also want to use liftBase, liftBaseWith, and restoreM.

  3. Don't provide MonadBase or MonadBaseControl instances, but export the constructor for ActionCtxT. That would let users write their own MonadBase and MonadBaseControl instances.

    We could still provide the MonadTransControl instance from commit 0aa24d4, since it doesn't require UndecidableInstances.

@agrafix Let me know what you think about these three options.

cdepillabout commented 7 years ago

Hmm, after playing around with it a little bit, I couldn't figure out a way of defining the instance MonadBaseControl IO (ActionCtx ctx m) that doesn't require UndefinedInstances. Looks like (2) isn't really a viable option. We should probably go with either (1) or (3).

agrafix commented 7 years ago

I'd say UndecidableInstances should be okay here. I think a great thing would be to write a small unit test to make sure that the instances are usable and work as advised. Then we can merge :-)

cdepillabout commented 7 years ago

Okay great, I'll add some tests.

BTW, I was wondering about UndecidableInstances, so I submitted a question to the Haskell reddit. Edward Kmett gave a nice long reply explaining when UndecidableInstances is safe and when it is dangerous.

tl;dr It's basically safe in the case of MonadBase and MonadBaseControl.

cdepillabout commented 7 years ago

Tests have been added in commit 5e38734.

The tests are somewhat contrived, but they do test the functionality provided by MonadBase, MonadBaseControl, and MonadTransControl.

agrafix commented 7 years ago

Great, thanks! :-) Will merge when green 👍