VinylRecords / Vinyl

Extensible Records for Haskell. Pull requests welcome! Come visit us on #vinyl on freenode.
http://hackage.haskell.org/package/vinyl
MIT License
260 stars 49 forks source link

Implement record functor / field functor sequencing and joining #126

Open cfhammill opened 5 years ago

cfhammill commented 5 years ago

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.

cfhammill commented 5 years ago

@acowley any advice for getting this mergeable?

acowley commented 5 years ago

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

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.