-
Notifications
You must be signed in to change notification settings - Fork 141
Export unstreamPrimM & unsafeUnstreamPrimM #544
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
base: master
Are you sure you want to change the base?
Conversation
This gives more options for working with monadic streams Fixes haskell#416
-- makes writes to a single buffer and copies it when finished. Note | ||
-- for monads that encode nondeterminism result may be different | ||
-- from `unstreamM`. | ||
-- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for monads that encode nondeterminism result may be different from
unstreamM
.
Is it possible? Can the results be different? I mean, I believed that the results are always the same; I thought I just can't prove it mathematically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure myself. But they are different operationally: unstreamM
uses immutable data structures and unstreamPrimM
and in principle can observe mutations. ListT
mutates elements in particular order so results are always same.
Perhaps some lawless variant of OmegaT
built on top https://hackage-content.haskell.org/package/control-monad-omega-0.3.3/docs/Control-Monad-Omega.html will work. Or some tricks with ContT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't this is the case at all and IMHO unstreamPrimM
is absolutely safe with unsafeFreeze
instead of freeze
at the end.
If this operation is unsafe for some transformer that would mean its instance for PrimMonad
is incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite agree with this PR. I think we can just safely export unstreamPrimM
as it is today
-- @since NEXT_VERSION | ||
unsafeUnstreamPrimM :: (PrimMonad m, Vector v a) => MBundle m u a -> m (v a) | ||
{-# INLINE_FUSED unsafeUnstreamPrimM #-} | ||
unsafeUnstreamPrimM s = M.munstream s >>= unsafeFreeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in my other comment I don't think there is anything unsafe about this function. I would love to be proven wrong with an example of using some known PrimMonad
instance that violates the safety of this function.
-- This function is unsafe. For monads that encode nondeterminism | ||
-- (e.g. @ListT@) it allows to break referential transparency. More | ||
-- precisely if 'unsafeFreeze' is called more than once we will | ||
-- perform writes into buffer which is considered immutable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an example of using ListT
where it results in this craziness?
Honestly, even if there is one, ListT
has been deprecated for a long time.
I believe that if there is a transformer that violates properties of this function, then it does not deserve an instance for PrimMonad
, i.e. it is unlawful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're talking about ListT done right; see
#416 (comment)
This ListT satisfies:
ListT m
satisfies monad laws for every monadm
ListT
safisfiesMonadTrans
lawsListT m
wherePrimMonad m
satisfiesstToPrim v >>= stToPrim . f == stToPrim (v >>= f)
andstToPrim (pure a) == pure a
What else would you require?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole reason why PrimMonad
exists is to allow mutation of arrays and other mutable structures. Therefore I would expect any monad that has an instance would also satisfy: new[Array|ByteArray..] n >>= unsafeFreeze == new[Array|ByteArray..] n >>= freeze
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is obviously satisfied by ListT m
, right?
newArray n >>= unsafeFreeze
== stToPrim (newArray n) >>= stToPrim . unsafeFreeze
== stToPrim (newArray n >>= unsafeFreeze)
== stToPrim (newArray n >>= freeze)
== stToPrim (newArray n) >>= stToPrim . freeze
== newArray n >>= freeze
Well, I'm sure you're meaning something stronger, but I don't know how to formalize your idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can't rely on a function like this to be safe:
generateArray :: PrimMonad m => Int -> (Int -> m a) -> m (Array a)
generateArray n
| n <= 0 = pure emptyArray
| otherwise = do
marr <- newArray n undefined
let go i = when (i < n) $ f i >>= writeArray marr i >> go (i + 1)
go 0
unsafeFreezeArray marr
Then I am more than sure there is a lot of code that is unsafe out there!
So, if you think there is an issue here, then we need to tighten requirements on PrimMonad
and what sort monads can have instances for that class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should raise a ticket on the primitive
package and cross reference this conversation and get its maintainers involved in the discussion. Any volunteers?
but I'd love both unstreamPrimM and unstreamPrimMProtective.
If this issue is solved at the PrimMonad
level, then there is no point in having an alternative version, because it would not even be clear what this "protective" variant would mean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. This is not only vector's problem. I'm planning to do that tomorrow. Hopefully it would be possible to create reasonable design.
Also I wonder whether it's possible to violate referential transparency using ContT monad. It allows basically anything but I couldn't construct such example. I didn't try very hard though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether it's possible to violate referential transparency using ContT monad
Answering my own question. Of course it is. Everything is possible with ContT. One just need to encode some backtracking:
import Control.DeepSeq
import Control.Monad.Trans.Cont
import Control.Monad.IO.Class
import Data.Vector.Unboxed qualified as VU
import Data.Vector.Unboxed.Mutable qualified as VUM
liftList :: Monad m => [a] -> ContT [r] m a
liftList xs = ContT $ \cont ->
concat <$> traverse cont xs
continuedHorror :: IO [([Int], VU.Vector Int)]
continuedHorror = flip runContT (pure . pure) $ do
mv <- VUM.generateM 2 $ \i -> liftList [i, i+5]
v <- VU.unsafeFreeze mv
let !i = force $ VU.toList v
pure (i,v)
It returns:
([0,1],[5,6])
([0,6],[5,6])
([5,1],[5,6])
([5,6],[5,6])
and of course replacing unsafeFreeze
with safe variant fixes bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lehins I created haskell/primitive#431
Since ContT
is instance of PrimMonad
already and allows to break referential transparency solution will probably involve subclass of PrimMonad
where such use of unsafeFreeze
is safe. Besides simply having way to lift ST into monad is useful too event if it can do backtracking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shimuuar Very nice minimal reproducer for the problem 💪
This gives more options for working with monadic streams
As was discussed in #416 unstream variant which writes directly in buffer is unsafe when it uses
unsafeFreeze
and monad support nondeterminism. ThusunstreamPrimM
usesfreeze
and add copy but this is still an improvement overunstreamM
which create intermediate list.unsafeUnstreamPrimM
is provided as wellFixes #416