ekmett / bifunctors

Haskell 98 bifunctors, bifoldables and bitraversables
Other
57 stars 42 forks source link

Fix the Bifoldable instance of Coyoneda #107

Open Topsii opened 1 year ago

Topsii commented 1 year ago

Coyoneda's Foldable instance currently requires Bifunctor in its context. The plan is for Foldable to become a superclass of Bifoldable, see haskell/core-libraries-committee#93. If this happens, then Coyoneda's Bifoldable instance will require Bifunctor, because it is a requirement of its superclass Foldable.

Topsii commented 1 year ago

Currently the Foldable and Traversable instances of Coyoneda are implemented using bimap. The use of bimap requires the context Bifunctor p in Coyoneda p's Bifoldable (and Foldable) instance. It is possible to implement Foldable and Traversable without bimap as follows:

instance (forall x. Foldable (p x)) => Foldable (Coyoneda p a) where
  foldMap = \f (Coyoneda _ yb pxy) -> foldMap (f . yb) pxy

instance (forall x. Traversable (p x)) => Traversable (Coyoneda p a) where
  traverse = \f (Coyoneda xa yb pxy) -> Coyoneda xa id <$> traverse (f . yb) pxy

To me this appears to match their Bifoldable/Bitraversable instances more closely. Note that the Bifoldable instance does not require Bifunctor p in its context in this case.

instance Bifoldable p => Bifoldable (Coyoneda p) where
  bifoldMap = \f g (Coyoneda h i p) -> bifoldMap (f . h) (g . i) p

Unfortunately the use of quantified constraints makes the instance contexts more restrictive than they were before.

RyanGlScott commented 1 year ago

Sorry for not noticing this earlier!

@ekmett, do you have an opinion on whether to:

  1. Update the Bifoldable instance to instance (Bifoldable p, Bifunctor p) => Bifoldable (Coyoneda p), as is done in this patch, or
  2. Changing the Foldable instance to instance (forall x. Foldable (p x)) => Foldable (Coyoneda p a), which would avoid needing to change the Bifoldable instance's context

?

ekmett commented 1 year ago

If we can get Bifunctor, etc. changed in base to have quantified superclass constraints, then I'd be all about getting these instances changed to use quantified constraints, as it'd be almost a pure win.