Good morning all,

I have now split Richard's commit in two, so that it's explicit in the commit history that we are adding those instances:

https://gitlab.haskell.org/ghc/ghc/-/merge_requests/5509/diffs?commit_id=2cbad0e7ced9de896eb9a1732631786a6adb676a

Ben: I have briefly tried to add the Hlint annotations, but it turned out to be trickier than anticipated. I couldn't find any easy way in HLint to add a custom hint on a *typeclass instance*, and adding something like this into `compiler/.hlint.yaml` didn't change anything:

- modules:
  - {name: GHC.Data.Bag, badidents: [mempty], message: "Use emptyBag as it's more descriptive"}
  - {name: GHC.Data.Bag, badidents: [(<>)], message: "Use unionBags as it's more descriptive"}

(I suspect that's not what's the badidents syntax is for). I have also tried to add two `ANN` to the definitions, like this:

{-# ANN (<>) ("HLint: use unionBags as it's more descriptive" :: String) #-}
instance Semigroup (Bag a) where
  (<>) = unionBags

However, this won't work as (<>) is imported qualified and there is no way in the ANN syntax to specify a fully qualified identifier. However, I suspect this is not the correct syntax either, as `(<>)` is really the *typeclass* method, but we want to target the particular instance implementation.

Having said that, Richard carefully defined `(<>) = unionBags` and `mempty = emptyBag`, so I agree that using `(<>)` and `mempty` would be more opaque but at the end of the day it should have the asymptotic complexity than using `unionBags` and `emptyBag` (modulo dictionary passing, but I hope that won't each our lunch). 

A.


On Thu, 15 Apr 2021 at 02:20, Ben Gamari <ben@smart-cactus.org> wrote:
Richard Eisenberg <rae@richarde.dev> writes:

> Hi devs,
>
> In the work on simplifying the error-message infrastructure (heavy lifting by Alfredo, in cc), I've been tempted (twice!) to add
>
>> instance Semigroup (Bag a) where
>>   (<>) = unionBags
>>
>> instance Monoid (Bag a) where
>>   mempty = emptyBag
>
> to GHC.Data.Bag.
>
> The downside to writing these is that users might be tempted to write
> e.g. mempty instead of emptyBag, while the latter gives more
> information to readers and induces less manual type inference (to a
> human reader). The upside is that it means Bags work well with
> Monoid-oriented functions, like foldMap.
>
> I favor adding them, and slipped them into !5509 (a big commit with
> lots of other stuff). Alfredo rightly wondered whether this decision
> deserved more scrutiny, and so I'm asking the question here.
>
My sense is that adding the instances is a Good Thing. However, I do
think that we probably ought to refrain from using (<>) and mempty where
more specific functions would do. Adding a lint would be one way to
accomplish this. Hiding the functions from GhcPrelude would be another.

Cheers,

- Ben


_______________________________________________
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs