purescript-st icon indicating copy to clipboard operation
purescript-st copied to clipboard

Remove `STRef`, unify with `Ref`

Open garyb opened this issue 5 years ago • 9 comments

Now there's the Global region, MonadST, etc. we can do away with having two Ref types:

  • Rework Ref in purescript-refs to include Region (basically, STRef -> Ref)
  • Introduce a Ref.Lifted module where the operations have a MonadST constraint so they can be used in Effect without friction.
  • Remove STRef from purescript-st
  • Remove the MonadRec instance currently in purescript-st, as it currently is implemented with STRef. Implement it in purescript-tailrec instead (implementation is basically identical to the Effect instance already in there).

I've done all this locally to check it works out 🙂.

I'd like to make this change when we do the updates for PS 0.14 unless there is a strong feeling we shouldn't for some reason. Aside from some module stuff moving around the main breaking difference is that existing Refs will need to become Ref Global.

garyb avatar Mar 07 '20 14:03 garyb

I’d really prefer to not make this such a large breaking change if we possibly can. Can we consider continuing to have two separate Ref types, one in ST and one in Refs, which are the same underneath - perhaps one is a newtype of the other? That way you still get to share the implementation and also allow converting between them if that’s something you care about, but you also get to continue not to care that they’re the same underneath if that’s irrelevant to your use case (which, in my experience, it is most of the time).

hdgarrood avatar Mar 07 '20 15:03 hdgarrood

Global was kinda introduced specifically so this change could be made. If we were just trying to restrict the FFI implementation to one place we could have coerced the existing Ref implementation (or STRef, either one).

I get not wanting to break things unnecessarily, but also I don't think we should maintain situations that are less than ideal just to avoid breaking changes, especially if we're making a batch of breaking changes anyway.

Another step in the above that is something I know you've been a proponent of in the past: renaming the modules to match the library names rather than having Control.Monad.ST and Effect.Ref as it is now. I don't think the other changes I'm proposing are any more severe than the work required for people to update their code for that.

garyb avatar Mar 07 '20 15:03 garyb

Do we need optimizer rules for MonadST?

natefaubion avatar Mar 08 '20 22:03 natefaubion

Do we even have optimizer rules for ST at the moment? They got lost when we changed from Eff but I can't remember if they were restored.

garyb avatar Mar 08 '20 22:03 garyb

https://github.com/purescript/purescript/commit/02ecfe6d10fdc02ff4cf599c040707dab1a7281a#diff-7a57dc19daa5d2bf0d6c4ea9f7dd54a9

natefaubion avatar Mar 08 '20 22:03 natefaubion

Ah ok, great. In that case, if there is some kind of sensible optimization for MonadST, having something for it would be good yeah. Did you have something in mind for what it'd do?

garyb avatar Mar 08 '20 22:03 garyb

I think that we would want to apply optimizations based on which dictionary is chosen (whether for Effect or ST) and inline appropriately.

natefaubion avatar Mar 08 '20 22:03 natefaubion

Ideally there wouldn't be a penalty for using MonadST even in a monomorphic setting.

natefaubion avatar Mar 08 '20 22:03 natefaubion

A proposal that @natefaubion suggested and I had also mulled but disregarded: to make the change less breaking we could exports synonyms and such from the existing name modules, as existing code should still work then.

garyb avatar Mar 09 '20 20:03 garyb