ndmitchell / hlint

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

Bad suggestion in the presence of MonadComprehensions #1261

Open hseg opened 3 years ago

hseg commented 3 years ago

[x | True] = return x in the presence of MonadComprehensions, but hlint incorrectly suggests replacing it with [x] instead. Using a trivial guard here for homogeneity in a long list of clauses like

fromMaybe [] . sequence $
   [ [f x y | True]
   , [g x r | r <- k]
   , [h x | True]
   , [j r s | r <- l, s <- m]
   ]

cf #775

ndmitchell commented 3 years ago

Looking at that code it seems like GHC knows whether it is a monad comp or not, so just a case of checking for that case. We should also check that monad comprehensions are not enabled by default. Patch welcome!

shayne-fletcher commented 3 years ago

Monad comprehensions are enabled by default.

f :: Maybe Int
f = [ x + y | x <- Just 1, y <- Just 2 ]

main = print f

A quick play with this program suggests parsing succeeds regardless of whether the extension is in effect or not. The extension is required to be enabled for the program to type however.

I expect therefore disabling the extension by default won't have an effect, good or bad.

ndmitchell commented 3 years ago

I think the GHC syntax tree is different if monad comprehensions are enabled, so we should probably disable monad comps, then if they are enabled and reflected in the syntax tree it is probably because a user enabled them explicitly, and then we should not apply the [x | True] modification.

shayne-fletcher commented 3 years ago

The parser list rule (for comprehensions)

        | texp '|' flattenedpquals

inspects the parser's MonadComprehensionBit in order to calculate correctly the "statement context" (list or monad) via

checkMonadComp = do
    monadComprehensions <- getBit MonadComprehensionsBit
    return $ if monadComprehensions
                then MonadComp
                else ListComp

So yes, you are right, the parse tree is different when the rule is enabled from when it's not.

I will offer a PR that stops MonadComprehensions from being enabled by default.

hseg commented 2 months ago

Was going through my old open tickets, this is still broken on Hlint 3.8:

{-# LANGUAGE MonadComprehensions #-}

f x = [x | True]

g = fromMaybe [] . sequence $
   [ [f x y | True]
   , [g x r | r <- k]
   , [h x | True]
   , [j r s | r <- l, s <- m]
   ]

emits (regardless of MonadComprehensions):

|| Test.hs:1:7-16: Suggestion: Redundant True guards
|| Found:
||   [x | True]
|| Perhaps:
||   [x]
|| 
|| Test.hs:4:6-19: Suggestion: Redundant True guards
|| Found:
||   [f x y | True]
|| Perhaps:
||   [f x y]
|| 
|| Test.hs:6:6-17: Suggestion: Redundant True guards
|| Found:
||   [h x | True]
|| Perhaps:
||   [h x]
|| 
|| 3 hints