Use TypeLits in the meta-data encoding of GHC.Generics
ClosedPublic

Authored by RyanGlScott on Nov 18 2014, 11:09 AM.

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes
hvr requested changes to this revision.Nov 23 2014, 11:33 AM

Please rebase to latest HEAD, so that we can see if the build would pass...

This revision now requires changes to proceed.Nov 23 2014, 11:33 AM
hvr updated this object.Nov 23 2014, 11:38 AM
hvr updated the Trac tickets for this revision.
dreixel updated this revision to Diff 1690.Nov 23 2014, 2:35 PM

Update the patch to the latest HEAD.

Patch is rebased, but I don't really know how I can see exactly what failed...

hvr added a comment.Nov 24 2014, 6:57 AM

@dreixel I just ran validate w/ D493 on top of a7c29721535d636fb16ab756b3f44224e04a5113, and got the following compile error:

$ make
===--- building phase 0
make -r --no-print-directory -f ghc.mk phase=0 phase_0_builds
make[1]: Nothing to be done for 'phase_0_builds'.
===--- building phase 1
make -r --no-print-directory -f ghc.mk phase=1 phase_1_builds
make[1]: Nothing to be done for 'phase_1_builds'.
===--- building final phase
make -r --no-print-directory -f ghc.mk phase=final all
"inplace/bin/ghc-cabal" hscolour libraries/template-haskell dist-install
Running hscolour for template-haskell-2.10.0.0...
Preprocessing library template-haskell-2.10.0.0...
"/home/hvr/Haskell/GHC/ghc/inplace/bin/haddock" --odir="libraries/template-haskell/dist-install/doc/html/template-haskell" --no-tmp-comp-dir --dump-interface=libraries/template-haskell/dist-install/doc/html/template-haskell/template-haskell.haddock --html --hoogle --title="template-haskell-2.10.0.0: Support library for Template Haskell" --prologue="libraries/template-haskell/dist-install/haddock-prologue.txt" --hide=Language.Haskell.TH.Lib.Map --read-interface=../base-4.8.0.0,../base-4.8.0.0/src/%{MODULE/./-}.html\#%{NAME},libraries/base/dist-install/doc/html/base/base.haddock --read-interface=../pretty-1.1.1.1,../pretty-1.1.1.1/src/%{MODULE/./-}.html\#%{NAME},libraries/pretty/dist-install/doc/html/pretty/pretty.haddock --optghc=-hisuf --optghc=dyn_hi --optghc=-osuf --optghc=dyn_o --optghc=-hcsuf --optghc=dyn_hc --optghc=-fPIC --optghc=-dynamic --optghc=-H32m --optghc=-O --optghc=-Werror --optghc=-Wall --optghc=-H64m --optghc=-O0 --optghc=-this-package-key --optghc=templ_C42vZOlutVC8TW7eaTG1Aw --optghc=-hide-all-packages --optghc=-i --optghc=-ilibraries/template-haskell/. --optghc=-ilibraries/template-haskell/dist-install/build --optghc=-ilibraries/template-haskell/dist-install/build/autogen --optghc=-Ilibraries/template-haskell/dist-install/build --optghc=-Ilibraries/template-haskell/dist-install/build/autogen --optghc=-Ilibraries/template-haskell/. --optghc=-optP-include --optghc=-optPlibraries/template-haskell/dist-install/build/autogen/cabal_macros.h --optghc=-package-key --optghc=base_Fk2bxXXwDEyIsDbL6nDNHy --optghc=-package-key --optghc=prett_Gs6aZ8cu8OQ4HFwF3s4B6l --optghc=-Wall --optghc=-this-package-key --optghc=template-haskell --optghc=-XHaskell2010 --optghc=-O2 --optghc=-O --optghc=-dcore-lint --optghc=-fno-warn-deprecated-flags --optghc=-fno-warn-tabs --optghc=-no-user-package-db --optghc=-rtsopts --optghc=-fno-warn-inline-rule-shadowing --optghc=-odir --optghc=libraries/template-haskell/dist-install/build --optghc=-hidir --optghc=libraries/template-haskell/dist-install/build --optghc=-stubdir --optghc=libraries/template-haskell/dist-install/build --source-module=src/%{MODULE/./-}.html --source-entity=src/%{MODULE/./-}.html#%{NAME}   libraries/template-haskell/./Language/Haskell/TH.hs  libraries/template-haskell/./Language/Haskell/TH/Lib.hs  libraries/template-haskell/./Language/Haskell/TH/Ppr.hs  libraries/template-haskell/./Language/Haskell/TH/PprLib.hs  libraries/template-haskell/./Language/Haskell/TH/Quote.hs  libraries/template-haskell/./Language/Haskell/TH/Syntax.hs  libraries/template-haskell/./Language/Haskell/TH/Lib/Map.hs  +RTS -tlibraries/template-haskell/dist-install/doc/html/template-haskell/template-haskell.haddock.t --machine-readable
Haddock coverage:
   0% (  0 /  5) in 'Language.Haskell.TH.Lib.Map'
haddock: panic! (the 'impossible' happened)
  (GHC version 7.9.20141124 for x86_64-unknown-linux):
	kindFunResult

Please report this as a GHC bug:  http://www.haskell.org/ghc/reportabug

libraries/template-haskell/ghc.mk:4: recipe for target 'libraries/template-haskell/dist-install/doc/html/template-haskell/template-haskell.haddock' failed
make[1]: *** [libraries/template-haskell/dist-install/doc/html/template-haskell/template-haskell.haddock] Error 1
Makefile:71: recipe for target 'all' failed
make: *** [all] Error 2

Currently this diff panics when haddock runs on Language.Haskell.TH.Syntax. Removing all derive Generic clauses from that file makes it go through. Surprisingly, removing the NoInline constructor on line 1268 also makes the panic go away (though things fail later on due to a missing constructor).

I have no idea what could be causing this, as this diff does not touch haddock, and everything else but this passes successfully. I also don't exactly know how to compile haddock in debug mode so that I can obtain a stack trace, for example. So I could really use some help with further debugging.

In D493#14324, @dreixel wrote:

Currently this diff panics when haddock runs on Language.Haskell.TH.Syntax. Removing all derive Generic clauses from that file makes it go through. Surprisingly, removing the NoInline constructor on line 1268 also makes the panic go away (though things fail later on due to a missing constructor).

I have no idea what could be causing this, as this diff does not touch haddock, and everything else but this passes successfully. I also don't exactly know how to compile haddock in debug mode so that I can obtain a stack trace, for example. So I could really use some help with further debugging.

The diff does not need to touch Haddock for something to crap out while it runs: the error comes from GHC, *not* Haddock. What this indicates is that Haddock's use of GHC API causes a GHC panic with your changes.

There are two ways to debug this: either start entering traces &c inside GHC or Haddock. I highly recommend you do it inside Haddock as that is far quicker to recompile than all of GHC. Of course we would still need to run Haddock on the offending module. To do that you could run the command similar to what hvr's log shows:

"/home/hvr/Haskell/GHC/ghc/inplace/bin/haddock" --odir="libraries/template-haskell/dist-install/doc/html/template-haskell" --no-tmp-comp-dir --dump-interface=libraries/template-haskell/dist-install/doc/html/template-haskell/template-haskell.haddock --html --hoogle --title="template-haskell-2.10.0.0: Support library for Template Haskell" --prologue="libraries/template-haskell/dist-install/haddock-prologue.txt" --hide=Language.Haskell.TH.Lib.Map --read-interface=../base-4.8.0.0,../base-4.8.0.0/src/%{MODULE/./-}.html\#%{NAME},libraries/base/dist-install/doc/html/base/base.haddock --read-interface=../pretty-1.1.1.1,../pretty-1.1.1.1/src/%{MODULE/./-}.html\#%{NAME},libraries/pretty/dist-install/doc/html/pretty/pretty.haddock --optghc=-hisuf --optghc=dyn_hi --optghc=-osuf --optghc=dyn_o --optghc=-hcsuf --optghc=dyn_hc --optghc=-fPIC --optghc=-dynamic --optghc=-H32m --optghc=-O --optghc=-Werror --optghc=-Wall --optghc=-H64m --optghc=-O0 --optghc=-this-package-key --optghc=templ_C42vZOlutVC8TW7eaTG1Aw --optghc=-hide-all-packages --optghc=-i --optghc=-ilibraries/template-haskell/. --optghc=-ilibraries/template-haskell/dist-install/build --optghc=-ilibraries/template-haskell/dist-install/build/autogen --optghc=-Ilibraries/template-haskell/dist-install/build --optghc=-Ilibraries/template-haskell/dist-install/build/autogen --optghc=-Ilibraries/template-haskell/. --optghc=-optP-include --optghc=-optPlibraries/template-haskell/dist-install/build/autogen/cabal_macros.h --optghc=-package-key --optghc=base_Fk2bxXXwDEyIsDbL6nDNHy --optghc=-package-key --optghc=prett_Gs6aZ8cu8OQ4HFwF3s4B6l --optghc=-Wall --optghc=-this-package-key --optghc=template-haskell --optghc=-XHaskell2010 --optghc=-O2 --optghc=-O --optghc=-dcore-lint --optghc=-fno-warn-deprecated-flags --optghc=-fno-warn-tabs --optghc=-no-user-package-db --optghc=-rtsopts --optghc=-fno-warn-inline-rule-shadowing --optghc=-odir --optghc=libraries/template-haskell/dist-install/build --optghc=-hidir --optghc=libraries/template-haskell/dist-install/build --optghc=-stubdir --optghc=libraries/template-haskell/dist-install/build --source-module=src/%{MODULE/./-}.html --source-entity=src/%{MODULE/./-}.html#%{NAME} libraries/template-haskell/./Language/Haskell/TH.hs libraries/template-haskell/./Language/Haskell/TH/Lib.hs libraries/template-haskell/./Language/Haskell/TH/Ppr.hs libraries/template-haskell/./Language/Haskell/TH/PprLib.hs libraries/template-haskell/./Language/Haskell/TH/Quote.hs libraries/template-haskell/./Language/Haskell/TH/Syntax.hs libraries/template-haskell/./Language/Haskell/TH/Lib/Map.hs +RTS -tlibraries/template-haskell/dist-install/doc/html/template-haskell/template-haskell.haddock.t --machine-readable

Personally I would start by inserting prints (traces) in various places in Haddock to first establish how far it gets before GHC panics. Once that's narrowed down, I would trace what function with what values it calls GHC with. From there it should be much easier to come up with smaller test case and chase it up and fix it in GHC itself.

austin requested changes to this revision.Dec 5 2014, 4:55 PM

(Requesting changes pending ./validate failure, to move this out of the blocking queue)

This revision now requires changes to proceed.Dec 5 2014, 4:55 PM

@dreixel, this looks like a nice use of promotion. Is there any chance you will be continuing this work?

I'm afraid not, at least not in the foreseeable future. I'd love it if someone were to pick it up, though!

RyanGlScott commandeered this revision.Nov 22 2015, 12:53 AM
RyanGlScott added a reviewer: dreixel.
RyanGlScott added a subscriber: RyanGlScott.

I rebased the changes on top of master and, interestingly, could not reproduce the Haddock errors related to template-haskell. I hope that's a good sign. I'm going to commandeer this revision and try to apply my updates to see what happens.

  • Update Generics TypeLits-related changes to GHC HEAD
bgamari requested changes to this revision.Nov 22 2015, 10:45 AM

I'm really happy to see someone pick this up. It looks like the testsuite needs to be updated, however.

This revision now requires changes to proceed.Nov 22 2015, 10:45 AM
  • Update T5642
  • Generics are faster, update perf test to reflect this
  • Update changelog for base

Yay, it passes validate!

By the way, a side-effect of this Diff is that the ill-behaved Generic instances for Char, Double, Float, and Int were removed. As a result, Trac #9526 and Trac #10512 are probably moot now.

Thanks for picking this up! I'm not surprised that the TH error is gone; this patch didn't change anything TH-related, I think it was just tripping on some unrelated bug which has been fixed in the meantime.

@kosmikus, would you care to review this?

This looks pretty good to me. I've left an idea for refinement inline.

libraries/base/GHC/Generics.hs
859

This type needs at least a brief Haddock comment.

861

Why not split the no-selector case from MetaSel (e.g. introduce a MetaNoSel constructor or make the field Maybe Symbol)? Seems a bit cleaner than just using an empy string.

testsuite/tests/perf/compiler/all.T
524

Nice improvement. All of those dummy types, instances, etc. really added up I guess.

RyanGlScott updated this revision to Diff 5371.Nov 30 2015, 2:54 PM
  • Use MetaNoSel for fields without record selectors
RyanGlScott updated this revision to Diff 5374.Nov 30 2015, 3:26 PM
  • Add documentation for Meta
  • Remove unused declaration in PrelNames
austin accepted this revision.Dec 2 2015, 2:56 PM

Okay, after refreshing myself over Trac #9766 and reading the main meat of the patch, I think this is good to go in. Ben, do you have any other concerns?

bgamari accepted this revision.Dec 2 2015, 3:40 PM

I agree; this looks good.

@RyanGlScott, once this lands do you think you could update the wiki page to reflect the fact that the proposal is now reality?

All in all, looks very nice. Thanks for this. Added two really minor comments.

I've also briefly wondered whether it'd be better to use a dedicated enumeration type as opposed to Bool to encode the isNewtype status, because the False / True in the encoding is not particularly descriptive. But I think it's ok to leave it like this for now.

compiler/prelude/PrelNames.hs
885–886

This is a minor point, but by removing the Par1 declaration from between U1 and Rec1 and changing Par0 to Par1 here, the effect is that the order of these declarations does not longer match the order of assigning the numeric ids elsewhere.

1633

See my other comment about Par1.

kosmikus accepted this revision.Dec 3 2015, 8:54 AM

@bgamari: Sure, I can pretty up the wiki.

@kosmikus: The isNewtype function has been around for a bit, so I'd be reluctant to change its type signature. I wouldn't be opposed to changing the type-level representation to something like data IsNewtype = Newtype | NotNewtype (name suggestions welcome) and changing the Datatype 'MetaData instance to something like

instance (KnownSymbol n, KnownSymbol m, KnownSymbol p, SingI nt)
    => Datatype ('MetaData n m p nt) where
  ...
  isNewtype _ = fromSing  (sing  :: Sing nt) == Newtype

Would it be appropriate to introduce data ConIsRecord = Record | NoRecord for conIsRecord as well?

RyanGlScott updated this revision to Diff 5442.Dec 3 2015, 1:39 PM
  • Make TyConNames have same order as TyConKeys for consistency
RyanGlScott updated this revision to Diff 5444.Dec 3 2015, 1:47 PM
  • Remove unused type signature for pTyConKey
bgamari requested changes to this revision.Dec 4 2015, 2:06 PM

@RyanGlScott, it appears that there is a spurious change here that needs to be fixed. See inline comment.

compiler/prelude/PrelNames.hs
412

It looks like you accidentally deleted dATA_MAYBE here which is causing validation to fail.

This revision now requires changes to proceed.Dec 4 2015, 2:06 PM
RyanGlScott updated this revision to Diff 5474.Dec 5 2015, 1:35 AM

Rebase on top of master

RyanGlScott added inline comments.Dec 5 2015, 1:38 AM
compiler/prelude/PrelNames.hs
412

I'm confused. The history doesn't show that a dATA_MAYBE ever existed in PrelNames AFAIK. Am I missing something?

bgamari accepted this revision.Dec 5 2015, 4:24 AM

Looks good. I'll merge once the validation has finished.

compiler/prelude/PrelNames.hs
412

That is incredibly odd. Indeed whatever oddness arc thought up seems to have dissipated.

RyanGlScott updated this revision to Diff 5501.Dec 6 2015, 3:45 PM
  • Remove noArityDataCon_RDR and arityDataCon_RDR
This revision was automatically updated to reflect the committed changes.