Remove `STRef`, unify with `Ref`

purescript / purescript-st

The ST effect, for safe local mutation

BSD 3-Clause "New" or "Revised" License

25 stars 25 forks source link

Remove `STRef`, unify with `Ref` #30

Open garyb opened 4 years ago

garyb commented 4 years ago

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.

hdgarrood commented 4 years ago

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).

garyb commented 4 years ago

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.

natefaubion commented 4 years ago

Do we need optimizer rules for MonadST?

garyb commented 4 years ago

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.

natefaubion commented 4 years ago

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

garyb commented 4 years ago

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?

natefaubion commented 4 years ago

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

natefaubion commented 4 years ago

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

garyb commented 4 years ago

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.