lspitzner / brittany

haskell source code formatter
GNU Affero General Public License v3.0
690 stars 72 forks source link

Fix interaction with "package environment files" (.ghc.environment.*) #173

Closed lspitzner closed 5 years ago

lspitzner commented 6 years ago

First, the facts:

1) Since ghc-8.0, ghc considers package environment files that may make packages available for usage (or something in that direction. It is not even relevant what exactly their purpose is). See the ghc users guide 2) ghc looks for these files in the current working directory and its parents. 3) cabal has started writing such files automatically, as a side-effect of invoking e.g. cabal new-build. 4) These files can go stale, and ghc may stop working in such cases. 5) Even though brittany does not invoke ghc as a process but uses the GHC API, the behaviour is identical, meaning that brittany can randomly stop working in the presence of .ghc.environment.* files.

> \> cat ./.ghc.environment.x86_64-linux-8.2.2
> outdated
> \> echo "" | brittany
> \<command line>: cannot satisfy -package-id outdated
>     (use -v for more information)

6) Since version 8.6 ghc prints a note about the environment files it considers to stdout/stderr (probably the latter, but I can't be bothered to test). This also affects usages of the GHC API. Consequently, brittany on ghc-8.6+ also prints stuff to stdout/stderr, even if this "feature" does not break the GHC API. This affects both the brittany executable and the brittany library interfaces (used by HIE).

I have opened a ticket in the ghc tracker and pointed out that this behaviour breaks stuff in a relevant cabal issue but so far there is no resolution in sight.

This raises the question of how to best mitigate this (let's be really nice and call it) excessive carelessness for the users of brittany. Let's consider possible solutions:

1) Call the GHC API in such a way that it does not read environment files. Problem with this approach is that it does not work, according to what I have tested so far. Correction: It works, if you know the magic GHC API words.

~~2) Write out a clean .ghc.environment file (to CWD) before invoking the GHC API. Downside is that we may overwrite a hand-written file, which the user might not be too happy about. 3) Delete any .ghc.environment files we can find in CWD and parent directories. Same downside as above. 3b) Delete only those generated by cabal; warn otherwise. 4) Detect the existence of .ghc.environment files, and print a nice fat warning. Make this configurable (turn off) via config. 5) Detect only exactly the broken case (bad environment file causes GHC API to fail) and print a note to the user. Downside is that is annoying to implement and does not even cover the brittany API. 6) Accept the behaviour, but document it in the README and other brittany docs.~~

None of these are satisfactory. Any other ideas welcome. I will have to/want to make a decision before the ghc-8.6 compat release, currently leaning towards 4 together with 6 to cover the brittany API.

ElvishJerricco commented 6 years ago

There must be some way to achieve solution 1, right? GHC has flags that disable the use of these files, so surely brittany can invoke the same behavior?

lspitzner commented 6 years ago

@ElvishJerricco As it turns out, yes. The trick is to use this

  dflags0           <- GHC.getSessionDynFlags
  (dflags1, [], []) <- GHC.parseDynamicFlagsCmdLine dflags0 [GHC.noLoc $ "-hide-all-packages"]
  void $ GHC.setSessionDynFlags dflags1

and this needs to be before any other invocation of setSessionDynFlags. I'll make a PR for ghc-exactprint that does this, which hopefully will solve this, for all supported ghc versions.

lspitzner commented 6 years ago

PR at https://github.com/alanz/ghc-exactprint/pull/68.

Unfortunately brittany also calls setSessionDynFlags before initDynFlags, https://github.com/lspitzner/brittany/blob/33a403975169a7ecf8f12121f9788ed8c2a65715/src/Language/Haskell/Brittany/Internal/ExactPrintUtils.hs#L70-L77

And was done on purpose, see 1b7576dcd1823e1c685a44927b1fcaade1319063. Fixed in 8c5cce50709d6c0e41aed13df808a36ce13df4a4.

Leaving this open until the changes have landed upstream and the fix is confirmed.

tfausak commented 5 years ago

Closing since this is fixed now.