-
Notifications
You must be signed in to change notification settings - Fork 28
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 effectful-th package #56
Conversation
@arybczak I think this one can be reviewed and merged. 🙂 |
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.
Thanks!
Do other packages have more tests for a bit more "weird" effect data types that can be added?
For example, I wonder if this works: polysemy-research/polysemy#339
@@ -0,0 +1,238 @@ | |||
{-# LANGUAGE CPP #-} | |||
{-# LANGUAGE TemplateHaskell #-} |
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.
Can you replace this with TemplateHaskellQuotes?
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.
Unfortunately that is not possible. Changing that to TemplateHaskellQuotes
results in:
src/Effectful/TH.hs:156:40: error:
Operator applied to too few arguments: :>
|
156 | effectConstraint = [t| $(effect) :> $(varT es) |]
|
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.
This requires getting rid of splices, but it should be possible as you can construct the AST manually. FWIW optics-th
uses TemplateHaskellQuotes only. This matters for cross-compilation.
, setMakeFunctionFor | ||
, setToFunctionName | ||
|
||
-- * Re-exports |
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.
Why the re-exports?
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.
I included them for slightly better ergonomics: This way one can just import Effectful.TH
and is not confronted with a bunch of "... not in scope" error messages immediately.
Unfortunately the current implementation does not work well with "weird" effects.
are reified to something like:
Note the order of the type variables of the explicit forall is completely lost and we cannot figure out how to construct the type applications at the call site. I will think about a more elaborate solution using |
I wouldn't worry about that, it's better to use |
That's why I chose it in the first place 🙂 I propose the following: Unless I find a clear path forward throughout the next days will push some more documentation regarding known limitations and we can merge this (I will of course address the remaining issues first). If we figure out a way to improve the module in this regard it can happen on a separate PR. |
I think I'll go with #60. The code is trivially ported from |
Superseded by #60, thanks for the work though! |
This PR adds a new package
effectful-th
including a TH functionmakeSendFunctions
that generates functions for the effect operations of dynamic effects (See the OP of #20).The test suite included just checks if the generated functions for the dynamic effects defined in
effectful-core
compile.