bifunctors icon indicating copy to clipboard operation
bifunctors copied to clipboard

Fix the Bifoldable instance of Coyoneda

Open Topsii opened this issue 2 years ago • 3 comments

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 avatar Oct 17 '22 22:10 Topsii

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.

Topsii avatar Oct 22 '22 01:10 Topsii

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

?

RyanGlScott avatar Jun 09 '23 12:06 RyanGlScott

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.

ekmett avatar Jun 09 '23 15:06 ekmett