-
Notifications
You must be signed in to change notification settings - Fork 24
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 support for bidirectional effects #29
Conversation
I'll rebase to solve conflicts. |
71ab306
to
5a77660
Compare
5a77660
to
a1aab85
Compare
Could you maybe add a couple of example files with expected output check files to the |
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.
Looks already pretty good to me. I added a few suggested changes that would be great if you could incorporate them. As already mentioned, it would also be great to have a few examples that illustrate the new feature.
In the changes I propose to reject programs where resume
is used in the wrong way. I think that is easiest for now. If you would like to stick with the automatic adaption to the correct kind, I think we need another, consistent way that actually changes the terms.
@b-studios I applied suggested changes and added an example. Please review it. |
closing and reopening to trigger CI |
Thank you @long-long-float for your contributions! As you will see (after fixing my accidental bug missing a brace), there are still a few tests that do not pass and I cannot merge your feature without the CI going through. The Effekt language has multiple backends and your bidirectionality feature only supports one of them (JavaScript), which is fine. It is expected that your new test fails on all other backends. You can see how the bidirectionality test can be disabled in the other backends here: effekt/jvm/src/test/scala/effekt/ChezSchemeTests.scala Lines 18 to 28 in 581b111
(If you are interested, you can of course also consider supporting the other backends -- like Chez Scheme, but this is not strictly necessary for now). However, all other tests that are not concerned with bidirectionality should still pass. Maybe you can look into this? If you have any questions, I am happy to assist. |
@@ -100,7 +100,7 @@ case class VarDef(id: IdDef, annot: Option[ValueType], binding: Stmt) extends De | |||
case class EffDef(id: IdDef, ops: List[Operation]) extends Def { | |||
type symbol = symbols.UserEffect | |||
} | |||
case class Operation(id: Id, tparams: List[Id], params: List[ValueParams], ret: Effectful) extends Definition { | |||
case class Operation(id: IdDef, tparams: List[Id], params: List[ValueParams], ret: Effectful) extends Definition |
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.
Oh.. I forgot the opening brace here, when editing this in the online editor. @long-long-float could you fix this in your branch?
@b-studios The CI fails and says an error |
Yes, it should warn about an effect leaving its scope as you can see in the check file. Would be great if you could look into this |
@b-studios I fixed the test case of
I Investigated the cause and guess that the position of the type of an operator ( |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Update: I just modified Namer in master so you only need to rebase. This should hopefully also solve the problem you described here. |
@@ -571,7 +579,13 @@ class Typer extends Phase[Module, Module] { typer => | |||
effs = (effs ++ (stmtEffs -- handled)) | |||
} | |||
|
|||
(params zip args) foreach { case (ps, as) => checkArgumentSection(ps, as) } | |||
(params zip args) foreach { | |||
// Special case - treat 1st param type () => T as T |
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.
Maybe you could expand a bit on this comment (and maybe add an example to examples/pos
?). This part is really the most tricky bit, I believe.
(the detailed explanation is not necessary for merging into master, but would be nice at some point)
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 reconsidered about it, it is unnecessary. So I removed it.
Co-authored-by: Jonathan <jonathan@b-studios.de>
47a79b5
to
ff5ea1a
Compare
@b-studios All tests are passed! Thanks for your advice. |
Thanks for your contributions @long-long-float, which I'll merge now! If you want, you could add more examples of the feature, potentially translating the examples from the original paper? The paper is presented this week at OOPSLA and having translated their examples could facilitate discussion. |
Of course I'm creating more examples for the paper 😄 |
@long-long-float Just to be clear: What I meant is: could you take examples from the existing paper by Zhang er al., translate them to Effekt and add them to this repository? |
Yes, I'm translating examples from the existing paper by Zhang er al., to Effekt.
|
Hey, thanks! I added a quick translation of the iterator example here to talk about it with people at the OOPLSA conference. |
Cool! I have just translated the same example here. |
That's great. Since you are the expert on bidirectional effects in the Effekt language, do you want to document it on the website? We have a separate repository for that. I added a quick stub, but there could be much detailed explanation about what bidirectional effects motivates, how they work and how they interact with effect polymorphism: https://github.com/effekt-lang/effekt-website/blob/master/docs/bidirectional.md |
Sure, I want to do. I'll write it with my master thesis. |
Supersedes and closes #29 Co-authored-by: @edding4500
This PR adds the feature of bidirectional control flow to the Effekt language. The feature is inspired by the paper