Add Applicative, Semigroup, and Monoid instances in GHC.Generics
ClosedPublic

Authored by lysxia on Feb 24 2018, 11:01 PM.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
lysxia created this revision.Feb 24 2018, 11:01 PM

Just for the record, this has been discussed on the libraries@ mailing list here. The patch looks good to me.

RyanGlScott requested changes to this revision.Feb 25 2018, 8:14 AM
RyanGlScott added a subscriber: RyanGlScott.

Thanks, @lysxia! This mostly looks good—I've left some suggestions.

  • Change all of the @since 4.11.0.0 annotations to @since 4.12.0.0, since the feature window for base-4.11.0.0 has already passed.
  • Please add an entry to libraries/base/changelog.md about this.
libraries/base/GHC/Generics.hs
896

Note that the Applicative instance for Const is actually slightly different from this:

instance Monoid m => Applicative (Const m) where
    pure _ = Const mempty
    liftA2 _ (Const x) (Const y) = Const (x `mappend` y)
    (<*>) = coerce (mappend :: m -> m -> m)
-- This is pretty much the same as
-- Const f <*> Const v = Const (f `mappend` v)
-- but guarantees that mappend for Const a b will have the same arity
-- as the one for a; it won't create a closure to raise the arity
-- to 2.

It would be good to match this in the Applicative instance for K1.

This revision now requires changes to proceed.Feb 25 2018, 8:14 AM
lysxia updated this revision to Diff 15552.Feb 25 2018, 9:49 AM
  • Fixed arity of (<*>) for K1
  • Updated @since annotations to 4.12.0.0
  • Updated changelog
RyanGlScott added inline comments.Feb 25 2018, 9:55 AM
libraries/base/GHC/Generics.hs
896

Don't forget to implement liftA2!

lysxia added inline comments.Feb 25 2018, 10:12 AM
libraries/base/GHC/Generics.hs
896

Wouldn't it be better for Const and K1 to define liftA2 = coerce (const mappend) as opposed to the current implementation for Const?

RyanGlScott added inline comments.Feb 25 2018, 10:13 AM
libraries/base/GHC/Generics.hs
896

That would be fine too—I just want to ensure that they're consistent!

lysxia updated this revision to Diff 15554.Feb 25 2018, 1:45 PM

Implement liftA2 for K1

lysxia added inline comments.Feb 25 2018, 3:05 PM
libraries/base/GHC/Generics.hs
896

Ok! I can fix Const when I get to update instances in Data.Functor.(...)

Can I also add Eq1, Ord1, Read1, Show1 instances here, or should that go in another diff?

RyanGlScott accepted this revision.Feb 25 2018, 5:04 PM

LGTM.

Can I also add Eq1, Ord1, Read1, Show1 instances here, or should that go in another diff?

Well, given that the libraries mailing list discussion never mentioned any of those classes, I'd definitely say you should pursue those in another Diff (after discussing them on the libraries mailing list, of course, to make sure folks are OK with them.)

This revision is now accepted and ready to land.Feb 25 2018, 5:04 PM
This revision was automatically updated to reflect the committed changes.