tweag / ormolu

A formatter for Haskell source code
https://ormolu-live.tweag.io
Other
950 stars 83 forks source link

Parentheses between some single class constraints conflict with hlint rules #1003

Closed arybczak closed 1 year ago

arybczak commented 1 year ago

As pointed in https://github.com/tweag/ormolu/issues/264#issuecomment-1436508899, the change introduced with https://github.com/tweag/ormolu/pull/975 made it so that now specific code formatted with ormolu causes hlint to complain about redundant parentheses.

This is quite inconvenient and the workaround described in the above comment, i.e. manually adding hlint annotations is not very satisfying.

Would you consider (partially) reverting this change? I think hlint only complains about parentheses around constraints with no type parameters (like HasCallStack), so at least these could be made to be formatted with no parentheses.

amesgen commented 1 year ago

Thanks for raising a separate issue for this. There also is a potential alternative of tweaking hlint not to warn about these redundant parentheses at the constraint level (or adding an option for that behavior), as suggested in https://github.com/tweag/ormolu/issues/264#issuecomment-1436616064. WDYT?

mrkkrp commented 1 year ago

I think we should probably keep parentheses in all cases, since the original motivation for adding parentheses was that like this it is easier to add new constraints. This still holds no matter what kind of single constraint we have.

arybczak commented 1 year ago

WDYT?

I'm fine with any solution that doesn't make ormolu and hlint conflict.

amesgen commented 1 year ago

Re https://github.com/tweag/ormolu/issues/1003#issuecomment-1481460506: I opened https://github.com/ndmitchell/hlint/issues/1503.

tbidne commented 1 year ago

Incidentally, I wonder if there is any interest in adding hlint to the CI here? It could help mitigate such conflicts in the future (or make it less likely people are caught off guard, at least).

I experimented adding it here and it was very easy. Sadly this would not have caught the current issue, as it appears the ormolu code base does not have any such single class constraints. Still, seems beneficial to me (+ the benefit of hlint itself).

mrkkrp commented 1 year ago

IMO about 20-30% of HLint suggestions degrade readability of code (e.g. its obsession with writing everything as concisely as possible eliminating case expressions), so if we go that path, it would need quite a bit of tweaking.

ocharles commented 1 year ago

While I agree, with a big codebase it doesn't take long to build a good ruleset. I think it's a net win

amesgen commented 1 year ago

https://github.com/ndmitchell/hlint/issues/1503 has been resolved; hlint does no longer warn about parentheses around class constraints in all cases. We could close this issue now, or wait until there is an hlint release with the relevant fix.

mrkkrp commented 1 year ago

I think we can close. Thanks for opening that issue!

amesgen commented 1 year ago

FTR: HLint 3.6 has been released, which contains the aforementioned fix: https://github.com/ndmitchell/hlint/blob/v3.6/CHANGES.txt