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

Add Accum effect and carriers #391

Merged
merged 9 commits into from
Mar 1, 2022
Merged

Conversation

turion
Copy link
Contributor

@turion turion commented Jun 18, 2020

This is still work in progress, but I'm happy for some guidance. There are a few places where I've just tried to follow the conventions, but I'm not sure whether it makes sense.

src/Control/Effect/Accum.hs Outdated Show resolved Hide resolved
@turion
Copy link
Contributor Author

turion commented Jun 18, 2020

  • Tests
  • Does an example make sense?
  • Should I add benchmarks?

@turion
Copy link
Contributor Author

turion commented Jun 18, 2020

Fixes #389

@turion turion marked this pull request as draft June 18, 2020 16:40
@robrix
Copy link
Contributor

robrix commented Jul 3, 2020

Sorry I’m late on this one, but thanks for the PR!

Copy link
Contributor

@robrix robrix left a comment

Choose a reason for hiding this comment

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

This is a fantastic start, thank you @turion!

src/Control/Carrier/Accum/Church.hs Outdated Show resolved Hide resolved
ChangeLog.md Outdated Show resolved Hide resolved
src/Control/Effect/Accum.hs Outdated Show resolved Hide resolved
src/Control/Effect/Accum.hs Outdated Show resolved Hide resolved
src/Control/Effect/Accum.hs Outdated Show resolved Hide resolved
src/Control/Effect/Accum.hs Outdated Show resolved Hide resolved
src/Control/Carrier/Accum/Church.hs Outdated Show resolved Hide resolved
src/Control/Carrier/Accum/Strict.hs Outdated Show resolved Hide resolved
src/Control/Carrier/Accum/Church.hs Outdated Show resolved Hide resolved
src/Control/Carrier/Accum/Strict.hs Outdated Show resolved Hide resolved
@robrix
Copy link
Contributor

robrix commented Jul 7, 2020

@turion: Heads up that I’m about to release v1.1, which means that the @since annotations will all be 1.1.1.

@turion
Copy link
Contributor Author

turion commented Sep 10, 2020

@robrix I noticed that the monoid instance for W in the tests is Integer, which is commutative. That makes the tests less sensitive because it doesn't detect the order in which things are added to monoidal logs. Does it make sense to generalise it somewhat, e.g. to Maybe Integer?

@turion
Copy link
Contributor Author

turion commented Sep 10, 2020

@robrix I cannot figure out why this one test fails. It claims that the AccumT carrier isn't lawful, but I can't figure out why.

@turion turion marked this pull request as ready for review September 10, 2020 16:14
@turion
Copy link
Contributor Author

turion commented Sep 10, 2020

Ah. I did indeed test a property that isn't fulfilled by the transformers AccumT. In runAccumT :: AccumT w m a -> w -> m (a, w), the given w is not appended to the resulting log.

@turion
Copy link
Contributor Author

turion commented Sep 10, 2020

I think it doesn't make sense to base Accum carriers on State carriers (like it is done for Writer). Accum is fundamentally different.

@turion
Copy link
Contributor Author

turion commented Sep 10, 2020

For some reason, the test suite often hangs. I wonder whether I introduced a bug in mfix or so.

@turion
Copy link
Contributor Author

turion commented Sep 10, 2020

I haven't found the mistake in mfix yet, but it's a bit cumbersome to implement mfix for continuation monads. I'll have a look at the StateC implementation tomorrow and try again.

Also, I haven't fixed the expected law yet, but looking at the test it seems it's going to be simple.

@turion turion force-pushed the dev_accum branch 2 times, most recently from e49c6a4 to 2649c05 Compare September 11, 2020 08:11
@turion
Copy link
Contributor Author

turion commented Sep 11, 2020

Hacking on fused effects is a lot of fun, but maintaining compatibility with older GHC versions is not :/ I usually drop older versions as soon as the most idiomatic code for newer versions doesn't compile there anymore

test/Accum.hs Outdated Show resolved Hide resolved
@robrix
Copy link
Contributor

robrix commented Oct 30, 2020

Hacking on fused effects is a lot of fun, but maintaining compatibility with older GHC versions is not :/ I usually drop older versions as soon as the most idiomatic code for newer versions doesn't compile there anymore

Unfortunately, we don’t have a good way to gauge what our users’ needs are here. We’ve discussed this in #185, but I don’t feel like we came to any really solid answer. For the moment I’d prefer not to drop back-compat for changes that would otherwise be a minor version bump unless it’s completely intractable to maintain it (and even then, let’s chat about it some more first 😊).

@turion
Copy link
Contributor Author

turion commented Nov 1, 2020

Hacking on fused effects is a lot of fun, but maintaining compatibility with older GHC versions is not :/ I usually drop older versions as soon as the most idiomatic code for newer versions doesn't compile there anymore

Unfortunately, we don’t have a good way to gauge what our users’ needs are here. We’ve discussed this in #185, but I don’t feel like we came to any really solid answer. For the moment I’d prefer not to drop back-compat for changes that would otherwise be a minor version bump unless it’s completely intractable to maintain it (and even then, let’s chat about it some more first blush).

I meant it more like this: My motivation to contribute with some new, interesting features is high, but it is very low to figure out how to fix the 8.2.2 pipeline, because I need to chase an uninteresting bug with a compiler version that is different from the rest of my stack.

@turion
Copy link
Contributor Author

turion commented Nov 1, 2020

Side question: Is there any way to run only a specific set of tests in hedgehog?

@turion
Copy link
Contributor Author

turion commented Nov 1, 2020

@robrix Your suggestion to make the lambda in mfix was great. Unfortunately I had the wrong mfix implementation in the first place, but I've fixed that now.

@turion turion requested a review from robrix May 30, 2021 18:46
@turion
Copy link
Contributor Author

turion commented May 30, 2021

The only thing left is fixing older GHC versions. Let's see how the pipelines work out.

@turion
Copy link
Contributor Author

turion commented Jul 27, 2021

Ping @robrix

@turion turion mentioned this pull request Sep 6, 2021
@patrickt
Copy link
Collaborator

patrickt commented Feb 25, 2022

Can you merge master into this? I’d like to add it as part of 1.1.2.0. (Apologies for the long wait!)

@robrix
Copy link
Contributor

robrix commented Mar 1, 2022

Approved CI. Let the good times roll!

Copy link
Contributor

@robrix robrix left a comment

Choose a reason for hiding this comment

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

@turion: I'm terribly sorry this has taken me so long to get back to, and I'm so grateful for your patience. You've done a fantastic job here, and I'm delighted to add it to the library!

@robrix
Copy link
Contributor

robrix commented Mar 1, 2022

@turion Looks like you'll need to import Data.Semigroup, or else use mappend. I think we do something like this elsewhere, so a quick grep might help.

Edit: the tests for Cut are one such example: we import Data.Semigroup as S and then use S.<> in the body of the file. Using the symbol qualified prevents us from triggering warnings about a redundant import.

@robrix
Copy link
Contributor

robrix commented Mar 1, 2022

@turion: Apologies for hijacking your PR, I just couldn't wait to see CI go green ✅ 😀

@robrix
Copy link
Contributor

robrix commented Mar 1, 2022

Ah, I guess we need to CPP-guard (part of) the tests, too.

@robrix
Copy link
Contributor

robrix commented Mar 1, 2022

@turion: I'm really feeling your earlier point about supporting older versions of ghc 😅

@robrix
Copy link
Contributor

robrix commented Mar 1, 2022

It's green! It's green! Quick, merge before CI notices!!

@robrix robrix merged commit 8a95c66 into fused-effects:master Mar 1, 2022
@turion turion deleted the dev_accum branch March 1, 2022 16:16
@turion
Copy link
Contributor Author

turion commented Mar 1, 2022

Wow, the last two hours apparently were an exciting ride!

@turion
Copy link
Contributor Author

turion commented Mar 1, 2022

@turion: I'm really feeling your earlier point about supporting older versions of ghc sweat_smile

Yeah, I respect your effort to go all the way down to 8.2 a lot. In my hobby projects (which are less used than fused-effects) I typically discard an old GHC version in the moment I would have to use CPP otherwise.

@turion
Copy link
Contributor Author

turion commented Mar 1, 2022

Hah, it seems you actually agree in principle 😆 #185 (comment)

@robrix robrix mentioned this pull request Mar 6, 2022
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.

None yet

3 participants