TH 'TExp' docs + 'liftedTyped'
Needs ReviewPublic

Authored by harpocrates on Oct 2 2018, 5:00 PM.

Details

Summary
  • add Haddocks to 'TExp' describing typed splices and their advantages
  • add a new 'liftTyped :: Lift a => a -> Q (TExp a)' function (see Trac #14671)
harpocrates created this revision.Oct 2 2018, 5:00 PM
mpickering accepted this revision.Oct 2 2018, 5:18 PM
mpickering added a subscriber: mpickering.

Much better than before.

libraries/template-haskell/Language/Haskell/TH/Syntax.hs
252

Perhaps note that really lift should be implemented in terms of liftTyped rather than having this unsafe cast.

This revision is now accepted and ready to land.Oct 2 2018, 5:18 PM
harpocrates added inline comments.Oct 2 2018, 5:21 PM
libraries/template-haskell/Language/Haskell/TH/Syntax.hs
252

That would mean breaking code in the wild. Are you suggesting we put liftTyped into Lift and take lift out, or just to add a comment here as a reminder that we may someday want to do this?

monoidal added a subscriber: monoidal.

What about changing the definition of Lift to

class Lift t where
    lift :: t -> Q Exp
    lift = unTypeQ . liftTyped

    liftTyped :: t -> Q (TExp t)
    liftTyped = unsafeTExpCoerce . lift

    {-# MINIMAL lift | liftTyped #-}

This way liftTyped is a method, old instances still work, and in the future we could move lift out of the class.

What about changing the definition of Lift to

class Lift t where
    lift :: t -> Q Exp
    lift = unTypeQ . liftTyped

    liftTyped :: t -> Q (TExp t)
    liftTyped = unsafeTExpCoerce . lift

    {-# MINIMAL lift | liftTyped #-}

This way liftTyped is a method, old instances still work, and in the future we could move lift out of the class.

I would much prefer this. Unfortunately, the Lift class currently has default lift :: Data t => t -> Q Exp, so code relying on being able to derive Lift via -XDeriveAnyClass would be broken.

Although now that I write this out explicitly, I wonder if it really is a serious concern: Lift should anyways be derived via -XDeriveLift and _not_ via -XDeriveAnyClass... Besides anyone really relying on the existing behaviour could fix their could in a backwards compatible fashion with instance Lift MyDataType where { lift = liftData }.

Thoughts on this matter?

DeriveLift is supported since GHC 8.0, DeriveAnyClass since 7.10. Since GHC 8.0, "deriving (Lift)" is interpreted as stock strategy - you have to opt in for anyclass strategy (which you can do in GHC 8.2+) or write an empty "instance Lift X". Except for compatibility with 7.10, I don't see any reason to use anyclass deriving for Lift. It should give the same instance.

I've checked my local hackage mirror, there are no occurrences of "anyclass" and "Lift" in the same line, and there are only 2 packages which define an empty Lift instance: type-interpreter-0.1.4 and pads-haskell-0.0.0.1. My mirror is a bit old though (Jul 26).

I'd vote for removing "default lift :: Data t => t -> Q Exp", but either way the patch is an improvement - we can always change the Lift instance later.

harpocrates updated this revision to Diff 18189.Oct 2 2018, 9:41 PM
  • Move 'liftTyped' into the 'Lift' class

I was just suggest a comment indicating this future direction of travel not actually changing the existing patch. I don't know how this will impact code in the wild at all.

RyanGlScott requested changes to this revision.Oct 3 2018, 7:42 AM
RyanGlScott added a subscriber: RyanGlScott.

Thanks, @harpocrates! I'll get the usual requests out of the way:

  1. In the future, do make sure to update the relevant Trac tickets' info once you've submitted a patch. (I went ahead and updated Trac #14671 myself this time.)
  2. Please update template-haskell's changelog! Especially since this is a breaking change.

As far as the existing default for lift goes, I don't think it would be worthwhile fretting over it, since (1) DeriveLift is a thing, and (2) I've always been skeptical that a Data-based definition is the one we one to enshrine as the default anyway. (You could just as well implement a Generic-based default, like in here.) Now that we have DerivingVia, perhaps the prudent thing to do would to make a LiftData newtype and encourage folks who want to leverage a Data-based default to derive it via that. (Don't feel compelled to do this in this patch, however.)

I think @simonpj's original intention when opening Trac #14671 was to have liftTyped be the only method of Lift (and turn lift into a top-level function on top of liftTyped), but I find the approach you've taken here to be much more comforting from a backwards compatibility perspective, so I'd rather see this design instead.

This revision now requires changes to proceed.Oct 3 2018, 7:42 AM
harpocrates updated this revision to Diff 18193.Oct 3 2018, 9:42 AM
  • Update changelog

I think @simonpj's original intention when opening Trac Trac #14671 was to have liftTyped be the only method of Lift (and turn lift into a top-level function on top of liftTyped), but I find the approach you've taken here to be much more comforting from a backwards compatibility perspective, so I'd rather see this design instead.

That was my original idea, but I'm ok with having two methods.

I wonder if we should run a quick GHC proposal though. The discussion there is almost always illuminating, and this is a user-facing change.

I wonder if we should run a quick GHC proposal though. The discussion there is almost always illuminating, and this is a user-facing change.

It would require parking this Diff for now, but I'm inclined to agree that we should seek some amount of community feedback (be it through the GHC proposals page, the libraries mailing list, or something equivalent) before intentionally making a breaking change. (After all, it's possible that people are making use of the current default without making their code public!)

I wonder if we should run a quick GHC proposal though. The discussion there is almost always illuminating, and this is a user-facing change.

It would require parking this Diff for now, but I'm inclined to agree that we should seek some amount of community feedback (be it through the GHC proposals page, the libraries mailing list, or something equivalent) before intentionally making a breaking change. (After all, it's possible that people are making use of the current default without making their code public!)

That sounds like a good idea. I'll write up a short GHC proposal tonight.

I think that would be very helpful, thank you.

Simon

I'm very happy to see typed TH see some attention; it's a very underappreciated feature IMHO.

@harpocrates, you might consider splitting this into two differentials: one for the documentation and the other for the proposed addition.