search

clash-lang / clash-compiler

Haskell to VHDL/Verilog/SystemVerilog compiler
https://clash-lang.org/
Other
1.42k stars 151 forks source link

Add fourmolu as formatter #2735

Open rowanG077 opened 3 months ago

rowanG077 commented 3 months ago

There are a ton of file which won't pass through formatting yet. The main reason is CPP.

To make this reviewable I intend to make a formatting prepare commit per sub-project where I try to fix it(or ignore that specific file if necessary). Then once that is done I will make a wholesale formatting commit. This is done to ensure the changes I made to make formatting work won't drown out in changes done by the formatter.

leonschoorl commented 3 months ago

Please exclude clash-ghc/src-bin-* from this. Those file are basically copies of GHC's ghc/ghci binary with a little extra code added to handle the --[hdl] options and the :[hdl] commands in interactive mode.

When adding support for new GHC version to clash it is very helpful to have them changed as little as possible.

If there are actual problems with that code they should be fixed upstream.

DigitalBrains1 commented 3 months ago

Thanks for all the hard work; it's hardly fun but very useful. Some initial feedback.

I really like it if every commit passes CI. You first add the fourmolu formatting check to CI, and in a later commit make the existing code conform to that CI check. Could you reverse those so CI would also pass on the earlier commits?

Furthermore, you add LANGUAGE TemplateHaskell and LANGUAGE MagicHash to files in clash-testsuite, purely to make the file pass fourmolu. On the face of it, I think it would be better to adjust the fourmolu invocation for shouldwork and shouldfail so the extensions that clash enables by default for compilation are also enabled for fourmolu; you can apparently use -o or --ghc-opt. But actually, since these files are on occasion the literal reproducer sent to us by the people reporting an issue, I suggest we should just not invoke fourmolu on shouldwork and shouldfail. Someone might some day submit a reproducer that only works with a specific CPP incantation that fourmolu can't handle, where rewriting it to the basic form it can handle would make the reproducer stop working; sometimes very specific structures are needed to trip a certain compiler bug. We could work our way around it, by resolving the file to several versions without any CPP and then calling the correct file under the correct circumstances, but really, maybe those bug reproducers are not the code we want to make pretty anyway. I certainly apply a lot less rigour to writing those than to code that's part of Clash proper. It sounds like effort for very little gain.

rowanG077 commented 3 months ago

The goal of the commits is not to pass CI. It's to make fourmolu accept the file at all. To make CI pass I need to actually format the files and it will be the penultimate commit. Regarding formatting tests. I dislike adding those extensions wholesale to fourmolu since that means those extensions will be enabled for all other files as well. But I'm good with disable formatting for all tests as well.

What do you think @martijnbastiaan?

DigitalBrains1 commented 3 months ago

Ah, I think you misunderstood.

I like it if every commit could pass CI. But you add the fourmolu check in CI before you make the code pass that check. I'm saying that I'd rather see that you first make the code pass the check and only in a later commit add the check itself.

Because currently the commit which adds the fourmolu check will not pass CI. It's just an ordering thing, nothing has to change about the content.

If you were to currently force-push this branch to only contain the first commit Add fourmolu as formatter, the CI that runs for this PR would fail, because it would fail the added CI check. For the purposes of bisection, I really like it if any commit passes any check we throw at it.

DigitalBrains1 commented 3 months ago

Regarding formatting tests. I dislike adding those extensions wholesale to fourmolu since that means those extensions will be enabled for all other files as well.

I was suggesting making one step run fourmolu on all code except shouldwork and shouldfail, and adding another step running fourmolu on shouldwork and shouldfail, not run all code with the added options.

martijnbastiaan commented 3 months ago

What do you think @martijnbastiaan?

Keeping yet another place with default extensions doesn't seem right to me, so I'd vote for disabling formatting for it entirely. I'm very unhappy with the current state of clash-testsuite anyway - the whole fact that non of these files are part of a cabal file makes it really hard to do general improvements like these. (I understand that some bugs are only triggerable by calling Clash on source files directly, but these are the exceptions, not the rule - that should be reflected in the testsuite structure.)