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

Implement record functor / field functor sequencing and joining #126

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cfhammill
Copy link

These help implement unnesting for list records.

I'm not completely
happy with the MapTypeDeCon family in that it isn't applicable to
symbol paired fields. I couldn't get the reverse equivalent of
ApplyToField to work with the injectivity annotation.

Also not sure if join is the right term here. Really it's about swapping
applicative implementations for pure and liftA2.

These help implement unnesting for list records. I'm not completely
happy with the MapTypeDeCon family in that it isn't applicable to
symbol paired fields. I couldn't get the reverse equivalent of
ApplyToField to work with the injectivity annotation.
@cfhammill
Copy link
Author

@acowley any advice for getting this mergeable?

@acowley
Copy link
Contributor

acowley commented Mar 8, 2019

Right off the bat, I apologize for how long this has taken me. My hang up is that it looked like something would be confusing in Core.hs with these changes. I think it's mostly just that the types are perhaps a bit subtle, but I'd like to feel confident we've landed on the right types.

Okay, first, rsequenceIn is similar to rsequence. In rsequenceIn, we go from Rec (f :. m) xs -> Rec m (MapTyCon f xs). While in rsequence, we have a bunch of m x fields carried by a Rec f, so we could imagine a Rec f (MapTyCon m xs) -> Rec (f :. m) xs function. This isn't easy to write I think because GHC doesn't see that Rec f (MapTyCon m xs) should (I think) be coercible to Rec (f :. m) xs. Could we fix that? I'd like this to be possible, but I've spent some time trying and haven't gotten it yet. Basically something like this:

rcoerce1 :: Rec f (MapTyCon m xs) -> Rec (f :. m) xs
rcoerce1 = unsafeCoerce

rcoerce2 :: Rec (f :. m) xs -> Rec f (MapTyCon m xs)
rcoerce2 = unsafeCoerce

rsequence :: forall f m (xs :: [*]). (Applicative m, Traversable f, RMap xs)
          => Rec f (MapTyCon m xs) -> Rec m (MapTyCon f xs)
rsequence = rcoerce2 . rmap (onCompose sequenceA) . rcoerce1

We know that every field is exists a. f (m a) and that forall f m a. Coercible (f (m a)) ((f :. m) a), but I don't know what to write to let GHC see that.

Second, we usually do rjoin-like operations with rtraverse. If we are adding some duplication that we think is worth the potential confusion, we have to take care to document it, which you do with the observation in the rjoin comments. I think that sort of flexibility is usually achieved with newtypes, but I'm open to the perspective that newtype is more of a hassle. If we want to draw a line in the sand there, maybe we should define rtraverseFuns in terms of a record of functions (except will we then run into problems with inlining?), and then a helper that uses the Applicative constraint to get those functions (i.e. rtraverse = rtraverseFun pure liftA2).

I also find the rjoin naming a little uncomfortable since it seems closer to what Traversable offers rather than Monad. The fact that it fixes Identity as the result stands out, so we may want to give it a more concrete name like rtaverseId or something.

Again, sorry for how long this took. Would you be open to reconsidering the rjoin naming? Do you agree with my concern about the rsequence factoring, or can you help me see what's happening? If we can't find a better way of defining the rsequence-related things over the next couple days, we'll go ahead with what you've got here.

@cfhammill
Copy link
Author

Thanks @acowley, no problem at all on the delay.

Your sketch for rsequence looks promising, what is the biggest advantage to switching to the coerce based approach? I've never used unsafeCoerce before, so I'm a bit squeamish, but happy to try that approach out. Alternatively we could make an analogue to rtraverseIn (rtraverseOut?) where we swap mapTyCon for mapTyDeCon. I think we might get caught on injectivity, but worth a shot.

Also yes re rjoin, it was a bad name choice, very open to changing it. I like the idea of rtraverseFun. I think we can generalize the type to get rid of the explicit identity. If we put a Distributive constraint in we can be polymorphic over the input record functor. I also don't know if we want to pass in a Record of functions, I think having a single fixed unnesting function is ok for now.

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