ndmitchell / hlint

Haskell source code suggestions
Other
1.47k stars 196 forks source link

"Redundant <$>" in case with lambda case #1190

Open complyue opened 3 years ago

complyue commented 3 years ago

Regarding this hint:

Redundant <$>
Found:
  fifoDeque <$> swapTVar backlog fifoEmpty
  >>=
    \case
      (Nothing, _) -> finallyDone
      !more -> go more
Why not:
  swapTVar backlog fifoEmpty
  >>=
    (\case
       (Nothing, _) -> finallyDone
       !more -> go more)
      . fifoDeque
hlint(refact:Redundant <$>)

I'm against the suggestion as I feel <$> working with the lambda case reads more pleasing (especially when the branches of the cases go rather lengthy):

(as formatted by Ormolu FYI)

chkQue :: STM ()
chkQue =
  fifoDeque <$> swapTVar backlog fifoEmpty >>= \case
    (Nothing, _) -> finallyDone
    !more -> go more

I'm not sure if it is produced due to

warn: {lhs: x Data.Functor.<&> f >>= g, rhs: x >>= g . f}

from #949 ?

And would you agree to silent it in presence of lambda cases?

I'm using {- HLINT ignore "Redundant <$>" -} to disable it for now, but think it's desirable for cases without lambda case.

ndmitchell commented 3 years ago

Agreed that this not firing for lambda case makes sense. I wonder if it's more general - perhaps anywhere that ends up introducing brackets around either g or f?

josephcsible commented 3 years ago

Eliminating redundant fmaps is a performance win for some Functors, so that hint isn't just supposed to be for readability. Is there some alternative way we could rewrite it, rather than removing the hint in that case altogether?

complyue commented 3 years ago

Or the suggestion may be sth like this?

chkQue :: STM ()
chkQue =
  (swapTVar backlog fifoEmpty >>=) $
    (. fifoDeque) $ \case
      (Nothing, _) -> finallyDone
      !more -> go more

I personally like this form to get rid of fmap.