Add Semigroup and Monoid instances for GenT that lift the inner Monoid

[andrewthad]

This uses the technique suggested in Equational Reasoning at Scale to provide a Monoid instance for GenT. This was motivated by more than just theoretical concerns. At work, I was recently in a situation where I needed to flatten a list of generators like this:

flatten :: [Gen ByteString] -> Gen ByteString

With the instances provided by this PR, flatten is just mconcat. I've written these instances so that they should be compatible with all phases Semigroup-Monoid-Proposal that are represented by GHC 8.4 and lower.

[andrewthad]

cc @chessai

[chessai]

I needed this at work the other day, in the situation @andrewthad described.

Also, thanks for doing this, @andrewthad. I meant to do so when I got over the flu.

[andrewthad]

No problem. I figured that when you're running a giant fever, there are probably more pressing things on your mind.

[moodmosaic]

Nitpick: technically should start with an upper-case T, so it looks sane in Haddock docs on Hackage.

[andrewthad]

This comment does not currently show up in haddock docs, but I can make it show up there since that might actually be mildly useful information for users of the library.

[thumphries]

Can just leave this unannotated after adopting George's feedback

[gwils]

I would prefer that you weaken the constraint to Semigroup a and delete the comment.

this would be annoying for older GHCs

Not if the user depends on the semigroups package, as I've written about. It provides Semigroup compatibility at least as far back as GHC 7.2.

On the flip-side, the current approach would be unhelpful to any user whose type is a Semigroup but not a Monoid.

I have made this change on my fork here if you'd like to use that patch.

[thumphries]

Yeah, George's version looks right to me

[andrewthad]

Good call. I had originally thought this wouldn't work with pre-SMP base, but it totally does. This is certainly the best option. I merged it into the PR.

[thumphries]

Thanks for the PR!

This seems like the most sensible monoid for GenT. Anyone object?

[andrewthad]

I've merged @gwils changes into the PR.

[gwils]

I've just had a look at the definition of GenT and I'm wondering if we want to give Tree a similar liftA2-ish Semigroup and Monoid instance. Then on GenT we could GeneralizedNewtypeDeriving our way to Semigroup and Monoid.

I'm not sure whether that's a reasonable thing to have on Tree though. Is there a more suitable Monoid for it?

Don't let this concern hold up the PR, but I thought I'd mention it.

[andrewthad]

I'm not sure whether that's a reasonable thing to have on Tree though. Is there a more suitable Monoid for it?

I would argue that this is always the most appropriate Monoid instance for any Applicative type. Various discussion around this topic:

• https://mail.haskell.org/pipermail/libraries/2017-November/028312.html
• https://mail.haskell.org/pipermail/libraries/2014-November/024310.html
• https://www.reddit.com/r/haskell/comments/4r5bcj/listtransformer_a_beginnerfriendly_listt/d4z0i55/

But yeah, I agree that it would be better to discuss that on a separate PR.

[thumphries]

I'm not sure whether that's a reasonable thing to have on Tree though.

Tree is internal, so unless there's an internal motivation it's not important either way. This change looks fine as is.

I would argue that this is always the most appropriate Monoid instance for any Applicative type.

Agree, the Alternative-based instance doesn't seem too compelling.

Will wait for @jystic to weigh in before merging, but LGTM.

Thanks for the contribution! Hope you're getting value out of Hedgehog.

[andrewthad]

Does @jystic have any additional thoughts on this?

[thumphries]

Thanks again for the PR!

[chessai]

@thumphries thank you