Warning: use Just: fromMaybe False -> (Just True ==)

[jrp2014]

When I run hlint on

  152 hasComments :: (Data a, Monad m) => Located a -> TransformT m Bool
  153 hasComments e = do
  154   anns <- getAnnsT
  155   let b = isCommentAnnotation <$> Map.lookup (mkAnnKey e) anns
  156   return $ fromMaybe False b

The suggestion is:

Found:
  fromMaybe False
Perhaps:
  (Just True ==)

Are the parens necessary?

[ndmitchell]

For the replacement HLint suggested, which is merely replacing fromMaybe False, the correct replacement is to use an == section, so (Just True ==) is the right replacement. It just so happens that in the bigger context the argument is applied immediately to a value, and could be further reduced - but that should be a separate hint. In a few cases HLint zooms out to look at the bigger context, but it's both fiddly to implement and not often that helpful, so it usually doesn't.

As it happens, HLint doesn't suggest anything for (Just True ==) b, which it should, so I'll add that as a hint.

[ndmitchell]

I've added detection of redundant sections, so closing this as expected (if not necessarily ideal) behaviour.

[steve-chavez]

@ndmitchell Really appreciate the work on hlint, the suggestions have always(I really mean always) made my code clearer.

However this particular recommendation always makes me scratch my head.

Here's a sample on the postgrest codebase: https://github.com/PostgREST/postgrest/blob/24064f86265b54701fa3c508099e211fb52b0887/src/PostgREST/Config.hs#L167

I find the fromMaybe much more straightforward to understand, also to write. Why is the (Just True ==) more correct?

[ndmitchell]

Usually this is part of a refactoring that takes you from fromMaybe True x to Just True == x - and I'd argue that the second of those is much more readable. And since we can make the hint apply more, might as well eta reduce it... However, after your comments, I agree that using the == in a section is a bit confused, so I'll eta-expand the hint and it will only apply with an additional argument - so not in the postgrest code. Does that make it feel like a more useful hint once more?

[steve-chavez]

To elaborate more, when I want:

• A fromMaybe True I have to use (Just False /=)(example here). I literally see the opposite value I want in the code.
• A fromMaybe False I have to use (Just True ==)(example here). Also the opposite value.

[steve-chavez]

@ndmitchell Thanks a lot for the prompt reply and fix!

Usually this is part of a refactoring that takes you from fromMaybe True x to Just True == x - and I'd argue that the second of those is much more readable

I totally agree.

so I'll eta-expand the hint and it will only apply with an additional argument - so not in the postgrest code. Does that make it feel like a more useful hint once more?

Yes! Thank you!

[ndmitchell]

And totally agreed about the negation semantics. That's why I find fromMaybe False x quite confusing, since you write False but then are secretly selecting on True. Of course, if you are filling in defaults higher order, then you get the flip around again. Thanks for the taking a look at this hint!

[codygman]

That's why I find fromMaybe False x quite confusing, since you write False but then are secretly selecting on True

Can you give an example of this?

[ndmitchell]

@codygman I think the classic is I have an optional value, x :: Maybe Bool, which defaults to False. A good way to "fill in the defaults" is to write:

x2 = fromMaybe False x

Replace Nothing with False, filling in defaults. But if instead I fill in the default with:

x2 = Just True == x

That seems rather confusing. I'm not filling in a default, I'm matching on a value. They happen to be equivalent, but the purpose is obscured.

Conversely, there are places with fromMaybe False is less clear - hence HLint lets people pick whichever matches their intended semantics.