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

Support CPP as immovable comments #415

Closed
tseenshe opened this issue Oct 21, 2019 · 12 comments · Fixed by #557
Closed

Support CPP as immovable comments #415

tseenshe opened this issue Oct 21, 2019 · 12 comments · Fixed by #557
Assignees
Labels
awaiting-pr Issues that users should solve themselves and open a PR. feature-request

Comments

@tseenshe
Copy link

tseenshe commented Oct 21, 2019

I understand why you have chosen not to support CPP and I can understand why it is extremely difficult to support CPP in general. I also understand that you have an option to ignore CPP, but this typically results in uncompilable source code.

I would really like it if there were an option to treat CPP sections as immovable comments. That way the obligation would be on the user to ensure that the whitespace of unused codepaths (or full CPP blocks) is correct according to the reformatting of the rest of the file. This would allow me to continue using ormolu in my projects that use the ghc api and necessarilly must use CPP, e.g. in https://gitlab.com/tseenshe/hsinspect/tree/master/library/HsInspect

I have some ideas how to do this... e.g. in https://gitlab.com/tseenshe/hsinspect/blob/6ad08107b8ff5deec417c3c6969112f84d3bec03/library/HsInspect/Workarounds.hs#L31-64 I use the ghc api to perform the necessary preprocessing of the file (but this needs access to the ghc flags). Diffing the processed file against the original should create all the CPP "holes" that can be refilled with the original CPP paths in comment form. The correct flags are not necessary, it'd be fine if all CPP paths evaluated to false. Indeed, it should even be possible to just preprocess the file to convert all #ifdef ... #endef sections into {-#ifdef ... #endif-} and then strip the comment markers afterward.

@mrkkrp
Copy link
Member

mrkkrp commented Oct 21, 2019

you have an option to ignore CPP

To be clear, if what you mean is --tolerate-cpp, it does not ignore CPP directives and does not produce incompatible source code (to best of my knowledge). It just allows CPP language pragma to be present. Even with this flag, CPP directives won’t be parsed.

Do you propose to comment out entire conditional sections, not just the pre-processor directives?

At any rate I imagine this can pretty easily render code syntactically invalid.

@tseenshe
Copy link
Author

Hmm, try running -tolerate-cpp on the files in https://gitlab.com/tseenshe/hsinspect/tree/master/library/HsInspect to see what I mean. It seems to be treating # like an operator and therefore producing very invalid output. If that's a bug, then please disregard this feature request.

@mrkkrp
Copy link
Member

mrkkrp commented Oct 22, 2019

Interesting. I did not know that this happens. The --tolerate-cpp option is only there for debugging/testing purposes.

@mboes
Copy link
Member

mboes commented Oct 23, 2019

@tseenshe your use of CPP seems very light. If you could get rid of it entirely, then you'd get the benefit of automatic formatting, without requiring contorsions from Ormolu, which we're never going to get 100% right for all cases. You'll end up with code that's easier to reason about to boot.

Two out of the three uses of CPP currently in HsInspect are to deal with XImportDecl as an extra constructor in the ImportDecl datatype. The other one is to deal with en extra field for plugins. This is dealt with easily enough by introducing a compatibility layer, and two modules (one for GHC 8.6, and one for GHC 8.8). See CPP considered harmful.

@tseenshe
Copy link
Author

@mboes I'd rather keep the CPP and not get automatic formatting to be honest. I am aware of the post about "CPP considered harmful" but when using the ghc lib api it is simply not possible to escape use of CPP (I have plans to use CPP more extensively in the future, especially for the 8.8 api). There are many situations that can only be handled by CPP and dropping this language feature is not an option.

If ormolu were to treat CPP blocks as immovable comments, then that'd work great. If you need access to the ghcflags to be able to do that accurately, then I'm happy providing them as the user. Indeed I've hit that problem myself https://gitlab.com/tseenshe/hsinspect/tree/master/plugin and IMHO is the reason why so many Haskell tools end up not working for end users.

@mrkkrp
Copy link
Member

mrkkrp commented Oct 27, 2019

@tseenshe We talked about this internally (perhaps even more than necessary) and we concluded that we won't be spending Tweag's time on this feature because your are arguments are not convincing enough. However, if you implement it yourself it may end up in Ormolu.

@mrkkrp mrkkrp added the awaiting-pr Issues that users should solve themselves and open a PR. label Oct 27, 2019
@mboes
Copy link
Member

mboes commented Oct 27, 2019

because your are arguments are not convincing enough

To clarify: there are surely good arguments to be made in favour of this feature. They likely all need specific examples of code in the wild (which would be better than hypothetical examples of code that hasn't been written yet).

@cdsmith
Copy link

cdsmith commented Nov 1, 2019

However, if you implement it yourself it may end up in Ormolu.

I'm potentially interested in this. Could I get a better sense for the likelihood of work on this being included in ormolu? My main question, for example, is whether you're willing to accept a patch for this that doesn't provide any kind of guarantee of correctness. Guaranteeing correctness with CPP seems very difficult, but doing something that works in practice for most uses of CPP is a different matter.

@mrkkrp mrkkrp added this to Harder in Google engagement Apr 9, 2020
@mrkkrp mrkkrp moved this from Harder to Easy in Google engagement Apr 9, 2020
@NorfairKing
Copy link

I'd like to see this happen.
I'd also be happy if ormolu just did nothing when seeing cpp instead of erroring.
Now when I use --tolerate-cpp, I still get a parse error because the cpp is in the middle of my imports.

@juhp
Copy link
Contributor

juhp commented Apr 13, 2020

Just to add +1 here: erroring on CPP is a showstopper for me.

@mrkkrp mrkkrp moved this from Backlog to In progress in Google engagement Apr 21, 2020
@mrkkrp mrkkrp self-assigned this Apr 21, 2020
@mrkkrp mrkkrp moved this from In progress to Done in Google engagement Apr 22, 2020
@NorfairKing
Copy link

Beautiful!

@juhp
Copy link
Contributor

juhp commented Apr 23, 2020

Awesome I just did stack --resolver nightly install and tested it on my latest project and it seems to work just fine! 👍 👍
Thank you very much

mheinzel added a commit to wireapp/wire-server that referenced this issue Apr 27, 2020
* update ormolu version

* run new ormolu

* don't exclude CPP anymore

* format Sodium.Crypt.Sign

Changelog:
https://github.com/tweag/ormolu/blob/master/CHANGELOG.md

Most changes are caused by:

* Records with a single data constructor are now formatted more compactly. [Issue 425](tweag/ormolu#425).

* Added experimental support for simple CPP. [Issue 415](tweag/ormolu#415).

Other notable changes we can start making use of:

* Grouping of statements in `do`-blocks is now preserved. [Issue 74](tweag/ormolu#74).

* Comments on pragmas are now preserved. [Issue 216](tweag/ormolu#216).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-pr Issues that users should solve themselves and open a PR. feature-request
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants