Skip to content
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

Skip copyRecord for buildFromScratch in Builder #80

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jvliwanag
Copy link

@jvliwanag jvliwanag commented Mar 25, 2021

Description of the change

Skip needing to copyRecord for buildFromScratch.


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

@natefaubion
Copy link
Contributor

In general, I think it's a little dodgy to reason about allocation and references within PureScript like this, since it's outside the semantics of the language, and there's otherwise no indication in the types what's happening. For example, if an optimization were to lift out the {} to the top-level, since it can be trivially shared, you'd wind up with a broken program, where every call is mutating the same reference.

I'm not necessarily giving a thumbs down, because I think it's likely we have issues like this all over the place in core.

@natefaubion
Copy link
Contributor

One option could be to just move buildFromScratch to the FFI.

@jvliwanag
Copy link
Author

If we follow that line of thought though, perhaps we shouldn't be declaring: newtype Builder a b = Builder (a -> b) as well? Since we're hiding the mutation.

@natefaubion
Copy link
Contributor

If we follow that line of thought though, perhaps we shouldn't be declaring: newtype Builder a b = Builder (a -> b) as well? Since we're hiding the mutation.

Right, which is why I provided the caveat:

because I think it's likely we have issues like this all over the place in core.

I'm aware that core uses unsafe functions for builder stuff like this, but gives them pure types within PureScript's type system, which is semantically dodgy. It's like declaring type Effect a = Unit -> a, which is technically true, but I don't think anyone would agree it's a good idea.

@jvliwanag
Copy link
Author

Perhaps the cleanest representation of Builder would involve ST. My worry is it might not be as efficient.

Alternatively, we can make Builder a foreign type and move everything onto FFI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants