Possible bug in RIO's MoandWriter (listen and pass) implementations

[and-pete]

Hi Michael,

I think I've spotted two potential bugs in the RIO monad's MonadWriter implementation. Specifically with respect to listen and pass.

I'm only saying "bug" though on the assumption that transformers/mtl's WriterT output for equivalent actions being correct. I remain open to the issue being in the transformers implementation and not here.

Given the following MonadWriter actions:

say :: MonadWriter [Text] m => Text -> m ()
say x = tell [x]

actionListen :: MonadWriter [Text] m => m [Text]
actionListen = do
  say "BEFORE"
  (_, txts) <- listen (say "INSIDE")
  pure txts

actionPass :: MonadWriter [Text] m => m Int
actionPass = do
  say "START"
  pass $ pure (42, const mempty)

And the following action to allow us to get the results of the above actions in RIO.

runWriterRIO :: (MonadIO m, Monoid w) => RIO (SomeRef w) a -> m (a, w)
runWriterRIO action = do
  ref <- newSomeRef mempty
  runRIO ref do
    result <- action
    finalLog <- readSomeRef ref
    pure (result, finalLog)

I beleive that we should expect the same result if we were compare the results of using runWriterT on the two actions above to the results of using runWriterRIO.

Here are those results. For the action that uses listen:

λ> runWriterT actionListen
(["INSIDE"],["BEFORE","INSIDE"])

λ> runWriterRIO actionListen
(["BEFORE","INSIDE"],["BEFORE"])

And for the action that uses pass

λ> runWriterT actionPass 
(42,["START"])

λ> runWriterRIO actionPass 
(42,[])

Different in both cases.

If we look at the current implementation of listen:

listen :: RIO env a -> RIO env (a, w)
listen action = do
  w1 <- view writeRefL >>= liftIO . readSomeRef
  a <- action
  w2 <- do
    refEnv <- view writeRefL
    v <- liftIO $ readSomeRef refEnv
    _ <- liftIO $ writeSomeRef refEnv w1
    return v
  return (a, w2)

The two diferences from the WriterT implementation appear to be because:

1. the above does not start the Monoid w from mempty when running the action within listen like WriterT does
2. the above resets the ref to the initial pre-action value, rather than merging in the impact of the action on the Monoid into the ref with those from pre-action.

I believe the following implementation would make both consistent with WriterT:

listen :: RIO env a -> RIO env (a, w)
listen action = do
  ref <- view writeRefL
  w0 <- readSomeRef ref
  _ <- writeSomeRef ref mempty
  a <- action
  wNew <- readSomeRef ref
  modifySomeRef ref (\wNew -> w0 `mappend` wNew)
  pure (a, wNew)

And in the case of the current pass implementation for RIO:

pass :: RIO env a -> RIO env (a, w)
pass action = do
  (a, transF) <- action
  ref <- view writeRefL
  liftIO $ modifySomeRef ref transF
  return a

I believe the following should likewise fix any inconsistencies with WriterT in pass:

pass :: RIO env (a, w -> w) -> RIO env a
pass action = do
  ref <- view writeRefL
  w0 <- readSomeRef ref
  _ <- writeSomeRef ref mempty
  (a, transF) <- action
  modifySomeRef ref (\wNew -> w0 `mappend` transF wNew)
  pure a

If you think I'm on the right track and that the RIO output should match that of WriterT, I've got the above and some changes to the RIOSpec.hs tests over at this fork and am happy to send a PR. I'll push up another commit with a link to this issue in the changelog and the package.yaml bumped also.

Cheers!

[snoyberg]

I have to admit that I’m not that familiar with pass and listen, and that I haven’t read through the details here carefully. However, if our behavior mismatches mtl, I would definitely welcome a PR adding a test demonstrating this and including a fix.

[theGhostJW]

hello @and-pete
I see your fork looks pretty done but you haven't submitted a PR. I'd be happy to pick this up and run with it if you don't have time to bring this to completion.

[and-pete]

Sure @theGhostJW! Usually when I fork, it doesn't take 4 years to be brought to completion. Thanks for tagging in and taking it all the way.

[theGhostJW]

Hi @and-pete
I see your back! I'll take al look at this in the next couple of weeks.