Skip copyRecord for buildFromScratch in Builder

purescript / purescript-record

Functions for working with records and polymorphic labels

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

70 stars 31 forks source link

Skip copyRecord for buildFromScratch in Builder #80

Open jvliwanag opened 3 years ago

jvliwanag commented 3 years ago

Description of the change

Skip needing to copyRecord for buildFromScratch.

Checklist:

[X] 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 commented 3 years ago

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 commented 3 years ago

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

jvliwanag commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.