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

Redefine forever #197

Open
parsonsmatt opened this issue Aug 4, 2021 · 3 comments
Open

Redefine forever #197

parsonsmatt opened this issue Aug 4, 2021 · 3 comments

Comments

@parsonsmatt
Copy link

parsonsmatt commented Aug 4, 2021

Currently ClassyPrelude exports forever as defined in base.

forever :: (Applicative f) => f a -> f b
forever action = go
  where
    go = action *> go

The forever space leak is documented here, along with links to discussion and other reports. The issue is that the default implementation of *> is:

class Applicative f where
    -- ...
    (*>) :: f a -> f b -> f b
    a1 *> a2 = (id <$ a1) <*> a2

And, unless you specifically define one of (<$) or (*>), then the implementation of forever will cause a space leak.

resoucet is subject to this, and I suspect that HandlerFor is as well. Those types use DeriveFunctor, which may avoid the issue.

I'm proposing that this library export:

forever :: (Monad m) => m a -> m b
forever action = go
  where
    go = action >> go

foreverA :: (Applicative f) => f a -> f b
foreverA = Control.Monad.forever

This should prevent this footgun from impacting users of this library.

EDIT: HandlerFor does not have this issue

@snoyberg
Copy link
Owner

snoyberg commented Aug 5, 2021

I remember a similar issue a while ago with for_ I think.

I'm about -0.5 on this proposal. It's a breaking change, which I'm generally against. It also deviates away from base further and makes the standard function slightly less useful. Given that this is an upstream issue as well, patching it here seems like it's just hiding a footgun, since any library you use may decide to use a forever from base that has this issue. It seems like education would be a better option here.

An alternative I'm slightly less bothered by would be putting a WARNING on forever, and then exporting both foreverM and foreverA with docs to explain why you would want one versus the other.

@parsonsmatt
Copy link
Author

The WARNING approach sounds good to me - a great way to do education by API design :) I can work up a PR if you'd like.

@snoyberg
Copy link
Owner

snoyberg commented Aug 5, 2021

That would be great, thanks!

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

No branches or pull requests

2 participants