HLint rules to automatically reduce duplication across modules?

[saurabhnanda]

Is the following possible:

• We identify a commonly occurring code-pattern in our project, and add a function for it in some Utils module.
• HLint looks at all the functions in this Utils modules, and uses them to build rules for "Reduce duplication"

If not automatically, can we manually run some script and feed it the Utils module to generate custom hlint rules, every once in a while?

[ndmitchell]

See https://github.com/ndmitchell/hlint#adding-hints - the --find flag was exactly designed for what you are doing. I would say it's lightly tested and the documentation might be clearer, and I never advertised it - so feedback welcome.

[saurabhnanda]

Got it. Let me try this and report back.

[saurabhnanda]

I got the following suggestion via --find, which I promptly added to my config file...

- warn: {lhs: "a >>=\n  \\case\n      Left e -> throwM (ValidationErrors e)\n      Right r -> pure r", rhs: "withThrow a"}

...but it complained about LambdaCase not being enabled. So, I added this to the config file (LambdaCase is already enabled in the entire project via the cabal file):

- arguments: [-XArrows, -XQuasiQuotes, -XTypeApplications, -XLambdaCase]

Still, no luck. What am I doing wrong?

[saurabhnanda]

Another one, which doesn't seem to work (this is for Opaleye, btw, which uses Arrows):

- hint: {lhs: "(r ^. id) .== (constant a)", rhs: "findByPk a"}

Doesn't seem to catch the following code patterns:

 restrict -< (dep ^. id .== constant dpId)
-- or
restrict -< (r ^. id) .== (constant pid)
-- or
 restrict -< ((r ^. id) .== (constant bid)) .&& ((r ^. clientId) .== (constant cid))

[ndmitchell]

The first issue about LambdaCase was an issue with the .yaml file parser, now fixed in HEAD, and released soonish.

For the second, I added the hint exactly as you wrote it and then tried the file:

{-# LANGUAGE Arrows #-}

foo = proc x -> do
      restrict -< (dep ^. id .== constant dpId)
      -- or
      restrict -< (r ^. id) .== (constant pid)
      -- or
      restrict -< ((r ^. id) .== (constant bid)) .&& ((r ^. clientId) .== (constant cid))

And saw a suggestion to use findByPk at least once. Can you give me a complete snippet so I can be sure I'm seeing what you were expecting?

[saurabhnanda]

Can you give me a complete snippet so I can be sure I'm seeing what you were expecting?

HLint rule:

- hint: {lhs: "(r ^. id) .== (constant a)", rhs: "findByPk a"}

Code snippet/s that should've matched:

  (agents :: [Client.Client]) <- filterTable Client.tableForClient (\r -> (r ^. Client.id .== (constant $ booking ^. marketplaceId))) 

    filterFn ((((a, _), _), _), _) = a ^. Client.id .== (constant bookingId_)

client_ <- (filterTable tableForClient (\r -> r ^. Client.id .== constant clientId_)) >>= ensureSingle [qc|Cannot find a single client for {clientId_}|]

There are many in our project, which is why we've abstracted this out into a util function. Here's how I found these via regex:

\^\..*id.*\.==.*constant

[ndmitchell]

Given a sample file:

main = do
  (agents :: [Client.Client]) <- filterTable Client.tableForClient (\r -> (r ^. Client.id .== (constant $ booking ^. marketplaceId))) 
  client_ <- (filterTable tableForClient (\r -> r ^. Client.id .== constant clientId_)) >>= ensureSingle undefined
  return ()

filterFn ((((a, _), _), _), _) = a ^. Client.id .== (constant bookingId_)

Note that I replaced [qc|Cannot find a single client for {clientId_}|] with undefined as that was giving a parse error (might require an extension turning on, might be a haskell-src-exts bug, not really sure). With that, I get:

$ hlint Sample.hs
Sample.hs:3:69: Suggestion: Redundant bracket
Found:
  \ r -> (r ^. Client.id .== (constant $ booking ^. marketplaceId))
Why not:
  \ r -> r ^. Client.id .== (constant $ booking ^. marketplaceId)

Sample.hs:3:76: Suggestion: Move brackets to avoid $
Found:
  r ^. Client.id .== (constant $ booking ^. marketplaceId)
Why not:
  r ^. Client.id .== constant (booking ^. marketplaceId)

Sample.hs:4:14: Suggestion: Redundant bracket
Found:
  (filterTable tableForClient
     (\ r -> r ^. Client.id .== constant clientId_))
    >>= ensureSingle undefined
Why not:
  filterTable tableForClient
    (\ r -> r ^. Client.id .== constant clientId_)
    >>= ensureSingle undefined

Sample.hs:7:34: Suggestion: Redundant bracket
Found:
  a ^. Client.id .== (constant bookingId_)
Why not:
  a ^. Client.id .== constant bookingId_

4 hints

If I follow the bracket suggestions, and also rewrite your rule to:

- hint: {lhs: "r ^. Client.id .== constant a", rhs: "findByPk a"}

Then I get 3 suggestions to use findByPk. If you always import Client qualified as Client then I suggest you take that route. If not, you can define a package, say what it imports, and tap into more refined name matching (see the hlint.yaml that ships with HLint for some examples of defining packages and declaring which packages a hint imports).