Addresses Trac #9766
Details
Details
Diff Detail
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
Comment Actions
@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
Comment Actions
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.
Comment Actions
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.
Comment Actions
(Requesting changes pending ./validate failure, to move this out of the blocking queue)
Comment Actions
@dreixel, this looks like a nice use of promotion. Is there any chance you will be continuing this work?
Comment Actions
I'm afraid not, at least not in the foreseeable future. I'd love it if someone were to pick it up, though!
Comment Actions
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.
Comment Actions
I'm really happy to see someone pick this up. It looks like the testsuite needs to be updated, however.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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. | |
Comment Actions
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?
Comment Actions
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?
Comment Actions
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. | |
Comment Actions
@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) == NewtypeWould it be appropriate to introduce data ConIsRecord = Record | NoRecord for conIsRecord as well?
Comment Actions
@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. | |
| 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? | |
Comment Actions
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. | |