Hacking on GHC
After visiting CUFP and meeting a bunch of exciting folks, my desire to contribute to GHC and get some of my own changes into the Haskell language rose drastically. Starting contributing is quite intimidating, though; although I’d done it a bit before, it was quite a big challenge to do so again.
This time, I figured I’d document the process in its entirety, so that others like me could follow and commit their own changes to GHC. To be honest, much of this document is useless and is already documentd on the GHC wiki; treat it more of as a story or experience report rather than as a factual reference for anything useful.
Day 1
Day 1 of the adventure begins now!
Setting up an environment
The first large barrier to working on GHC is getting it set up. I had most of my tools set up already, so it was a matter of following the instructions on the Newcomers page.
I started by cloning GHC. I keep all my code in ~/code
, so I went ahead and cloned to
~/code/ghc
. This took about ten minutes.
$ cd ~/code/ghc $ git clone --recursive git://git.haskell.org/ghc.git Cloning into 'ghc'... Submodule 'libffi-tarballs' registered for path 'libffi-tarballs' Submodule 'libraries/Cabal' registered for path 'libraries/Cabal' Submodule 'libraries/Win32' registered for path 'libraries/Win32' Submodule 'libraries/array' registered for path 'libraries/array' Submodule 'libraries/binary' registered for path 'libraries/binary' Submodule 'libraries/bytestring' registered for path 'libraries/bytestring' Submodule 'libraries/containers' registered for path 'libraries/containers' Submodule 'libraries/deepseq' registered for path 'libraries/deepseq' Submodule 'libraries/directory' registered for path 'libraries/directory' Submodule 'libraries/dph' registered for path 'libraries/dph' Submodule 'libraries/filepath' registered for path 'libraries/filepath' Submodule 'libraries/haskeline' registered for path 'libraries/haskeline' Submodule 'libraries/hoopl' registered for path 'libraries/hoopl' Submodule 'libraries/hpc' registered for path 'libraries/hpc' Submodule 'libraries/parallel' registered for path 'libraries/parallel' Submodule 'libraries/pretty' registered for path 'libraries/pretty' Submodule 'libraries/primitive' registered for path 'libraries/primitive' Submodule 'libraries/process' registered for path 'libraries/process' Submodule 'libraries/random' registered for path 'libraries/random' Submodule 'libraries/stm' registered for path 'libraries/stm' Submodule 'libraries/terminfo' registered for path 'libraries/terminfo' Submodule 'libraries/time' registered for path 'libraries/time' Submodule 'libraries/transformers' registered for path 'libraries/transformers' Submodule 'libraries/unix' registered for path 'libraries/unix' Submodule 'libraries/vector' registered for path 'libraries/vector' Submodule 'libraries/xhtml' registered for path 'libraries/xhtml' Submodule 'nofib' registered for path 'nofib' Submodule 'utils/haddock' registered for path 'utils/haddock' Submodule 'utils/hsc2hs' registered for path 'utils/hsc2hs' Cloning into 'libffi-tarballs'... Cloning into 'libraries/Cabal'... Cloning into 'libraries/Win32'... Cloning into 'libraries/array'... Cloning into 'libraries/binary'... Cloning into 'libraries/bytestring'... Cloning into 'libraries/containers'... Cloning into 'libraries/deepseq'... Cloning into 'libraries/directory'... Cloning into 'libraries/dph'... Cloning into 'libraries/filepath'... Cloning into 'libraries/haskeline'... Cloning into 'libraries/hoopl'... Cloning into 'libraries/hpc'... Cloning into 'libraries/parallel'... Cloning into 'libraries/pretty'... Cloning into 'libraries/primitive'... Cloning into 'libraries/process'... Cloning into 'libraries/random'... Cloning into 'libraries/stm'... Cloning into 'libraries/terminfo'... Cloning into 'libraries/time'... Cloning into 'libraries/transformers'... Cloning into 'libraries/unix'... Cloning into 'libraries/vector'... Cloning into 'libraries/xhtml'... Cloning into 'nofib'... Cloning into 'utils/haddock'... Cloning into 'utils/hsc2hs'...
Once the GHC repository and all submodules are cloned, you must set up how you want to build GHC. To
do so, you create a build.mk
file with your own configuration; in my case, I wanted to build my
compiler quickly, and so stole the configuration from the
GHC wiki page on how
to make a quickly building stage 2 compiler. There are also a bunch of pre-configured builds, so you
can edit build.mk
and just uncomment one of the lines to choose a build configuration. I went
ahead and copied the GHC wiki suggestion and just inserted the following at the top of build.mk
(a
copied version of build.mk.sample
):
# My build settings for hacking on stage 2 SRC_HC_OPTS = -H32m -O -fasm GhcStage1HcOpts = -O -fasm GhcStage2HcOpts = -O0 -DDEBUG -Wall GhcLibHcOpts = -O -fasm GhcLibWays = v SplitObjs = NO
I next ran ./boot
:
$ ./boot Creating libraries/array/ghc.mk Creating libraries/array/GNUmakefile Creating libraries/base/ghc.mk Creating libraries/base/GNUmakefile Creating libraries/bin-package-db/ghc.mk ... and so on... Booting . Booting libraries/base/ Booting libraries/directory/ Booting libraries/integer-gmp/ Booting libraries/process/ Booting libraries/terminfo/ Booting libraries/time/ Booting libraries/unix/
After that comes ./configure
:
$ ./configure ... everything was ok until... checking for DocBook DTD... I/O error : Attempt to load network entity http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd conftest.xml:5: warning: failed to load external entity "http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd" ]> ^ conftest.xml:6: element book: validity error : No declaration for attribute id of element book <book id="test"> ^ conftest-book.xml:2: parser error : Entity 'ldquo' not defined <title>A DocBook “Test Document”</title> ^ conftest-book.xml:2: parser error : Entity 'rdquo' not defined <title>A DocBook “Test Document”</title> ^ conftest-book.xml:11: parser error : chunk is not well balanced ^ conftest.xml:7: parser error : Failure to process entity conftest-book &conftest-book; ^ conftest.xml:7: parser error : Entity 'conftest-book' not defined &conftest-book; ^ failed configure: WARNING: cannot find a DTD for DocBook XML V4.5, you will not be able to validate your documentation configure: WARNING: check your XML_CATALOG_FILES environment variable and/or /etc/xml/catalog checking for xsltproc... /usr/bin/xsltproc checking for DocBook XSL stylesheet... no configure: WARNING: cannot find DocBook XSL stylesheets, you will not be able to build the documentation checking for dblatex... /usr/local/bin/dblatex checking for ghc-pkg matching /Users/silver/.stack/programs/x86_64-osx/ghc-7.10.1/bin/ghc... /Users/silver/.stack/programs/x86_64-osx/ghc-7.10.1/bin/ghc-pkg checking for happy... no checking for version of happy... configure: error: Happy version 1.19.4 or later is required to compile GHC.
Turns out I was missing a few things: happy
, alex
, and something related to docbook. Installing happy
was easy with stack install happy
; same for alex
. After doing so ./configure
succeeded, so
perhaps DocBook wasn’t really an issue.
Finally we get to the long-running part: make
$ make + test -f mk/config.mk.old + cp -p mk/config.mk mk/config.mk.old touch -r mk/config.mk.old mk/config.mk + test -f mk/project.mk.old + cp -p mk/project.mk mk/project.mk.old touch -r mk/project.mk.old mk/project.mk + test -f compiler/ghc.cabal.old + cp -p compiler/ghc.cabal compiler/ghc.cabal.old touch -r compiler/ghc.cabal.old compiler/ghc.cabal ===--- building phase 0 /Applications/Xcode.app/Contents/Developer/usr/bin/make --no-print-directory -f ghc.mk phase=0 phase_0_builds ghc.mk:153: *** dyn is not in $(GhcLibWays), but $(DYNAMIC_GHC_PROGRAMS) is YES. Stop. make: *** [all] Error 2
This quickly fails with the error message above. It looks like the issue is that the variable
DYNAMIC_GHC_PROGRAMS
defaults to YES
which contradicts the fact that my copied build.mk
sets
GhcLibWays
to v
and not something including dyn
. I have no idea what this means, and after
Googling, stumble upon this page. This
still doesn’t make things quite clear, but in the interest of time I just add dyn
to GhcLibWays
and hope it fixes things, and run make -j8
again. This time, it starts compiling things!
Looking at htop
, it seems like at least for the first while, nothing really gets parallelized, at
least while building Cabal
. Once Cabal
is compiled though, the compile happily pegs my CPUs at
100% utilization.
Everything seems to go well for about 15-20 minutes until this happens:
sed 1d docs/users_guide/flags.xml >> docs/man/flags.xml "inplace/bin/ghc-stage1" -optc-m64 -optc-fno-stack-protector -optc-Wall -optc-Wall -optc-Wextra -optc-Wstrict-prototypes -optc-Wmissing-prototypes -optc-Wmissing-declarations -optc-Winline -optc-Waggregate-return -optc-Wpointer-arith -optc-Wmissing-noreturn -optc-Wnested-externs -optc-Wredundant-decls -optc-Iincludes -optc-Iincludes/dist -optc-Iincludes/dist-derivedconstants/header -optc-Iincludes/dist-ghcconstants/header -optc-Irts -optc-Irts/dist/build -optc-DCOMPILING_RTS -optc-fno-strict-aliasing -optc-fno-common -optc-DDTRACE -optc-Irts/dist/build/autogen -optc-Wno-unknown-pragmas -optc-O2 -optc-fomit-frame-pointer -optc-g -optc-DRtsWay=\"rts_v\" -static -H32m -O -fasm -Iincludes -Iincludes/dist -Iincludes/dist-derivedconstants/header -Iincludes/dist-ghcconstants/header -Irts -Irts/dist/build -DCOMPILING_RTS -this-package-key rts -dcmm-lint -DDTRACE -i -irts -irts/dist/build -irts/dist/build/autogen -Irts/dist/build -Irts/dist/build/autogen -O2 -c rts/hooks/OutOfHeap.c -o rts/dist/build/hooks/OutOfHeap.o "inplace/bin/ghc-stage1" -optc-m64 -optc-fno-stack-protector -optc-Wall -optc-Wall -optc-Wextra -optc-Wstrict-prototypes -optc-Wmissing-prototypes -optc-Wmissing-declarations -optc-Winline -optc-Waggregate-return -optc-Wpointer-arith -optc-Wmissing-noreturn -optc-Wnested-externs -optc-Wredundant-decls -optc-Iincludes -optc-Iincludes/dist -optc-Iincludes/dist-derivedconstants/header -optc-Iincludes/dist-ghcconstants/header -optc-Irts -optc-Irts/dist/build -optc-DCOMPILING_RTS -optc-fno-strict-aliasing -optc-fno-common -optc-DDTRACE -optc-Irts/dist/build/autogen -optc-Wno-unknown-pragmas -optc-O2 -optc-fomit-frame-pointer -optc-g -optc-DRtsWay=\"rts_v\" -static -H32m -O -fasm -Iincludes -Iincludes/dist -Iincludes/dist-derivedconstants/header -Iincludes/dist-ghcconstants/header -Irts -Irts/dist/build -DCOMPILING_RTS -this-package-key rts -dcmm-lint -DDTRACE -i -irts -irts/dist/build -irts/dist/build/autogen -Irts/dist/build -Irts/dist/build/autogen -O2 -c rts/hooks/StackOverflow.c -o rts/dist/build/hooks/StackOverflow.o Warning: -rtsopts and -with-rtsopts have no effect with -no-hs-main. Call hs_init_ghc() from your main() function to set these options. Build the book set list... Build the book set list... warning: failed to load external entity "/System/Library/Frameworks/Python.framework/Versions/2.7/share/dblatex/xsl/latex_book_fast.xsl" compilation error: file /private/var/folders/0l/vg6g5pdd32l6ggh_xdngnkwh0000gn/T/tmpi_nTXo/custom.xsl line 6 element import xsl:import : unable to load /System/Library/Frameworks/Python.framework/Versions/2.7/share/dblatex/xsl/latex_book_fast.xsl warning: failed to load external entity "/System/Library/Frameworks/Python.framework/Versions/2.7/share/dblatex/xsl/common/mkdoclist.xsl" compilation error: file doclist.xsl line 7 element import xsl:import : unable to load /System/Library/Frameworks/Python.framework/Versions/2.7/share/dblatex/xsl/common/mkdoclist.xsl warning: failed to load external entity "/System/Library/Frameworks/Python.framework/Versions/2.7/share/dblatex/xsl/latex_book_fast.xsl" compilation error: file /private/var/folders/0l/vg6g5pdd32l6ggh_xdngnkwh0000gn/T/tmphVadXL/custom.xsl line 6 element import xsl:import : unable to load /System/Library/FramewoUnexpected error occured Error: xsltproc failed rks/Python.framework/Versions/2.7/share/dblatex/xsl/latex_book_fast.xsl warning: failed to load external entity "/System/Library/Frameworks/Python.framework/Versions/2.7/share/dblatex/xsl/common/mkdoclist.xsl" compilation error: file doclist.xsl line 7 element import xsl:import : unable to load /System/Library/Frameworks/Python.framework/Versions/2.7/share/dblatex/xsl/common/mkdoclist.xsl Unexpected error occured Error: xsltproc failed make[1]: *** [utils/haddock/doc/haddock.ps] Error 1 make[1]: *** Waiting for unfinished jobs.... make[1]: *** [utils/haddock/doc/haddock.pdf] Error 1 make: *** [all] Error 2
This seems once more related to the DocBook issues from before. Perhaps I don’t have xsltproc
,
whatever that is?
$ which xsltproc /usr/bin/xsltproc
Nope, that’s not it. It does look like it’s unable to load some sort of libraries though, looking at these lines:
xsl:import : unable to load /System/Library/Frameworks/Python.framework/Versions/2.7/share/dblatex/xsl/latex_book_fast.xsl xsl:import : unable to load /System/Library/FramewoUnexpected error occured Error: xsltproc failed
On a whim, I tried adding DocBook support:
$ brew install docbook $ export XML_CATALOG_FILES=`brew --prefix`/etc/xml/catalog
Sadly this didn’t seem to affect much — still the same errors…
I tried with another package, which perhaps provides some data files for DocBook, maybe the ones that the GHC build can’t import:
$ brew install docbook-xsl
Nope, this is still useless. At this point, I gave up on fixing this issue through getting DocBook
to work and instead decided to disable whatever Haddock was trying to do, which
this page told me how to do: add HADDOCK_DOCKS = NO
to my build.mk
.
I then realized that in order for this to affect anything, I’d have to run ./configure
again, and
it would maybe have to rerun a bunch of make
stuff. Instead of risking that, I edited
mk/config.mk
, and found these lines:
BUILD_DOCBOOK_PS = YES BUILD_DOCBOOK_PDF = YES
and changed YES
to NO
.
At this point, compilation happily proceeded onwards. This was probably the wrong way to handle
things, but it worked. After a little while, I see it configuring base-4.8
- great!
After all of this is done, I test that it works:
$ inplace/bin/ghc-stage2 --version The Glorious Glasgow Haskell Compilation System, version 7.11.20150904
Hooray!
Making a Test Parser Change
The changes I’d like to make pertain to the parser. In order to make sure I can change the parser,
I’m going to make a tiny change to the parser file Parser.y
and see if I can prove that the change
goes through to the final compiler. I think that even though I’m fiddling with the parser, I can
stick to just rebuilding the Stage 2 compiler, so I edit the build.mk
to reflect that. From the
Newcomers page, it’s not quite clear exactly how to
make it only build Stage 2, even though it suggests doing so. There’s some more info on the
Build System page, which
suggests using the commands make stage=2
or make 2
from the ghc
subdirectory. I try it and it
completes quickly.
My parser change concerns expressions; I find that the awkwardly named exp10
rule in
compiler/parser/Parser.y
is the main rule for expressions. My first nonsensical change (as a test)
is to add the following option:
| 'soup' stmtlist {% ams (L (comb2 $1 $2) (mkHsDo DoExpr (snd $ unLoc $2))) (mj AnnDo $1:(fst $ unLoc $2)) }
This is a rule that treats the new soup
keyword as a do
block.
Unsurprisingly, this doesn’t work — the lexer isn’t updated. Alright, I guess I have to make a more reasonable first change. As suggested by the GHC wiki, I find an error message:
"parse error in lambda: no expression after '->'"
and change it to something obviously different
"HELP ME! What's going on?! parse error in lambda: no expression after '->'"
make 2
happily reruns happy
and generates me a new ghc
. I create Test.hs
:
module Test where test = \x ->
And run a command to test my error message:
# ../inplace/bin/ghc-stage2 Test.hs [1 of 1] Compiling Test ( Test.hs, Test.o ) Test.hs:3:8: error: HELP ME! What's going on?! parse error in lambda: no expression after '->'
Hooray! I’ve proven that my development cycle work.
Making my actual change
Today, I’d like to make a parser change that allows code like this:
main = do fileExists <- doesFileExist "my_file.txt" when fileExists do putStrLn "It exists!"
The main change is in the when
: I’d like the traditional $
to be optional, allowing a naked
do
. I imagine there is a good reason why this hasn’t been done, so I expect to fail due to
shift-reduce conflicts or some other parser nonsense.
I start off by making a test file containing the code above, calling it Test1.hs
.
I trace down function application to a rule named fexp
, which expands to a left-recursive rule
fexp aexp
, with aexp
probably standing for "argument expression" and fexp
probably standing
for "function expression". It seems like I want aexp
to accept raw do
blocks; looking at that
rule, it seems to boil down to aexp1
, which in turn redirects to aexp2
, which is quite meaty.
As a first pass, I just copy the do
rule from exp
to aexp2
. Let’s see what happens!
$ ../inplace/bin/ghc-stage2 Test1.hs [1 of 1] Compiling Main ( Test1.hs, Test1.o ) Test1.hs:2:17: error: Variable not in scope: doesFileExist :: [Char] -> m t0 Test1.hs:3:3: error: Variable not in scope: when :: t0 -> IO () -> m b
It parses!
However, when building Parser.y
I noticed one some worrying lines:
unused rules: 1 reduce/reduce conflicts: 39
These didn’t appear before, so before declaring victory I’d like to know what’s going on. Surely it’s not this easy! I’m not 100% sure what reduce/reduce conflicts are, and googling brings me to a Bison documentation page on the issue. The key phrase is:
A reduce/reduce conflict occurs if there are two or more rules that apply to the same sequence of input. This usually indicates a serious error in the grammar.
Well, that’s sad, although pretty expected, since we literally duplicated a rule.
In order to resolve this, we really need to understand why aexp2
ane exp10
are different rules.
exp10
has fexp
as one of its rules; fexp
can be just a aexp
, which can be just an aexp1
,
which can be just an aexp2
, so as a result aexp2
can parse directly as an exp10
. Not the other
way around, however!
This means that in order to make do
blocks behave the way we want, we need to move them from
exp10
to aexp2
.
This is much better. Running make 2
now, I don’t get any reduce/reduce conflicts or unused rules,
and my Test1.hs
still passes my brief test (it parses).
This still seems too easy! How can I discover where this breaks?
There’s a clear answer: use GHC’s parser test suite.
Sadly, it’s midnight, and I need to be awake much too soon, and I haven’t gone to sleep yet. So this is the end of day one. Day two will consist of running my new compiler on a test suite, making some changes, and maybe trying to add a LANGUAGE pragma for this change and then making a Phabricator patch/diff/commit/PR/whatever they’re called in GHC land.
End of Day 1
Day 2
I quickly find a page about running the test suite,
which tells me exactly what I need to know. I cd
to testsuite/tests/parser
and run make THREADS=8
and quickly get a result:
OVERALL SUMMARY for test run started at Sat Sep 5 00:07:17 2015 PDT 0:00:13 spent to go through 193 total tests, which gave rise to 449 test cases, of which 257 were skipped 0 had missing libraries 189 expected passes 3 expected failures 0 caused framework failures 0 unexpected passes 0 unexpected failures 0 unexpected stat failures
No changes in the parser test suite from my minor change.
I guess I should add a few tests!
My first test will just check that my feature does what it claims to do. It goes in
should_compile
. It seems like there are files called EmptyDecls.hs
and DoAndIfThenElse.hs
,
so I decide I should come up with a name for my extension and use it to name the test case. The
extension will be called ArgumentDo
(bikeshedding later) and goes in ArgumentDo.hs
:
{-# LANGUAGE ArgumentDo #-} module ArgumentDo where when :: Bool -> IO () -> IO () when True a = a when False _ = return () foo :: IO () foo = when True do return ()
Running make
again, I get the same numbers, so it seems just adding the file isn’t enough. Luckily
there’s a page on adding new test cases,
and it says I have to edit all.T
. I add the line:
test('ArgumentDo', normal, compile, [''])
Running make
now yields:
Unexpected results from: TEST="ArgumentDo" OVERALL SUMMARY for test run started at Sat Sep 5 00:15:59 2015 PDT 0:00:14 spent to go through 194 total tests, which gave rise to 452 test cases, of which 259 were skipped 0 had missing libraries 189 expected passes 3 expected failures 0 caused framework failures 0 unexpected passes 1 unexpected failures 0 unexpected stat failures Unexpected failures: should_compile ArgumentDo [exit code non-0] (normal)
Good news: it picked up the test. Bad news: the test seems to fail! Luckily, the test suite dumps
X.comp.stderr
files (where X
is the filename of the test), so I can look in
should_compile/ArgumentDo.comp.stderr
to see what went wrong:
ArgumentDo.hs:1:14: error: Unsupported extension: ArgumentDo
Oh, of course! I forgot to add the ArgumentDo
language extension. Let’s add this extension, even
though for now it will do nothing (I don’t know how to make the parser depend on extensions yet).
Adding a Language Pragma
I take DoAndIfThenElse
as an example: it’s a simple extension. Grepping through the source code
shows me that I need to change main/DynFlags.hs
. I search for DoAndIfThenElse
in that file and
make some very similar lines in each place for ArgumentDo
, and run make 2
again. Compilation
takes somewhat longer this time; more things depend on DynFlags.hs
than I expected. I guess it
makes sense, since language extensions pervade the entire way the compiler operates. I begin to
wonder if I should have attached -j8
to make make 2
command; restarting it tells me that yes, I
do indeed what that. It goes 8 times faster (ish).
Once that’s done, I run make
in the test directory again. This time it succeeds:
OVERALL SUMMARY for test run started at Sat Sep 5 00:26:34 2015 PDT 0:00:14 spent to go through 194 total tests, which gave rise to 452 test cases, of which 259 were skipped 0 had missing libraries 190 expected passes 3 expected failures 0 caused framework failures 0 unexpected passes 0 unexpected failures 0 unexpected stat failures
That said, I know these tests aren’t enough. I’ve almost certainly enabled some weird parses that weren’t valid before, and I’ll need to write code to eliminate those parses and then test that those parses were indeed elimianted. But that can come later; first, let’s make this extension actually enable and disable the feature.
To test this, I add a corresponding test in should_fail
, that mirrors ArgumentDo.hs
but lacks
the LANGUAGE pragma at the top, and then edit all.T
.
After adding that test, I see that it fails (that is, the program unexpectedly works):
Unexpected results from: TEST="ArgumentDo" OVERALL SUMMARY for test run started at Sat Sep 5 00:30:02 2015 PDT 0:00:14 spent to go through 195 total tests, which gave rise to 453 test cases, of which 259 were skipped 0 had missing libraries 190 expected passes 3 expected failures 1 caused framework failures 0 unexpected passes 1 unexpected failures 0 unexpected stat failures Unexpected failures: should_fail ArgumentDo [exit code 0] (normal)
In order to make this fail as expected, we’ll check that a do
block doesn’t occur in places it’s
not supposed to. Looking at the parser around lines 2245 in Parser.y
, it seems like the new places we’ve allowed the do
blocks
are:
-
as an argument to
fexp
-
after
static
-
after a
@
(is that parsing a pattern as an expression and then reinterpreting it?) see the section on overparsing -
after a
~
(same thing?) -
as the left hand side to a record constructor, e.g.
a { x = y}
, where thea
is -
in some arrow notation that uses
(|
and|)
For now let me just take care of the first case. From the
Parser page, it seems like
RdrHsSyn.hs
is the right place to do parse tree validation, so I stick a checkArgumentDo
in
there and export it. These changes take a very long back-and-forth with the compiler which makes me
grateful I’m working in Haskell; I have no idea how the parser or its API works, but I can just
muddle forward because I can rely on the type checker catching terrible mistakes.
-- | Yield a parse error if we have a function applied directly to a do block -- and ArgumentDo is not enabled. checkArgumentDo :: LHsExpr RdrName -> P (LHsExpr RdrName) checkArgumentDo appExpr = do pState <- getPState if isNakedDo then if xopt Opt_ArgumentDo (dflags pState) then return appExpr else parseErr else return appExpr where isNakedDo = case unLoc appExpr of HsApp _ arg -> case unLoc arg of HsDo{} -> True _ -> False _ -> False parseErr = parseErrorSDoc (getLoc appExpr) (text "Unexpected do block in function application:" $$ nest 4 (ppr appExpr) $$ text "Perhaps you meant to use ArgumentDo?")
Next I need to use this. Since it checks for function application, I can use it directly in the rule
for function application where it says fexp aexp
. That section goes from:
fexp :: { LHsExpr RdrName } : fexp aexp { sLL $1 $> $ HsApp $1 $2 } : ...
and becomes:
fexp :: { LHsExpr RdrName } : fexp aexp {% checkArgumentDo (sLL $1 $> $ (HsApp $1 $2)) } : ...
I believe I’m done! I’m going to go ahead and test again. Let’s see how badly everything breaks.
My test still fails:
Unexpected results from: TEST="ArgumentDo" OVERALL SUMMARY for test run started at Sat Sep 5 01:13:42 2015 PDT 0:00:14 spent to go through 195 total tests, which gave rise to 453 test cases, of which 259 were skipped 0 had missing libraries 190 expected passes 3 expected failures 1 caused framework failures 0 unexpected passes 1 unexpected failures 0 unexpected stat failures Unexpected failures: should_fail ArgumentDo [stderr mismatch] (normal)
This time, it’s because it returns stderr:
ArgumentDo.hs:8:7: error: Unexpected do block in function application: when True (do { return () }) Perhaps you meant to use ArgumentDo?
That’s awesome! I just need to stick that into a golden file and I’m good to go. I put it in
should_fail/ArgumentDo.stderr
and rerun. It works!
OVERALL SUMMARY for test run started at Sat Sep 5 01:15:20 2015 PDT 0:00:14 spent to go through 195 total tests, which gave rise to 453 test cases, of which 259 were skipped 0 had missing libraries 191 expected passes 3 expected failures 1 caused framework failures 0 unexpected passes 0 unexpected failures 0 unexpected stat failures
Phabricator and review
Next up, let’s make a Phabricator patch to get this in front of some other Haskell devs. I’d like to get some feedback and see what I’m missing! This still seems way too easy — there has to be a reason this hasn’t been done. Why haven’t I run into it yet?
I’ll follow the instructions on the contributing a patch page.
First, it has to be on Trac. Alright, I can make a bug report. I made one here.
Next up, I need to make a commit! I do so:
$ git checkout -b argument-do $ git add compiler/main/DynFlags.hs compiler/parser/Parser.y compiler/parser/RdrHsSyn.hs \ testsuite/tests/parser/should_compile/all.T testsuite/tests/parser/should_fail/all.T \ testsuite/tests/parser/should_compile/ArgumentDo.hs \ testsuite/tests/parser/should_fail/ArgumentDo.stderr \ testsuite/tests/parser/should_fail/ArgumentDo.hs \ $ git commit -m 'Support ArgumentDo (fixes #10843)'
I try to follow the Git commit message structure suggestions.
Alright, next up: Phabricator! I’ve already signed up before, so I just need to use the guide to figure out the Arcanist CLI commands or whatever.
Wow, it looks like all I have to do is run arc diff
. That’s easy and awesome! Sadly… I get the
following response:
zsh: segmentation fault arc diff
I should just reinstall Arcanist. It’s been a while.
That doesn’t help, but for some reason brew uninstall php55
does.
arc diff
tells me:
You have untracked files in this working copy. Working copy: /Users/silver/code/ghc/ Untracked changes in working copy: (To ignore this change, add it to ".git/info/exclude".) libraries/integer-gmp/gmp/objs/__.SYMDEF SORTED
Sure, I’ll just rm
them. arc diff
asks me a few questions and for some info, which is all easy
to fill in, and finally gives me a URL for my revision!
Hooray!