Add 'Lift' instances for 'NonEmpty' and 'Void'
ClosedPublic

Authored by harpocrates on Nov 28 2018, 5:54 PM.

Details

Summary

Since 'NonEmpty' and 'Void' are now part of 'base', it makes
sense that we put 'Lift' instances for them in 'template-haskell'.
Not doing so is going to force users to define their own (possibly
colliding) orphan instances downstream.

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.
harpocrates created this revision.Nov 28 2018, 5:54 PM

Thanks for doing this.

In an ideal world, we'd be able to derive all of (or at least, most of) the Lift instances in this module with DeriveLift itself. I briefly looked into this as part of Trac #14030, but alas, I ran into some technical difficulties in terms of import cycles, so I gave up on this idea. Until that becomes a reality, I think this is the best we can do here.

Some suggestions inline.

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

While this code works, it's actually subtly different than the code that DeriveLift would produce:

instance Lift a => Lift (NonEmpty a) where
  lift (x :| xs) = infixApp (lift x) (conE '(:|)) (lift xs)

That is, the AST that this instance would produce uses InfixE rather than two AppEs. I'd think it would be worth adhering to this convention for this hand-written instance as well, if only to adhere to the principle of least surprise.

715

I would define this as pure . absurd since, again, that's closer to the code that DeriveLift would actually produce.

  • Make code similar to what 'DeriveLift' generates
harpocrates marked 2 inline comments as done.Nov 28 2018, 11:29 PM
RyanGlScott accepted this revision.Nov 29 2018, 3:08 AM

LGTM.

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

Now that I think about it, hard-coding Name definitions like this is rather fragile, as it's possible that we could move NonEmpty out of GHC.Base at some point, making nonemptyName a dangling reference. This might be caught by a test if we're lucky, but we aren't always that lucky (at least in my experience, where I've had to maintain some similar code).

Of course, using DeriveLift would make this a non-issue, since then GHC would figure this stuff out for you at compile-time. Unfortunately, DeriveLift doesn't appear to Just Work™ yet when used in this module. But there is a middle ground: we could use TemplateHaskellQuotes so that we could write '(:|) instead of nonemptyName (without punishing stage-1 or cross-compiled GHCs).

If we do this, however, that does suggest that we should apply similar treatment for trueName, falseName, justName, etc. in this module. In fact, do we even need to expose these definitions from Language.Haskell.TH.Syntax (which is technically a part of the public API) at all? It seemsa bit silly to have to define (and expose) a new set of Names for each Lift instance that we add, especially since (1) TemplateHaskellQuotes already does this job much better, and (2) we'll likely continue adding more Lift instances in the future.

Wow, that comment ended up being longer than I expected. In any case, I'm only writing this up as food for thought, not as a task that needs to be completed in this Diff.

This revision is now accepted and ready to land.Nov 29 2018, 3:08 AM
harpocrates added inline comments.Nov 29 2018, 3:21 AM
libraries/template-haskell/Language/Haskell/TH/Syntax.hs
769

I heartily agree. In fact, I intially had nonemptyName = mkNameG DataName "base" "Data.List.NonEmpty" ":|"!

Getting DeriveLift to work here would really lead to simpler more robust code. I'll see if there is anything I can do for https://ghc.haskell.org/trac/ghc/ticket/14030. (If you have a good understanding of what the problem seems to be, please do drop a comment on Trac!)

RyanGlScott added inline comments.Nov 29 2018, 3:29 AM
libraries/template-haskell/Language/Haskell/TH/Syntax.hs
769

Thanks! Alas, I've forgotten why exactly I encountered troubles the last time I attempted to implement this (it must have been a full GHC release or two ago), but I do recall that I encountered build failures due to (rather unexpected) import cycles. Like an idiot, I forgot to put this information in the ticket, so it's been lost to history.

When I get a chance, I should try this again and recover just what went wrong. Or, if you happen to hit this roadblock in your own work, you could just as well document it yourself on that ticket. Whichever happens first, I suppose :)

RyanGlScott requested changes to this revision.Nov 29 2018, 5:49 PM

It looks like there's one test case that needs to be updated, per Harbormaster:

--- "/tmp/ghctest-u69n2edc/test   spaces/quotes/TH_localname.run/TH_localname.stderr.normalised"	2018-11-29 07:43:41.633694507 +0000
+++ "/tmp/ghctest-u69n2edc/test   spaces/quotes/TH_localname.run/TH_localname.comp.stderr.normalised"	2018-11-29 07:43:41.633694507 +0000
@@ -19,7 +19,7 @@
                  Language.Haskell.TH.Syntax.Lift (Maybe a)
           -- Defined in ‘Language.Haskell.TH.Syntax’
         ...plus 14 others
-        ...plus 10 instances involving out-of-scope types
+        ...plus 12 instances involving out-of-scope types
         (use -fprint-potential-instances to see them all)
      In the expression: Language.Haskell.TH.Syntax.lift y
       In the expression:
This revision now requires changes to proceed.Nov 29 2018, 5:49 PM
  • Fix test output
RyanGlScott accepted this revision.Nov 30 2018, 8:04 AM

There we go.

This revision is now accepted and ready to land.Nov 30 2018, 8:04 AM
This revision was automatically updated to reflect the committed changes.