-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add Final Effect #217
Add Final Effect #217
Conversation
@@ -66,7 +67,7 @@ withLowerToIO action = do | |||
res <- embed $ A.async $ do | |||
a <- action (runViaForklift inchan) | |||
(putMVar signal ()) | |||
putMVar signal () | |||
`finally` (putMVar signal ()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pointing this out so you won't miss it; this fixes the "exception" part of #205 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<3
Should I add |
@@ -1,5 +1,5 @@ | |||
name: polysemy | |||
version: 1.1.0.0 | |||
version: 1.2.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version bump in order to update dependencies for polysemy-plugin-test.
@@ -1,5 +1,34 @@ | |||
# Changelog for polysemy | |||
|
|||
|
|||
## 1.2.0.0 (TODO) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marked the date as TODO here since I figure we might want to do more for this version before we publish it to Hackage.
Pretty much ready now. It's a bit silly; I figured it'd be a bit longer before we were ready to merge this, which is why I first imported the final rework to |
I'm gonna stew on this for a day to make sure I'm satisfied with it before I merge it in. |
A question in my mind is if we shouldn't just have runM :: Monad m => Sem '[Embed m, Final m] a -> m a It's backwards-compatible (except when people explicitly write out their effect stack instead of using Plus, if we do this we can remove I know we already discussed this when it came to |
Oh, I should ping you too about that. It's the only unresolved question I have about this, with the exception of potential performance issues. @isovector |
For the record: the performance issue I'm worried about is how It that's indeed the case, A quick and dirty fix for that is to have |
@TheMatten Does that sound right to you? (As in, is it plausible that's indeed what would happen?) I tried building GHC 8.9 to confirm this, but as I'm on windows, using Hadrian turned out to resemble hell much closer than to my liking. |
I'd prefer to keep |
Oh, the performance question was separate from the question about Whatever the case, I have no strong opinions. I prefer changing |
I guess I'm still not sure what |
(go . wv) | ||
ex | ||
ins | ||
Left g -> hoist go g |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found this neat optimisation made possible from the new injWeaving
, which cleans up the code to boot!
@KingoftheHomeless having some simple example that we can bench and |
Although I found it while working on getting continuations to work, |
Ahh, right. Sorry, not sure why I can't seem to remember that. I'm definitely sympathetic to the idea, though why do we need the |
@isovector As I understand it, this would let us use |
Partly backwards compatibility, partly user convenience. Since |
That's a pretty good argument. I'm on board, but let's hold off for a 2.0 release. |
Ok! That's reasonable to me. I'll go ahead and merge this, then. |
There are a few things I want to hash out with you before I declare this ready.