streaming icon indicating copy to clipboard operation
streaming copied to clipboard

Streaming exposes an unsafe destroy

Open treeowl opened this issue 8 years ago • 3 comments

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 avatar Aug 26 '17 02:08 treeowl

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 avatar Aug 26 '17 03:08 treeowl

FYI, it appears this bug was introduced in e2ab123d446428f668527935c70e302c9612a1bd. 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.

treeowl avatar Aug 26 '17 05:08 treeowl

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

andrewthad avatar Sep 13 '17 13:09 andrewthad