Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parse errors with CPP #774

Closed
robx opened this issue Sep 14, 2021 · 7 comments · Fixed by #775
Closed

parse errors with CPP #774

robx opened this issue Sep 14, 2021 · 7 comments · Fixed by #775

Comments

@robx
Copy link
Contributor

robx commented Sep 14, 2021

Describe the bug
Just tested out the new 0.3.0.0 release, and I see some weird errors around CPP / #ifdef.

Parse error:

$ cat x.hs
{-# LANGUAGE CPP                  #-}

module Main where

f = do

#ifdef FOO
  putStrLn "foo"
#endif

  let msg = "yo!"
  putStrLn msg
$ ./ormolu x.hs
x.hs:12:3
  The GHC parser (in Haddock mode) failed:
  parse error (possibly incorrect indentation or mismatched brackets)

Bad indentation:

$ cat y.hs
{-# LANGUAGE CPP                  #-}

module Main where

f = do

#ifdef FOO
  putStrLn "foo"
#endif

  putStrLn msg
$ ./ormolu y.hs
{-# LANGUAGE CPP #-}

module Main where

f = do

#ifdef FOO
  putStrLn "foo"
#endif

putStrLn msg

Works:

$ cat z.hs
{-# LANGUAGE CPP                  #-}

module Main where

f = do

#ifdef FOO
  putStrLn "foo"
#endif
  putStrLn msg
$ ./ormolu z.hs 
{-# LANGUAGE CPP #-}

module Main where

f = do

#ifdef FOO
  putStrLn "foo"
#endif
  putStrLn msg

Parse error:

$ cat w.hs
{-# LANGUAGE CPP                  #-}

module Main where

f = putStrLn "foo"

{-

#ifdef FOO

#endif

-}
$ ./ormolu w.hs
w.hs:(7,1)-(9,0)
  The GHC parser (in Haddock mode) failed:
  unterminated `{-'

This is on macOS with the release binary, perhaps something went wrong with that? Seems unlikely that this would have passed the test suite?

Environment

  • OS name + version: Darwin ed 18.7.0 Darwin Kernel Version 18.7.0: Tue Jun 22 19:37:08 PDT 2021; root:xnu-4903.278.70~1/RELEASE_X86_64 x86_64
  • Version of the code:
    ormolu 0.3.0.0 HEAD 06b767c
    using ghc-lib-parser 9.0.1.20210324
@amesgen
Copy link
Member

amesgen commented Sep 14, 2021

This is unfortunate, but intended behavior, the examples you mention only did work as a side effect previously. From the "Limitations" section in the README:

CPP support is experimental. CPP is virtually impossible to handle correctly, so we process them as a sort of unchangeable snippets. This works only in simple cases when CPP conditionals surround top-level declarations.

Basically, the code excerpts before an #if and after the closing #endif are now formatted separately, which is why code using constructs which span a CPP block can no longer be formatted.


As a workaround, you can surround code which uses CPP in unsupported ways with magic comments, for example like this:

{-# LANGUAGE CPP                  #-}

module Main where

{- ORMOLU_DISABLE -}
f = do

#ifdef FOO
  putStrLn "foo"
#endif

  let msg = "yo!"
  putStrLn msg

(and possibly with an {- ORMOLU_ENABLE -} in the end if there is further code below which should be formatted.)

@amesgen
Copy link
Member

amesgen commented Sep 14, 2021

Also see this entry in the changelog:

  • As a side effect, using the magic comments like in Issue 601 is no longer supported.

@robx
Copy link
Contributor Author

robx commented Sep 14, 2021

Also see this entry in the changelog:

  • As a side effect, using the magic comments like in Issue 601 is no longer supported.

I think that change deserves a more explicit changelog entry...

Would it make sense for ormolu to have a flag --ignoreFilesWithCPP?

@mrkkrp
Copy link
Member

mrkkrp commented Sep 14, 2021

Indeed, the fact that it used to work in that particular case was a side-effect of the old implementation, not intended behavior. While adding --ignore-files-with-cpp is feasible, I wonder if this is what users really want. If you use a code formatter, chances are you want uniform formatting everywhere, or in as many files as possible. Thus, selectively disabling the formatter in few problematic cases with {- ORMOLU_DISABLE -} seems like a better choice than skipping entire files, especially given that there will be many files with benign CPP that Ormolu can handle correctly.

@robx
Copy link
Contributor Author

robx commented Sep 14, 2021

Alright, maybe the issue is smaller than I thought. It's just that running the new ormolu over graphql-engine, I got parse errors in ~half the files that use CPP, and the first example seemed like such an innocent use that I started questioning things.

@amesgen
Copy link
Member

amesgen commented Sep 14, 2021

Yeah, on CI, we test formatting of e.g. graphql-engine, but we remove CPP:

cpphs "$hs_file" --noline -DARCH_X86 > "''${hs_file}-nocpp" 2> /dev/null

I created #775 to feature this issue more prominently in the changelog. Thanks for checking back on this one!

@robx
Copy link
Contributor Author

robx commented Sep 14, 2021

Thanks! Closing.

@robx robx closed this as completed Sep 14, 2021
mrkkrp pushed a commit that referenced this issue Sep 14, 2021
hasura-bot pushed a commit to hasura/graphql-engine that referenced this issue Sep 16, 2021
Some of our use of CPP causes trouble for ormolu, compare tweag/ormolu#774.
Specifically, for understandable reasons, it can't deal well with `#ifdef` use that is not at the top-level.

This PR removes the problematic usage in ways that I hope are also a net non-loss regardless of helping
out ormolu (or other tooling).

- The default value for enabled APIs moves to the top level, next to the command line help, so
  they'll stay in sync more easily.
- All the CPP around using `assertNFHere` is moved to one module.

hasura/graphql-engine-mono#2361

GitOrigin-RevId: ed6e039
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants