purescript / purescript-st

The ST effect, for safe local mutation
BSD 3-Clause "New" or "Revised" License
25 stars 25 forks source link

Consider implementing as a newtype instead of an effect #8

Closed natefaubion closed 6 years ago

natefaubion commented 7 years ago

I recently worked up an example that highlights why an ST effect can be unintuitive: https://gist.github.com/natefaubion/30dbefe92aabc4111025859ec8b157f6

If ST were implemented as something like newtype ST h eff a = ST (Eff eff a), with a MonadEff instance, this sort of thing wouldn't be an issue.

paf31 commented 7 years ago

I modified your example so that it would compile:

runBuilder :: forall eff. (forall h. Eff (st :: ST h | eff) (Maybe String)) -> Eff eff String
runBuilder chunk = runST builderBlock
  where
  builderBlock :: forall h. Eff (st :: ST h | eff) String
  builderBlock = do
    buffer <- newSTRef ""
    build buffer

  build :: forall h. STRef h String -> Eff (st :: ST h | eff) String
  build buffer = do
    a <- chunk
    case a of
      Just str -> do
        modifySTRef buffer (_ <> str)
        build buffer
      Nothing ->
        readSTRef buffer
paf31 commented 7 years ago

I agree though that Eff rows might not be the best way to handle ST. I wouldn't be against pulling out ST into its own monad.

natefaubion commented 7 years ago

I think that's a neat solution that doesn't require a coercion, but I don't think it's something that is at all obvious 😛

kritzcreek commented 7 years ago

I wouldn't want to lose the Eff desugaring for ST refs though, the generated code is readable and fast.

paf31 commented 7 years ago

I don't think we'd have to. It would just need a few tweaks in the optimizer.

natefaubion commented 7 years ago

Also, @paf31's solution leaks the ST implementation detail into the API signature.

natefaubion commented 7 years ago

Also, @paf31's solution leaks the ST implementation detail into the API signature.

I'm not saying it's necessarily a bad thing, since it's relatively benign given that we have duplicate rows now. It's just a little unsightly (I have a related issue in purescript-run-streaming).