RFC: Applying HLint's

[ulysses4ever]

Dear all, we just merged an HLint config and CI.

• 9041

Cabal code base has plenty of violations of the rules: https://github.com/haskell/cabal/blob/bd7197b4e5bf6e2fdd806736a09e7ab20fa9fc36/.hlint.yaml

Should we fix some of them? I heard people have different tastes about which hlints are actually good. So maybe you have a set of "categorically" bad/good ones in mind, which we could use to define a set of those we want to fix...

[ulysses4ever]

/cc @philderbeast @Mikolaj @gbaz @Kleidukos @ffaf1 @fgaz @andreabedini

[andreabedini]

I don't have strong opinions on any of those. I have been silently applying some of them in the code I touch, avoiding perhaps eta reductions just to not be too conspicuous :-)

[ffaf1]

I dislike these

hlint -i "Use infix" -i "Use String" -i "Eta reduce" \
      -i "Redundant ==" -i "Use fromMaybe" -i "Use &&&" \
      -i "Redundant if" -i "Use isNothing" -i "Use <$>" \
      -i "Use $>" -i "Use >=>" -i "Use void" \
      -i "Avoid reverse" src/

but in general, for the sake of consistency, if the project decided to turn some warnings on, we act on those.

[Kleidukos]

Unfortunately I have opinions: While the following hints are lawful transformations, they require reading the codebase with too much context:

Eta reduce
Avoid lambda
Avoid lambda using `infix`
Use tuple-section

Sometimes it is obviously better to lay out things explicitly instead of implicitly, especially if we want to keep the overhead required for examining the codebase to a minimum. I don't mind requiring some domain knowledge about Cabal to read the code, but I particularly hate having to rewrite stuff in my head to expand the tacit forms. It is an unnecessary exercise when there are more important things to focus on.

Edit: @dixonary has stumbled upon the "Eta reduce" lint introducing a compilation error due to the incomplete implementation of Quick Look. This and "Redundant $" sometimes trigger such errors.

[philderbeast]

The effort to make a pull request for a single hlint suggestion can be pretty low (depending on the count and whether hlint --refactor can be used). If reviewers don't like a change that can be a good time to move a suggestion into a separate section of hints we don't like for this project.

[ulysses4ever]

@philderbeast I'm not a big fan of slicing it thin for a reason: it takes energy to process every PR on the reviewers' side, and we have a very limited pool of reviewers. I'd much rather we agree on which hlints we don't like and have one (or couple) PRs fixing the rest, or, at least, what is possible to fix automatically. But I can see value in a slower approach too. Just as one of the people who patrols PRs regularly I see a lot of extra work coming my way with the smaller PRs :-)

[philderbeast]

I am cautious and prefer the one pull request per hint approach (or if not that then at least one hint per commit). I find it much easier to manage (making the change and reviewing). Something to be wary of, sometimes hlint suggestions cascade. Another thing seen in #9112, there's going to be a bit of tension between hlint and fourmolu wherein a small hlint change leads to a larger fourmolu change.

[Mikolaj]

I like the help and discipline hlint provides, especially to newbies, but I'd at least wait a long while after the fourmolu transition is complete, we have feedback and can reflect on the ordeal and the dust generally settles.

[ulysses4ever]

@Mikolaj fair enough. I'm not very keen on "dust settles" timeouts because it's hard to measure. But anyway. Do you want to shut down #9111, #9112?

[philderbeast]

I quickly tried some of the other hints (but held back from turning them into pull requests). HLint is such a great tool.

[Mikolaj]

Do you want to shut down https://github.com/haskell/cabal/pull/9111, https://github.com/haskell/cabal/pull/9112?

These are perfectly good PRs that improve the codebase. I'm grateful for them. But do we encourage more? This is really tricky. Such code improvements are, relatively, shallow, they don't improve code style consistency in a reliable way (no hlint in CI and others may even undo the changes while working on the same code; it's their right in the absence of CI checks), they cost review time and make wading through patches while tracking a code change over time harder. I'm sure I missed some pros and cons.

Definitely, a PR that removes dead code (after asking around if it's not used in a weird way in some test script) is 10 times more valuable (and there are tools for that, e.g, weeder). The same about deep cabal-specific simplification of the design/algorithm/API//code.

Cabal is a big, complex system, both code and the dev activity, so I'd personally default to "if it ain't broke, don't fix it" or in other words, don't touch unless you have a very good reason (e.g,. a particular hlint hint really seems to you to clarify a code fragment a lot [edit: to the point that, e.g., you volunteer to rebase other people PR's to the end of time if conflicts arise --- in other words, you are willing to put your skin in the game]).

[ulysses4ever]

no hlint in CI

Only there is now :-) https://github.com/haskell/cabal/blob/c1659a3f43d57a3e15a32e248b444cbfd069ded0/.github/workflows/lint.yml

PR that removes dead code (after asking around if it's not used in a weird way in some test script) is 10 times more valuable (and there are tools for that, e.g, weeder).

We should open a ticket about Weeder (or find an old one). It looks like the repo still has remnants of a past Weeder setup even:

• https://github.com/haskell/cabal/blob/c1659a3f43d57a3e15a32e248b444cbfd069ded0/weeder.dhall
• https://github.com/haskell/cabal/blob/c1659a3f43d57a3e15a32e248b444cbfd069ded0/cabal.project.weeder

[Mikolaj]

no hlint in CI

Only there is now :-) https://github.com/haskell/cabal/blob/c1659a3f43d57a3e15a32e248b444cbfd069ded0/.github/workflows/lint.yml

Heh, not a binding one. Though it's one click (in Settings) away, I've just checked.

We should open a ticket about Weeder (or find an old one). It looks like the repo still has remnants of a past Weeder setup even:

Oh, cool. Yes, I think this would also not be controversial (except the initial deletion of any huge blobs reserved for whatever future lofty design).

[ulysses4ever]

they cost review time

Yes, this is my main objection to the whole enterprise, and I voiced it above. I'd prefer less PRs that batch several hlints if not one gigantic one to rip it out at once (just like what we did with reformatting).

make wading through patches while tracking a code change over time harder

Technically true, but it doesn't touch too many lines, so I doubt it will be very noticeable. Again, fewer commits (in fewer PRs) would be better: we could add big changes to .git-blame-ignore-revs at least.

[philderbeast]

We might get dead code detection with weeder #9121 but I found dead code with hlint.

Do I hold back (until the restraining order is lifted) or do you want to see it (as a pull request)?

[ulysses4ever]

I’d be curious to see it but I’d first ask more cautious of us, so, maybe @Mikolaj?..

[Mikolaj]

If it's dead code, it doesn't matter how it got detected. Great find, if so.