Move Data.Functor.Contravariant from the contravariant package to base.
ClosedPublic

Authored by andrewthad on Feb 8 2018, 6:05 PM.

Details

Summary

Move Data.Functor.Contravariant from the contravariant package to base.
Since base is the bottom of the dependency hierarchy, several instances
have been removed. They will need to be added to the following packages:
transformers, StateVar, and possibly tagged. There may not actually have
been any types from tagged that previous had instanced provided by this
module though, since it may have only been used for Data.Proxy. Additionally,
all CPP has been removed. Derived Typeable instances have been removed
(since Typeable is now automatically derived for everything). The language
extension Safe is still used, although it is unclear to ATM whether or not
it is necessary.

This resolves trac issue Trac #14767.

Test Plan

validate

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.
andrewthad created this revision.Feb 8 2018, 6:05 PM

I don't think you staged this Diff off of the right commit, since the parent commit (34c0b560451f) doesn't exist anywhere in the GHC tree. Perhaps try this?

arc diff --update D4399 <first commit in the series>^
andrewthad updated this revision to Diff 15414.Feb 8 2018, 6:18 PM

Move Data.Functor.Contravariant from the contravariant package to base.

Summary:
Move Data.Functor.Contravariant from the contravariant package to base.
Since base is the bottom of the dependency hierarchy, several instances
have been removed. They will need to be added to the following packages:
transformers, StateVar, and possibly tagged. There may not actually have
been any types from tagged that previous had instanced provided by this
module though, since it may have only been used for Data.Proxy. Additionally,
all CPP has been removed. Derived Typeable instances have been removed
(since Typeable is now automatically derived for everything). The language
extension Safe is still used, although it is unclear to ATM whether or not
it is necessary.

This resolves trac issue Trac #14767.

Test Plan: validate

Reviewers: RyanGlScott, ekmett, hvr, bgamari

Subscribers: rwbarton, thomie, ekmett, carter, RyanGlScott

GHC Trac Issues: Trac #14767

Differential Revision: https://phabricator.haskell.org/D4399

andrewthad updated this revision to Diff 15415.Feb 8 2018, 6:19 PM

Update the differential to start at the first commit.

I think I finally got the differential right. Third time's a charm.

Thanks for doing this, @andrewthad! I've left some comments, some here, and some inline. Several of these comments apply equally this patch and the original source code in contravariant, so if you apply these suggestions, I promise to update contravariant accordingly :)

One important thing: make sure to update the appropriate accompanying documents. Definitely mention this in the base changelog, and also put a blurb about this under base library in the 8.6.1 release notes, since it's such a major addition to base.

libraries/base/Data/Functor/Contravariant.hs
11

Since we're moving this into base, change the maintainer to libraries@haskell.org.

18

Add @since 4.12.0.0 at the bottom of these module Haddocks.

121

We generally err on the side of not explicitly marking functions as INLINE in base, so I think we should adopt the same convention here. Remove this INLINE pragma.

123

Put a period at the end of this sentence.

126

Remove this INLINE pragma.

131

Remove this INLINE pragma.

134

Since we have liberty of using GHC extensions here, let's define this using GeneralizedNewtypeDeriving + StandaloneDeriving:

deriving instance Contravariant f => Contravariant (Alt f)

You'll have to change this module from Safe to Trustworthy to do this, but I think it's worth it.

137

Since we have the liberty of using GHC extensions here, let's define this using EmptyCase:

instance Contravariant V1 where
  contramap _ x = case x of
140

Let's change this instance so that it matches the laziness of the corresponding Functor instance. We currently have:

instance Functor U1 where
  fmap _ _ = U1

So let's also have:

instance Contravariant U1 where
  contramap _ _ = U1
142

Use GeneralizedNewtypeDeriving here.

145

Use GeneralizedNewtypeDeriving here.

148

Use GeneralizedNewtypeDeriving here.

156

Remove this INLINE pragma.

170

Use GeneralizedNewtypeDeriving here.

175

Remove this INLINE pragma.

178

Akin to my earlier comment about the U1 instance, let's change this instance to match the laziness of the Functor instance for Proxy. That is,

instance Contravariant Proxy where
  contramap _ _ = Proxy
192

This mappend definition is equivalent to the default implementation (mappend = (<>)), so there's no need to define it explicitly. Remove this definition.

194

Put a period at the end of this sentence.

206

Change this to Comparison p <> Comparison q = Comparison $ p <> q.

Alternatively, you could just as well use GeneralizedNewtypeDeriving and define this as deriving instance Semigroup (Comparison a). Use whatever you feel is best.

210

This mappend definition is equivalent to the default implementation (mappend = (<>)), so there's no need to define it explicitly. Remove this definition.

Alternative, you could sidestep this whole issue by using GeneralizedNewtypeDeriving and defining this instance as deriving instance Monoid (Comparison a). Use whatever you feel is best.

252

This mappend definition is equivalent to the default implementation (mappend = (<>)), so there's no need to define it explicitly. Remove this definition.

255

Put a period at the end of this sentence.

274

Use GeneralizedNewtypeDeriving here.

277

Use GeneralizedNewtypeDeriving here.

RyanGlScott requested changes to this revision.Feb 8 2018, 7:00 PM
This revision now requires changes to proceed.Feb 8 2018, 7:00 PM
RyanGlScott added inline comments.Feb 8 2018, 7:02 PM
libraries/base/Data/Functor/Contravariant.hs
170

Oops, I just realized that GeneralizedNewtypeDeriving won't work for Const a + Contravariant, so please ignore this comment.

andrewthad updated this revision to Diff 15416.Feb 8 2018, 7:33 PM

update manual, update changelog, use more GND and more laziness, update to fit into the post-Semigroup-Monoid-Proposal world

andrewthad marked 16 inline comments as done.Feb 8 2018, 7:42 PM

I've implemented most of requested changes. As I was checking them off, I realized there were several that I missed, so I'm going to push another differential addressing those.

libraries/base/Data/Functor/Contravariant.hs
145

Cannot be done for same reasons that it cannot be done for Const.

andrewthad marked 7 inline comments as done.Feb 8 2018, 7:47 PM

Checked off more changes.

libraries/base/Data/Functor/Contravariant.hs
148

Cannot be done for same reasons as Const.

andrewthad updated this revision to Diff 15417.Feb 8 2018, 7:48 PM
  • put a period at the end of the docs for defaultComparison
andrewthad marked an inline comment as done.Feb 8 2018, 7:49 PM

Everything's been done. Thanks for the thorough review Ryan!

andrewthad marked 2 inline comments as done.Feb 8 2018, 7:52 PM

Checked off more boxes.

This revision is now accepted and ready to land.Feb 8 2018, 7:55 PM
This revision was automatically updated to reflect the committed changes.