NondecreasingIndentation-filtering broken

[hvr]

Summary. Given the following simple project:

name:                issue
version:             4443
homepage:            https://github.com/haskell/cabal/issues/4443
build-type:          Simple
cabal-version:       >=1.10

library
  build-depends:       base
  exposed-modules:     M
  default-language:    Haskell2010

  default-language: Haskell2010
  default-extensions: NondecreasingIndentation

and a dummy module M.hs containing just the single line

module M where

Try to build it via cabal build or cabal new-build for GHC 7.0.4 results in the following failure:

$ cabal build 
Preprocessing library for issue-4443..
Building library for issue-4443..
target `' is not a module name or a source file

The reason becomes apparent when looking at the -v2 output:

/opt/ghc/bin/ghc-7.0.4 --make -fbuilding-cabal-package -O -outputdir dist/build -odir dist/build -hidir dist/build -stubdir dist/build -i -idist/build -i. -idist/build/autogen -idist/build/global-autogen -Idist/build/autogen -Idist/build/global-autogen -Idist/build -optP-include -optPdist/build/autogen/cabal_macros.h -package-name issue-4443 -hide-all-packages -package-conf dist/package.conf.inplace -package-id base-4.3.1.0-bafbc7ad22c91044397c91929f8c61bc -XHaskell2010 '' M
target `' is not a module name or a source file

Which shows that -XNondecreasingIndentation got transformed into an empty '' CLI argument rather than being filtered out.

How to fix (by @ezyang).

1. Grep for NondecreasingIndentation. You're looking for a place that seems to be "filtering" the flag.

2. Here is one site in Cabal/Distribution/Simple/GHC/Internal.hs which is doing something suspicious (there's an empty string). At this point, you could verify if this was actually the relevant site by putting something in the empty strings and seeing if it shows up in the GHC command line invocation.

    let extensions0 = [ (ext, "-X" ++ display ext)
                      | Just ext <- map simpleParse extStrs ]
        extensions1 = if alwaysNondecIndent implInfo
                      then -- ghc-7.2 split NondecreasingIndentation off
                           -- into a proper extension. Before that it
                           -- was always on.
                           (EnableExtension  NondecreasingIndentation, "") :
                           (DisableExtension NondecreasingIndentation, "") :
                           extensions0
                      else extensions0

3. What is this code doing? There are two ways to find out. First, we can read the full body of getExtensions. We can see that this queries ghc --supported-languages (try it out yourself) which gives you a list of Foo and NoFoo extensions that the GHC supports. There is some extra munging going on, but that is the core of what is going on. Second, we can find the call site of getExtensions. It is in Cabal/Distribution/Simple/GHC.hs (you will see duplicate copies of the code in grep, because there is a lot of copy paste going on to support other compilers. Things with GHC in their name are likely to be relevant.) We see that it is being loaded into compilerExtensions, which records the extensions enabled for a compiler, and the FLAG used to toggle the extension on or off.

4. At this point, it is worth noting that if you have an extension like NondecreasingIndentation, this actually translates into TWO extensions in the list: NondecreasingIndentation and NoNondecreasingIndentation, because each of these are tokens which could arise in someone's default-extensions list. So, what this list is saying, is that if someone writes NoNondecreasingIndentation, this should TRANSLATE into the flag -XNoNondecreasingIndentation to be passed to GHC.

5. So, WHAT are the intended semantics of this code? The person who originally wrote this code (you can find this out with git blame) was trying to do something kind of interesting. In GHC 7.0, there was no support for -XNondecreasingIndentation or -XNoNondecreasingIndentation. Cabal could have left it at just that, but there was an interesting observation: GHC 7.0 DEFAULTED to non-decreasing indentation, so if we saw someone declare default-extensions: NondecreasingIndentation and was building with GHC 7.0, we could just ignore that extension and the user would get the desired semantics. So they said, "Well, we 'support' this flag, but if you try to specify it in a Cabal file, we're actually just going to ignore it."

6. Aren't you glad you read this code? You've discovered an unrelated bug: ghc-7.0 should NOT advertise for NoNondecreasingIndentation, there was literally no way to enable it. So you should delete that line, because that is not going to work with ghc-7.0.

7. OK, but let's talk about the actual bug. Let us trace further the flow of data from compilerExtensions. Grepping for this identifier, one important use is in Cabal/Distribution/Simple/GHC/Internal.hs, where it is slurped into ghcOptExtensionMap. This is subsequently used in Cabal/Distribution/Simple/Program/GHC.hs to determine how to render a flag to enable an extension. It is clear that if you have an entry from NondecreasingIndentation to a blank string, you will end up with a blank argument.

8. So, the hacky way to fix this, is to teach Cabal/Distribution/Simple/Program/GHC.hs to IGNORE a flag if it is just a blank string. That should be a one-liner. A more "proper" fix is to change the type of ghcOptExtensionMap and compilerExtensions from String (Flag is a synonym for String here) to Maybe String , but that may involve a lot more refactoring.

9. You should add a test for this. See the files above, and see https://github.com/haskell/cabal/tree/master/cabal-testsuite For this bug you need to make sure you pass -w ghc-7.0 because otherwise it won't repro

10. There's a lot of knowledge here! Wouldn't you have liked to see this in the code? If you are feeling kind, add documentation to relevant fields and functions with the information in this ticket.

What you will learn. How to navigate the Cabal codebase with grep. How default-extensions and other-extensions are implemented internally. The importance of accurate types and tests.