Skip to content

Remove STRef, unify with Ref #30

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

Open
garyb opened this issue Mar 7, 2020 · 9 comments
Open

Remove STRef, unify with Ref #30

garyb opened this issue Mar 7, 2020 · 9 comments
Labels
purs-0.15 A reminder to address this issue or merge this PR before we release PureScript v0.15.0 type: breaking change A change that requires a major version bump.

Comments

@garyb
Copy link
Member

garyb commented Mar 7, 2020

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 garyb changed the title Remove STRef, modify Ref Remove STRef, unify with Ref Mar 7, 2020
@hdgarrood
Copy link
Contributor

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
Copy link
Member Author

garyb commented Mar 7, 2020

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
Copy link

Do we need optimizer rules for MonadST?

@garyb
Copy link
Member Author

garyb commented Mar 8, 2020

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
Copy link

@garyb
Copy link
Member Author

garyb commented Mar 8, 2020

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
Copy link

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

@natefaubion
Copy link

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

@garyb
Copy link
Member Author

garyb commented Mar 9, 2020

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.

@JordanMartinez JordanMartinez added purs-0.15 A reminder to address this issue or merge this PR before we release PureScript v0.15.0 type: breaking change A change that requires a major version bump. labels Dec 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
purs-0.15 A reminder to address this issue or merge this PR before we release PureScript v0.15.0 type: breaking change A change that requires a major version bump.
Projects
None yet
Development

No branches or pull requests

4 participants