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 &ldquo;Test Document&rdquo;</title>
                          ^
conftest-book.xml:2: parser error : Entity 'rdquo' not defined
  <title>A DocBook &ldquo;Test Document&rdquo;</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:

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.

— Bison docs

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:

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 the a 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!