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

Streaming exposes an unsafe destroy #34

Open
treeowl opened this issue Aug 26, 2017 · 3 comments
Open

Streaming exposes an unsafe destroy #34

treeowl opened this issue Aug 26, 2017 · 3 comments

Comments

@treeowl
Copy link

treeowl commented Aug 26, 2017

foo, bar :: Stream (Of Char) [] Char
foo = lift ['a','b'] >>= lift . (: [])
bar = lift (['a','b'] >>= (: []))

By the monad transformer laws, foo = bar, but in fact destroy foo P.show P.show P.show /= destroy bar P.show P.show P.show. I blame destroy. Indeed, Streaming.Internal.destroyExposed carefully documents the dangers and necessary restrictions, but destroy is (presumably accidentally) implemented using identical code:

destroy
  :: (Functor f, Monad m) =>
     Stream f m r -> (f b -> b) -> (m b -> b) -> (r -> b) -> b
destroy stream0 construct effect done = loop stream0 where
  loop stream = case stream of
    Return r -> done r
    Effect m  -> effect (liftM loop m)
    Step fs  -> construct (fmap loop fs)

destroyExposed stream0 construct effect done = loop stream0 where
  loop stream = case stream of
    Return r -> done r
    Effect m  -> effect (liftM loop m)
    Step fs  -> construct (fmap loop fs)

The most obvious fix is to simply define

destroy = destroyExposed . unexposed

but there may be a slightly more efficient way.

@treeowl
Copy link
Author

treeowl commented Aug 26, 2017

I think the cleaned up version probably looks like this:

destroy
  :: (Functor f, Monad m) =>
     Stream f m r -> (f b -> b) -> (m b -> b) -> (r -> b) -> b
destroy s construct effect done = effect (loop s) where
  loop stream = case stream of
    Step fs -> return (construct (fmap (effect . loop) fs))
    Effect m -> m >>= loop
    Return r -> return (done r)

@treeowl
Copy link
Author

treeowl commented Aug 26, 2017

FYI, it appears this bug was introduced in e2ab123. I believe my implementation likely does a better job of fusing code than the previous working version did, but I haven't tried benchmarking that.

@andrewthad
Copy link
Contributor

This is no longer the official repo for streaming. Please continue discussion of this issue here.

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