Repository navigation
Strictness testing of transformers #123
Description
Activity
More context
haskell/mtl#160 (comment)Reacted by BenjaminHello! I've been looking for an opportunity to contribute to open source and came across this in Haskell weekly.
Is this something I can try my hand on? I can also look into setting up a testing framework (e.g. tasty + hspec).Yeah go for it!
We had some issues with trying to use a test framework so don't try too hard if you get esoteric errors when using them.
Reacted by Yui Nishizawa@L0neGamer Great, I'll get started then. Thank you. I think I understand what needs to be tested from the context you provided above, but do you have any preference about how the tests are written? e.g. similar tests in some other repo. I apologize in advance if I ask silly/obvious things, as this is my first time.
Hello @L0neGamer @sjshuck
I have a preliminary PR #125 with basic test package setup that I think allows using tasty (+ other test tools).
Also have a few WriterT tests with: ynishiza#2
Sample CI run https://github.com/ynishiza/transformers/actions/runs/27491953139Would love to know if this seems like the right direction + any other comments. Thanks!
@L0neGamer One question: is this blocking anything? I see that the constraint weakening where this came up have already been implemented.
I should have asked this first but is there a time you would like to have this done by?
The reason I ask is because I have work during the day so might progress will slow down...No time limit at all! If there were I'd be frantically working at it.
Haha, got it. One pressure off my back as well. This is also serving as a good review of my understanding of strictness and WHNF.. some are a bit tricky.
Reacted by BenjaminHello @L0neGamer @sjshuck , just wanted to let you know that I have a PR for the Writer tests #127. Please take a look when you have a chance!
I wasn't able to edit the reviewers for this PR.. perhaps some security setting?Hello @L0neGamer @sjshuck @Bodigrim
I may have found a breaking change in the state transformer due to #122
The weakened implementation of the new lazy
evalStateTis:transformers/Control/Monad/Trans/State/Lazy.hs
Lines 170 to 171 in 2033934
The issue is that now, if I understand it correctly, this
~is rather pointless. This is effectively the same asfst, which is strict regardless of the~. SoevalStateTis actually now the same for both lazy and strict States.In the original implement with a monad bind, the lazy pattern match does make a difference:
transformers/Control/Monad/Trans/State/Lazy.hs
Lines 170 to 173 in 651b439
This lazy
evalStateTwill always return a monad even if the tuple is undefined, whereas the strict version may not.For example:
ghci> import qualified Control.Monad.Trans.State.Strict as S ghci> import qualified Control.Monad.Trans.State.Lazy as L ghci> (S.evalStateT (S.StateT (const $ return undefined )) () :: Maybe ()) `seq` () -- Fails before weakening, OK after weakening ghci> (L.evalStateT (L.StateT (const $ return undefined )) () :: Maybe ()) `seq` () -- OK in both casesI think I'll have to tweak the writer tests again in #127 as well, as it probably does not catch breakages at this level.
I think that this is an acceptable weakening but I would appreciate the commentary of others too.
I realize I'm not the one who's comment is mainly sought for here, so just my humble observation
take.I'm guessing in the end it will come down to what are the canonical use cases of these lazy/strict transformers...
I can't say off the top of my head, but the documentation has this example.
This still works in the weakened version since it mainly depends on the laziness
weaknessin the monad bind.ghci> take 3 $ L.evalState (sequence $ repeat $ do { n <- L.get; L.put (n*2); return n }) 1 [1,2,4]Questions related to what are the assumed use cases has also come up during the review of #127 so I'm thinking of mentioning them in the documentation of the tests.
Reacted by BenjaminThanks for analysis @ynishiza, much appreciated.
I think it's fine and probably even better. The strictness of a transformer relates to the strictness of the bind
(>>=)operation, because this is what matters for pathological accumulation of thunks. The strictness ofrunStateT/evalStateT/execStateTis rather irrelevant in the grand scheme of things: there is no unbounded chain of them. That's why I believe that changingevalStateT/execStateTin this regard is fine.Why is it even better? Because now
runStateT/evalStateT/execStateTare all aligned in their strictness. Note that you always had> import qualified Control.Monad.Trans.State.Strict as S > (S.runStateT (S.StateT (const $ return undefined )) () :: Maybe ((), ())) `seq` () () > import qualified Control.Monad.Trans.State.Lazy as L > (L.runStateT (L.StateT (const $ return undefined )) () :: Maybe ((), ())) `seq` () ()
That's because
runStateTjust unwraps a newtype and does not force any evaluation. And now, as noticed above, the same holds forevalStateT/execStateT.Reacted by Yui NishizawaThank you @Bodigrim. That makes sense to me!
I suppose in a sense, a potential breakage is also suggested a little in the weakening itself with the context now being
Functorinstead ofMonadsince Functors don't provide control over the outer constructor.
I was mainly concerned that this could break, for example, any error handling with a lazy data type that depended on the outer constructor bottoming like theMaybeexample above, but hopefully that kind of code is rare and it sounds like a bad idea anyway...If we do accept this change, perhaps it'd be better to remove the
~here for clarity, since it has no effect on the strictness of this lambda?transformers/Control/Monad/Trans/State/Lazy.hs
Lines 170 to 171 in 2033934
Sorry, I don't have capacity for a deep dive at the moment. Could you compare Core (
-ddump-simpl -dsuppress-all -dno-suppress-type-signatures) forevalState? Is it indeed the same with and without~? If yes, it can be safely removed.Before:
After. The variable is changed to
bto be 100% sure I'm looking at the Core of the modified version, since there should be no other observable difference.
Core diff of evalStateT and execStateT. Left is before with
~, right is without.
Looks like the same expressions.
For reference, the monad implementation before the weakening, with
~on the left and without on the right. Here the evaluation is lazy.
Yep, that's identical. We can indeed drop
~, thanks for the analysis.Reacted by Yui NishizawaSorry for the apparent inactivity for the past 2 weeks..
The current state of the state transformer tests can be seen here: ynishiza#4
I still need to iron out the details and add documentation but please feel free to comment if you notice anything.
Thanks!Reacted by Benjamin
It would be good to assert the strictness properties of various transformers.
The transformers to test would be StateT.{Lazy,Strict}, WriterT.{Lazy,Strict,CPS}, and RWST.{Lazy,Strict,CPS}.
Tests would include setting values to undefined to see when an exception is thrown (if at all).
Different base monads should be used such as
IdentityandIOto test strictness with regards to base monad.Tests should be of varying complexity, from the basic (enter transformer, bind value, leave transformer), to complex (enter transformer, perform various operations, possibly include other transformers or monads in the stack).
We don't currently use a testing framework like tasty (@sjshuck has found that they don't compile for some reason), so this will likely have to be a bit basic. If you do get something working with compilation/CI, all the better!
This should cover the tests I was worried about for #122 (review).
StrictCheck may be useful but we may not be able to use it.