Skip to content
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

Remove HFunctor in preparation for improvements to Effect #295

Merged
merged 45 commits into from
Oct 25, 2019
Merged

Conversation

robrix
Copy link
Collaborator

@robrix robrix commented Oct 24, 2019

build-depends:
base >= 4.9 && < 4.14
, deepseq ^>= 1.4.3
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meant to 🔥 this when we eliminated Resumable, but better now than after 1.0.

@@ -3,3 +3,4 @@
dist-newstyle
stack.yaml
cabal.project.local
.ghci
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve started using cabal exec ghci more and so I’m using this to set stuff up for that, but I don’t think we want to commit it since others will likely be using cabal repl and it might break or confuse things?

@@ -179,7 +179,7 @@ instance
unReinterpretLogC k

R other ->
ReinterpretLogC (alg (R (handleCoercible other)))
ReinterpretLogC (handleCoercible other)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where I’ve come down re: #235: this is all boilerplate, so let’s at least make it minimal, obvious, & consistent boilerplate.

Comment on lines +91 to +98
-- | Thread a 'Coercible' carrier through an 'Effect'.
--
-- This is applicable whenever @m@ is 'Coercible' to @n@, e.g. simple @newtype@s.
--
-- @since 1.0.0.0
handleCoercible :: (Monad m, Effect eff, Member eff sig, Algebra sig n, Coercible m n) => eff m a -> n a
handleCoercible = handleIdentity coerce
{-# INLINE handleCoercible #-}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new bit here is that handleCoercible calls (via handleIdentity) send, and thus no longer requires you to count Rs on the left.

Comment on lines -54 to +58
instance (Algebra sig m, Effect sig) => Algebra (Error e :+: sig) (ErrorC e m) where
alg = ErrorC . alg . handleCoercible
instance Algebra sig m => Algebra (Error e :+: sig) (ErrorC e m) where
-- NB: 'send' (& thus 'handleCoercible') can’t send sums, so we decompose the sum manually.
alg (L (L op)) = ErrorC (handleCoercible op)
alg (L (R op)) = ErrorC (handleCoercible op)
alg (R op) = ErrorC (handleCoercible op)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the one weird case: because handleCoercible uses send, and because send uses Member, and because Member can’t resolve sums of effects on the left, we have to decompose the sum here instead of doing just alg = ErrorC . handleCoercible.

I wouldn’t expect end users to have to do this very often, however, since it’s not very often you want to delegate all of the effects you handle to something else (how I wish we could simply GND-derive the instance!), or that (as in this case), you’re handling multiple effects combined in a sum, let alone both.

=> (forall x . eff m x -> m x)
-> (forall s . Reifies s (Handler eff m) => InterpretC s eff m a)
-> m a
runInterpret f m = reify (Handler (InterpretC . f . handleCoercible)) (go m) where
runInterpret f m = reify (Handler (InterpretC . fmap runIdentity . f . handle (Identity ()) (fmap Identity . coerce))) (go m) where
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the other casualty of handleCoercible: we can’t use it here any more. I’m ok with that, again because this is something most users are unlikely to have to do themselves. In particular, users of runInterpret itself certainly don’t—runInterpretState bears no such scar.

Comment on lines -16 to -18
instance HFunctor (Catch e) where
hmap f (Catch m h k) = Catch (f m) (f . h) (f . k)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And now the good bit: 🔥

@@ -52,7 +52,7 @@ import qualified Data.Semigroup as S
--
-- @since 1.0.0.0
(<|>) :: Has Choose sig m => m a -> m a -> m a
(<|>) a b = send (Choose (bool b a))
a <|> b = send (Choose (bool b a))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really couldn’t tell you why I wrote it like that but when I noticed it I had to fix it.

@robrix robrix marked this pull request as ready for review October 25, 2019 12:42
@robrix
Copy link
Collaborator Author

robrix commented Oct 25, 2019

It’s pretty clear that we want #296, which means we can review & merge this. (#296 subsumes HFunctor, which is what makes this a reasonable thing to do.)

If it turns out that #296 is a no-go, we can revert this after the fact, but I really don’t think that’s likely.

Copy link
Collaborator

@patrickt patrickt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this. One fewer typeclass required to get up and running is an unquestionably good thing.

@patrickt patrickt merged commit 09abbdb into master Oct 25, 2019
@robrix robrix deleted the f-x branch October 25, 2019 18:17
patrickt added a commit that referenced this pull request Nov 1, 2019
This reverts commit 09abbdb, reversing
changes made to 96ace4b.
robrix added a commit that referenced this pull request Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

What should the idiom for delegating the tail of the signature be?
2 participants