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 MonadAccum #89

Closed
wants to merge 1 commit into from
Closed

Add MonadAccum #89

wants to merge 1 commit into from

Conversation

turion
Copy link

@turion turion commented Jul 16, 2021

See #86

@chessai
Copy link
Member

chessai commented Jul 16, 2021

This looks good, but it needs a changelog entry

@turion turion force-pushed the dev_monad_accum branch 2 times, most recently from 050fb31 to 64b79da Compare July 16, 2021 20:18
@turion
Copy link
Author

turion commented Jul 16, 2021

Weird, CI is not triggered...

@chessai
Copy link
Member

chessai commented Jul 16, 2021

Weird, CI is not triggered...

I think our travis credits ran out. I am preparing a PR to switch to GitHub actions.

@chessai
Copy link
Member

chessai commented Jul 16, 2021

Can you rebase on top of master to get CI changes?

Control/Monad/Accum.hs Outdated Show resolved Hide resolved
@turion turion force-pushed the dev_monad_accum branch 2 times, most recently from 2107849 to c3eded6 Compare July 17, 2021 08:46
@turion
Copy link
Author

turion commented Jul 17, 2021

Can you rebase on top of master to get CI changes?

Done :) I think it's waiting for you to trigger the CI.

@turion
Copy link
Author

turion commented Jul 17, 2021

I'm expecting some older versions to fail, but I think it's easier to test that on CI than to install all the old versions of GHC.

@chessai
Copy link
Member

chessai commented Jul 17, 2021

I have triggered CI.

@turion turion force-pushed the dev_monad_accum branch 2 times, most recently from 51b21a9 to b1b2cfe Compare July 17, 2021 17:21
@turion
Copy link
Author

turion commented Jul 17, 2021

Getting the CPP right is really painful. Is there a good trick to run the whole CI locally somehow? Possibly using nix?

@chessai
Copy link
Member

chessai commented Sep 3, 2021

Pinging @emilypi @gwils @ekmett for review.

-- Advanced School of Functional Programming, 1995.
-----------------------------------------------------------------------------

module Control.Monad.Accum (
Copy link
Member

@chessai chessai Sep 3, 2021

Choose a reason for hiding this comment

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

Instead of re-exporting the modules, we need the haddocks to look like others. In order to do that, we need three sections:

  • MonadAccum class
  • The Accum monad
  • The AccumT monad transformer

Please refer to eg the documentation for Control.Monad.Reader

@gwils
Copy link

gwils commented Sep 6, 2021

Does MonadAccum have laws? If so, I would like to see them documented. Do we know whether these instances obey them?
I skimmed the paper linked in Control.Monad.Accum, and it looks it does not mention acccumulation monads. Why is it linked?

@turion
Copy link
Author

turion commented Sep 6, 2021

Does MonadAccum have laws? If so, I would like to see them documented.

I thought about some laws, also see this discussion if you're interested: fused-effects/fused-effects#391 (comment)

It's not clear to me how to generalise them optimally to mtl. Possibly we can phrase them by how add and look interact:

add changes the environment for look

lhs == rhs
  where
    lhs = add w >> look
    rhs = do
      w' <- look
      add w
      return $ w' <> w

looking twice yields the same

(==) <$> look <*> look == return true

adding is monoidal

add w >> add w' == add (w <> w')
add mempty == return

Do we know whether these instances obey them?

For AccumT this is fairly simple to check (if I haven't done a typo in the above).

I skimmed the paper linked in Control.Monad.Accum, and it looks it does not mention acccumulation monads. Why is it linked?

That paper introduces transformers in general, not this specific one which was discovered later.

@gwils
Copy link

gwils commented Sep 7, 2021

Thank you for putting this patch together. I don't have any problem with the code, but I'm not really sold on adding MonadAccum to mtl. Maybe @chessai can help me.

Where did Accum come from? Does we know of anyone using it for anything? It looks like an awkward point in the design space. Does this have some helpful special property that I've missed?
I would prefer to use an update monad in many cases instead of this, or otherwise use State or Writer directly.

@turion
Copy link
Author

turion commented Sep 8, 2021

Where did Accum come from?

AccumT was introduced in transformers here: https://hub.darcs.net/ross/transformers/issue/24 It was known and used before that.

Does we know of anyone using it for anything?

I've used it myself in a few places in the past, even in production. Some reasons why it's not so common as other transformers might be:

  • It arrived later to the party, so all old tutorials, teaching materials and libraries don't refer to it.
  • It is not supported by mtl (and other effect libraries like fused-effects) yet, so using it is currently a maintability risk.

The second one is kind of a self-fulfilling prophecy. mtl doesn't support Accum, so noone uses it, so noone tries to add it. In fact I came to create this PR precisely because I used it in my code, and then at some point couldn't work with my transformer stack anymore because AccumT was poorly supported, both in lacking instances and the MonadAccum class. (@chessai has improved the situation with #80 since.)

It looks like an awkward point in the design space. Does this have some helpful special property that I've missed? I would prefer to use an update monad in many cases instead of this, or otherwise use State or Writer directly.

AccumT sits in between WriterT and StateT, and has advantages and disadvantages over both.

Monad AccumT Advantage AccumT Disadvantage
WriterT Can read the log (as a monadic action) WriterT is functorial in the log type, AccumT not
StateT Extra guarantee that old logs are not discarded StateT is more flexible

Typical use cases for me are counters, or handing out unique keys/ids/tokens.

I wasn't familiar with update monads, so not sure how those relate. To me it seems that type AccumT w m a = UpdateT w w m a, where the monoid acts on the state by multiplication. So maybe AccumT is a special case of UpdateT. Nevertheless I believe that UpdateT doesn't even exist in any popular library. But AccumT is part of one of the most popular and used Haskell libraries.

Should mtl support it? I believe yes. The github readme says:

MTL is a collection of monad classes, extending the transformers package

So if mtl extends transformers, it should ideally have a class for every transformer.

--
-- The MonadAccum class.
--
-- Inspired by the paper
Copy link

Choose a reason for hiding this comment

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

I think you should remove this section given this transformer is not inspired by that paper. Some similar modules don't have this disclaimer in them, such as Control.Monad.Identity.

@turion turion mentioned this pull request May 2, 2022
@kozross
Copy link
Collaborator

kozross commented May 6, 2022

Superceded by #109

@kozross kozross closed this May 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

4 participants