Warn when functions are unnecessarily monadic

[kostmo]

Closes #832

[shayne-fletcher]

very cool. lgtm.

[kostmo]

Interestingly, this new hint found two true positives in the hlint codebase, one of which I chose to fix (eb70b4e) and the other to ignore (d1c50ed).

[googleson78]

How does this do with something that isn't monadic but uses do?

e.g. these examples

{-# LANGUAGE BlockArguments #-}
x = 
  (+)
    do 3 + 5
    do 4 * 7
    
y = do
  let z = 5
  z

f bla do
  g x y z

I would really like to get warnings on none of these, personally, because they are quite useful in specific cases.

[googleson78]

That's awesome, thanks!

[kostmo]

How does this do with something that isn't monadic but uses do?

I am pleased to report that none of these examples emit a warning. I have added them as test cases (4b8904e). The rule is pretty conservative, and only triggers when pure or return are present.

[shayne-fletcher]

cc @ndmitchell

[shayne-fletcher]

on

f x = do
  pure x

we get

/L.hs:(4,1)-(5,8): Suggestion: Unnecessarily monadic
Found:
  f x = do pure x
Perhaps:
  Demote `f` to a pure function

the ideal i guess would be

/L.hs:(4,1)-(5,8): Suggestion: Unnecessarily monadic
Found:
  f x = do pure x
Perhaps:
  f x = x

not a blocker i would suppose but is that within reach?

[kostmo]

not a blocker i would suppose but is that within reach?

The type signature of f would also have to change.

[shayne-fletcher]

let's land this. we can always work in-situ if neil comes back with suggestions later.

[ndmitchell]

Thanks for the patch, and thanks Shayne for merging. I didn't notice this, and have unfortunately not been paying attention for a bit too long 😞 . Afraid I don't think this can work as an HLint hint. Firstly, there is a bug, in that if you have:

foo [] = pure 1
foo (_:_) = print 1

This reports that foo is unnecessarily monadic. But it's not - you have to consider all equations at once.

The other issue is that there isn't a good "fix" for this, as doing so necessarily changes the signature, and thus you have to update all callers. No other HLint hints have that property, and I'm not sure if it's something we want to start or not.

I guess we could have it as an off by default hint, but it still does things that HLint otherwise doesn't ever do, so I'm not sure it fits. Sorry for the effort you two invested in this.