Add MonadLoggerIO instance for RIO

[hololeap]

This is needed due to persist-sqlite adding a MonadLoggerIO constraint to various functions.

I found this bug due to a call to withSqliteConnInfo in pantry.

[snoyberg]

What do you mean by a "bug"? This sounds like a feature enhancement. I'm asking to make sure I understand the motivation here.

Technically there may be some issues with this PR, since it allows a logging function to outlive the scope of the RIO monad.

[hololeap]

@snoyberg pantry-0.5.1.5 fails to build against persist-sqlite-2.12.0.0:

[ 6 of 19] Compiling Pantry.SQLite    ( src/Pantry/SQLite.hs, dist/build/Pantry/SQLite.o, dist/build/Pantry/SQLite.dyn_o )

src/Pantry/SQLite.hs:30:5: error:                                          
    • Could not deduce (monad-logger-0.3.36:Control.Monad.Logger.MonadLoggerIO
                          (RIO env))                                       
        arising from a use of ‘withSqliteConnInfo’                                                                                                     
      from the context: HasLogFunc env                                                                                                                 
        bound by the type signature for:
                   initStorage :: forall env a.
                                  HasLogFunc env =>
                                  Text
                                  -> Migration
                                  -> Path Abs File
                                  -> (Storage -> RIO env a)
                                  -> RIO env a
        at src/Pantry/SQLite.hs:(19,1)-(25,14)
    • In the expression: withSqliteConnInfo (sqinfo True)
      In the second argument of ‘($)’, namely
        ‘withSqliteConnInfo (sqinfo True)
           $ runSqlConn $ runMigrationSilent migration’
      In the second argument of ‘($)’, namely
        ‘wrapMigrationFailure
           $ withSqliteConnInfo (sqinfo True)
               $ runSqlConn $ runMigrationSilent migration’
   |
30 |     withSqliteConnInfo (sqinfo True) $ runSqlConn $
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The issue is that there needs to be a MonadLoggerIO instance for RIO, and pantry compiles fine when this patch is applied to rio-orphans. Hope that makes sense.

[hololeap]

Technically there may be some issues with this PR, since it allows a logging function to outlive the scope of the RIO monad.

I'm not entirely sure what you mean by this or how to mitigate it, but if you have any specifics in mind, let me know and I'll update the PR.

I'm not familiar with the code base and this was the best I could come up with.

[snoyberg]

Hope that makes sense.

It makes sense now. Originally this read as a bug report against rio-orphans, where there isn't any specific bug.

I'm not entirely sure what you mean by this or how to mitigate it, but if you have any specifics in mind, let me know and I'll update the PR.

This is all somewhat inherent to how the systems work. The rio logging system works via the bracket pattern, allowing it to perform some cleanup after all log actions are completed (such as forcing a flush). If you use askRunInIO and then use the resulting function afterwards, the cleanup may run prematurely. It's a general problem to be aware of in Haskell.

Overall, I'm OK with this approach, if it's documented. Can you make the following changes:

• [ ] Minor version bump to the library
• [ ] Add Haddocks to the instance explaining that the user needs to ensure that usage of the instance doesn't escape the RIO context
• [ ] Add a @since comment to that Haddock
• [ ] Add a changelog entry

[hololeap]

Done. Let me know if there is anything else.

Edit:

Which of these is best? Would the second one make inlining easier?

askLoggerIO = askRunInIO <&> \runInIO loc source level str ->

askLoggerIO = askRunInIO <&> \runInIO -> \loc source level str ->

[snoyberg]

I'd prefer the former for simplicity, but in all honesty I probably would have used do-notation for a case like that.

[hololeap]

... I probably would have used do-notation for a case like that.

Alright, I switched back to do-notation and package.yaml has been updated.

[snoyberg]

Thanks!