DeriveLift extension (#1830)
ClosedPublic

Authored by RyanGlScott on Aug 23 2015, 9:40 PM.

Details

Summary

This implements -XDeriveLift, which allows for automatic derivation
of the Lift class from template-haskell. The implementation is based
off of Ian Lynagh's th-lift library
(http://hackage.haskell.org/package/th-lift).

Test Plan

./validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
RyanGlScott retitled this revision from to DeriveLift extension (#1830).Aug 23 2015, 9:40 PM
RyanGlScott updated this object.
RyanGlScott edited the test plan for this revision. (Show Details)
RyanGlScott updated the Trac tickets for this revision.

I'd like to get feedback on two implementation details:

  1. What should we do if someone attempts to derive Lift on a datatype with no constructors? I (somewhat arbitrarily) chose it to generate code like this:
data Void deriving Lift

instance Lift Void where
    lift _ = appE (varE 'error) (litE (stringL "Void lift"))

but lift _ = error "Void lift" would also work.

  1. How should primitive data types be handled? I adapted the approach Show takes, which has special cases for Char#, Float#, Double#, Int#, and Word#. Note that template-haskell also has a StringPrimL constructor, so we could handle Addr# as well, but there's not currently a function of type String -> [Word8] in base that I could use in conjunction with it (there is in ghc, but I'm not sure if it's verboten to put GHC internals in derived code).

I'd like to get feedback on two implementation details:

  1. What should we do if someone attempts to derive Lift on a datatype with no constructors?

My weakly held opinion is this should be a splice-time error.

  1. How should primitive data types be handled? I adapted the approach Show takes, which has special cases for Char#, Float#, Double#, Int#, and Word#.

This sounds reasonable to me.

Note that template-haskell also has a StringPrimL constructor, so we could handle Addr# as well, but there's not currently a function of type String -> [Word8] in base that I could use in conjunction with it (there is in ghc, but I'm not sure if it's verboten to put GHC internals in derived code).

Does map (fromIntegral . Data.Char.ord) not do the trick? That's all SMRep.stringToWord8s does.

docs/users_guide/glasgow_exts.xml
4556

Might it make sense to refer to this section from the Template Haskell section?

bgamari requested changes to this revision.Sep 2 2015, 6:31 AM

Bumping out of review queue for comments and validation failure.

This revision now requires changes to proceed.Sep 2 2015, 6:31 AM
RyanGlScott updated this revision to Diff 4182.Sep 17 2015, 9:06 PM
  • Respond to Diff feedback
RyanGlScott updated this revision to Diff 4183.Sep 17 2015, 9:12 PM
  • Beef up test
RyanGlScott added a subscriber: osa1.EditedSep 17 2015, 9:13 PM

I incorporated some of the feedback I received:

  • Deriving Lift for a constructor-less datatype now results in code which will yield an error at splice-time (I will try to coordinate with @osa1 so that it is handled uniformly alongside other language extensions in D978).
  • I expanded the documentation for how Lift works in the users' guide, and I included a little Haddock snippet for the Lift typeclass as well.
  • -XDeriveLift now handles primitive string literals.

@RyanGIScott, could you rebase this on the current state of the tree?

RyanGlScott updated this revision to Diff 4186.Sep 18 2015, 8:44 AM
  • Respond to Diff feedback
  • Beef up test
  • Fix unmatched tag in users' guide
  • Add DeriveLift to KnownExtensions

Hrm, I'm not sure why the build is failing this time. I had to make a change to a submodule (Cabal), so perhaps I didn't do it right?

goldfire accepted this revision.Sep 18 2015, 2:22 PM

Excellent patch. Thanks!

Modulo some suggestions I've made above, this looks good.

compiler/typecheck/TcGenDeriv.hs
1934

Since it would be easy to do so, perhaps the error message could be a bit more specific. It could at least include the name of the type that was lifted. It could even contain a source location!

docs/users_guide/glasgow_exts.xml
4569

Maybe modify this example to mention an unboxed thing to demonstrate that it still works in that case.

10130

It's nice to document all this. Perhaps include this fun fact: If GHC sees [| foo bar |], it looks up each name. If the name is global, the fully qualified name is used in the quotation, and any splicing code does not even need to import that identifier. If the name is local (say bar is local), then GHC uses lift, pretending that the code contains [| foo $(lift bar) |]. Thus the bar, which may not be in scope at splice locations, is actually evaluated when the quotation is processed. The use of lift here requires that a Lift instance be available.

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

Perhaps document that this can be derived.

  • goldfire's suggestions

Thanks for the review.

The comment about deriving Lift for an empty datatype was actually outdated, since I changed it to code that results in a splice-time error as per Ben and Reid's suggestions. I believe that the behavior for deriving classes on empty datatypes is going to change sometime in the future, so I'll defer any changes to that particular endeavor.

The upshot is that GHC HEAD already reports a source location whenever a TH splice fails, so I can fulfill half of your request :)

@RyanGlScott, this looks pretty good. One question inline.

Also, could you open a pull request against Cabal with the relevant changes?

compiler/typecheck/TcGenDeriv.hs
1934

@RyanGlScott, did this ever happen?

RyanGlScott updated this revision to Diff 4239.Sep 21 2015, 8:54 AM
  • More description error message when lifting empty datatypes

@RyanGlScott, this looks pretty good. One question inline.

Also, could you open a pull request against Cabal with the relevant changes?

I made the error message a bit more descriptive in the interim—it now tells you the type constructor of the empty datatype à la deriving Generic. I think that the deriving behavior for empty datatypes is bound to change eventually, though.

I opened up a pull request for adding DeriveLift to Cabal (#2831).

austin requested changes to this revision.Sep 21 2015, 7:14 PM

This patch looks great! Ryan, can you please just make this one change to the submodule? The good news is you can just get rid of the submodule change and slightly tweak T4437 instead.

libraries/Cabal
1 ↗(On Diff #4239)

Don't do this. Instead, add DeriveLift to expectedGhcOnlyExtensions inside T4437. Then, after your Cabal patch is merged, submit a PR to remove it from here and update the submodule!

This revision now requires changes to proceed.Sep 21 2015, 7:14 PM
RyanGlScott updated this revision to Diff 4252.Sep 21 2015, 7:31 PM
  • Wait until Cabal is updated (for real)
This revision was automatically updated to reflect the committed changes.
adamse added a subscriber: adamse.Sep 22 2015, 9:50 AM

This patch breaks the build:

*** framework failure for T1830(duplicate) There are multiple tests with this name 
*** framework failure for T1830(duplicate) There are multiple tests with this name

T1830 is defined in testsuite/tests/deriving/should_fail/all.T and testsuite/tests/th/all.T

RyanGlScott marked an inline comment as done.EditedSep 22 2015, 9:53 AM

Oh, now I understand why ./validate wasn't working... sorry about that.

What's the convention for dealing with this kind of thing? Should I address this in another Diff? (I can't seem to apply arc patch D1168 anymore, FWIW).

@RyanGlScott: Since it has been merged a new Diff is probably the way to go, but perhaps it is a simple enough fix that @austin or @bgamari can make a quick fix?

I'll go ahead and make another Diff anyway, since the Cabal submodule needs to be updated.

I've created D1269 to address this.