Add hlint configuration & gh workflow

[philderbeast]

Related to #589, adds an .hlint.yaml configuration with counts of the suggestions that would be triggered were they not ignored. This way we could progressively tackle one or more individual HLint suggestions at a time.

Also adds a GitHub action workflow to check the linting.

[quark17]

Thanks!

Currently all of our jobs are under one workflow, ci.yml. Do you have thoughts on when it makes sense to put jobs into their own workflow file? There are already a couple whitespace checks for source code, so I would at least group the linting with those -- either put the linting with them or move them into the file with the linting.

As an aside, there are a number of warnings in GHC 9.8 (use of partial functions like head) that we will also want to address. See #469.

Also, FYI, when I last looked at the hlint output, I recall some of them being unhelpful (possibly contrary to my own opinions on style, such as valuing reader clarity over redundancy), so maybe we'll want to use a non-default configuration? But certainly keeping track of the output is good. Instead of just the counts, though, would it be helpful to store the entire output and compute a diff, that way someone who makes changes to the source can see specifically what new message they introduced?

[philderbeast]

Do you have thoughts on when it makes sense to put jobs into their own workflow file? There are already a couple whitespace checks for source code, so I would at least group the linting with those -- either put the linting with them or move them into the file with the linting.

Yes I do. If a workflow step is much faster than other steps, I like seeing it in its own workflow. Taking the haskell/cabal actions for instance, rough timings for some of its workflows are;

• checking whitespace takes 10s,
• checking for typos takes 10s,
• checking formatting takes 40s,
• linting with hlint takes 1m30s,
• validation takes 1h30m.

Having a separate smaller workflow can make finding the error much easier if inspecting the logs online through the web interface of GitHub. I like how with haskell/cabal, there's usually a Makefile rule that corresponds to a CI workflow that I can run that will show me the failure locally.

$ make help
...
lint                           Run HLint.
...
style                          Run the code styler.
...
whitespace                     Run fix-whitespace in check mode.

[philderbeast]

Instead of just the counts, though, would it be helpful to store the entire output and compute a diff, that way someone who makes changes to the source can see specifically what new message they introduced?

That's a very interesting suggestion and would be a way to get feedback on ignored HLint suggestions. I fear it would be fragile because HLint output includes line numbers.

$ hlint -j .
src/comp/SystemVerilogScanner.lhs:493:22-41: Warning: Use isDigit
Found:
  d >= '0' && d <= '9'
Perhaps:
  isDigit d

1 hint

[philderbeast]

As an aside, there are a number of warnings in GHC 9.8 (use of partial functions like head) that we will also want to address. See #469.

I'd always prioritize doing fixes for compiler warnings first before fixing linting warnings and suggestions. That said, I find HLint very helpful. It usually suggests a change to make and I've learnt about Haskell, the language, that way.

[philderbeast]

Also, FYI, when I last looked at the hlint output, I recall some of them being unhelpful (possibly contrary to my own opinions on style, such as valuing reader clarity over redundancy), so maybe we'll want to use a non-default configuration?

I usually accept HLints suggestions and go with the flow but contrarian opinions can be accommodated with HLint's various ways of ignoring suggestions, either within the .hs source or within .hlint.yaml. It also allows for granular exclusions, such as this example:

- ignore: {name: Eta reduce, within: [MyModule1, MyModule2]}

[quark17]

Thank you again for this. In talking with others about it, there is agreement that the Makefile in src/comp/ should have a target (or targets) for running HLint. It was also suggested that we can incrementally respond to HLint recommendations by silencing all the checks that we can't currently deal with, and then work our way through by turning them back on incrementally as we are able to address them -- that way, the CI can run HLint and error if any checks fail (rather than comparing against expected counts of messages, which is fragile and requires updating).

I would also suggest putting the HLint configuration file in src/comp/ (or at least under src/ somewhere) and not using a dotted name (.hlint.yml). I'd suggest, say, src/comp/hlint.yml. This would require a flag to Hlint to tell it the file name, but I would prefer not having hidden files, or implicitly included files that might confuse users. Also, it would allow us to have multiple targets -- one for the CI that silences everything and one for running HLint without any silencing, say, in case someone wants to see everything. We should also update src/comp/Makefile so that running make with no target will print a message listing the various targets (just as the top-level makefile does) rather than jumping into compilation. I'm happy to set that up, if you'd prefer.

Also, the whitespace and testsuite linting checks could be moved of the GitHub workflows directory and into appropriate places -- again, I can do that if you prefer. I'd think that testsuite/Makefile should be updated to print a usage message when called with no target, and it could have a new lint target that calls the two scripts, that could be put in the testsuite/scripts/ directory. For the whitespace checks, I guess that'd be a new target in the toplevel makefile, perhaps lint as well. Maybe it also calls the lint target in the testsuite. And I guess we could create a directory for the scripts, maybe util/lint/ -- or, since it's not a util for users, maybe a new directory for repo tools or linting? (There's also a "allow_whitespace.pats" file that would need to go somewhere, so maybe a repo tools/linting directory is good.) Anyway, happy to make that separate from this PR.

On this topic:

Having a separate smaller workflow can make finding the error much easier if inspecting the logs online through the web interface of GitHub.

I find the exact opposite. Admittedly, I only have a color impairment, not other vision impairment, so I can't comment on how navigable the pages are for people who use screen readers and the like. However, here's my experience: For separate jobs within one workflow (I'm not talking about steps within a job), the status is immediately available and visible. See, for example, the whitespace checks, that appear on the web interface with a red X within 5 seconds if they fail. Yes, you don't get an email notification until the entire workflow is done -- I agree there, that separate workflows mean you get an email quicker. However, separate workflows make the web navigation harder: There is no longer one webpage containing all the checks. Each workflow shows up separately in the list of Actions (on the desktop browser pages) -- for one PR or commit, there will be multiple entries, and you just have to know how many entries to go look for. (On the mobile app, it's even worse; there are separate top-level tabs for each workflow, including tabs for any that ever existed in the history of the repo, even just inside a PR; and you have to click on each tab and then click on the particular commit/PR within that, to get to each report.) I'd much rather have one webpage/tab with all the jobs in one place; the status of every job is immediately visible, you just don't get a quicker email notification.

[philderbeast]

It was also suggested that we can incrementally respond to HLint recommendations by silencing all the checks that we can't currently deal with, and then work our way through by turning them back on incrementally as we are able to address them -- that way, the CI can run HLint and error if any checks fail (rather than comparing against expected counts of messages, which is fragile and requires updating).

That's pretty much the incremental workflow I use when tackling HLint linting.

If the codebase has changed, I'll delete all of the "Warnings currently triggered by your code", rerun hlint --default . > .hlint-cmp.yaml (cmp for compare but I discard this file). This command generates new ignores. Copying those warnings back into .hlint.yaml shows what has changed in the meantime. I'll commit this change on the working branch to set the new baseline showing the current linting problems (suggestions) with their # count hints comments.

# Warnings currently triggered by your code
- ignore: {name: "Avoid lambda"} # 38 hints
- ignore: {name: "Avoid lambda using `infix`"} # 15 hints
...
- ignore: {name: "Use zipWithM"} # 1 hint
- ignore: {name: "Used otherwise as a pattern"} # 28 hints

I'll then pick an individual ignore line, delete it, run hlint -j . to show me the linting errors and then, if possible, use hlint --refactor --refactor-options=--inplace <filename.hs> on each module <filename.hs> until HLint finds no hints. The refactoring of apply-refact has some problems so I'm prepared to either fix up its messes by hand or make the HLint suggested change by hand.

The whole process is pretty lightweight and quick. I feel like it is easier on reviewers to only tackle one suggested hint at a time, or a few together that are similar. Even then, I like keep commits for each hint separate. Those commits, with the initial baseline counts commit (and, if we're being thorough, a final counts commit), can be squashed after review and before merging.

[philderbeast]

@quark17, when the hlint-run action has failures, this is how they're shown¹:

Clicking on the failing hlint run and expanding the failing item shows:

Footnotes

1. Links to source location don't actually work in GitHub's web page user interface. ↩

[philderbeast]

I would also suggest putting the HLint configuration file in src/comp/ (or at least under src/ somewhere) and not using a dotted name (.hlint.yml).

I've changed the configuration to the non-standard name you suggest, hlint.yaml. For the moment, I've left this file in the root. I was able to configure the hlint-run GitHub action to use this configuration with this workaround:

FWIW, I got around this issue by adding them through hlint-bin, e.g., hlint-bin: hlint --hint=code/.hlint.yaml.
-- haskell-actions/hlint-run#20 (comment).

[philderbeast]

I would also suggest putting the HLint configuration file in src/comp/.

@quark17, would it be good to run HLint on *.hs files outside of src/comp/?

$ find . -type f -name '*.hs'
./src/Parsec/ParsecLanguage.hs
./src/Parsec/ParsecExpr.hs
./src/Parsec/ParsecToken.hs
./src/Parsec/ParsecChar.hs
./src/Parsec/ParsecPerm.hs
./src/Parsec/Parsec.hs
./src/Parsec/ParsecPrim.hs
./src/Parsec/ParsecCombinator.hs
./src/comp/...
...
./src/vendor/stp/HaskellIfc/STP.hs
./src/vendor/stp/HaskellIfc/STPFFI.hs
./src/vendor/yices/v2.6/HaskellIfc/YicesFFI.hs
./src/vendor/yices/v2.6/HaskellIfc/Yices.hs
./src/vendor/htcl/HTcl.hs
./testsuite/bsc.syntax/bsv05/strings/MakeOctalCharsAsInteger.hs
./testsuite/bsc.syntax/bsv05/strings/MakeOctalChars.hs
./testsuite/bsc.preprocessor/ifdef/iftestcase.ll.hs

As well as ignoring linting rules in specific modules, HLint can be configured to ignore path globs, such as haskell/cabal/.hilint.yaml does with its ignore-glob arguments.