Revamp instances

[treeowl]

• Make all the Read, Show, Read1, Show1, Read2, and Show2 instances act like Backwards and Reverse.
• Add reading and showing instances for Yoneda and Coyoneda.
• Change some instances to simplify constraints. For example, change (Eq1 f, Eq a) to Eq (f a) when possible.

[treeowl]

If you don't like these Show instances, I can easily make everything show record syntax by changing just one file. If you don't think the constraint simplification is a good idea in all places, I'll pull it out where you ask.

[treeowl]

I'm somewhat on the fence about the changes to constraints in various instance contexts. As one example, there was previously a nice symmetry between WrappedBifunctor's Functor instance...

https://github.com/ekmett/bifunctors/blob/755eafccddd61d863eeea742456264a8364b5618/src/Data/Bifunctor/Wrapped.hs#L110

...and its Show1 instance...

https://github.com/ekmett/bifunctors/blob/755eafccddd61d863eeea742456264a8364b5618/src/Data/Bifunctor/Wrapped.hs#L77

...in the sense that both leveraged a class of kind (Type -> Type -> Type) -> Constraint to do the lifting. The new Show1 instance for WrappedBifunctor (which uses Show1 (p a) instead) still does the same thing in the end, I suppose.

Another thing I'm unsure about is that the Read{1,2}/Show{1,2} instances now use a different convention for instance contexts than the Eq{1,2}/Ord{1,2} instances do, which is somewhat unsatisfying. I suppose the Eq{1,2}/Ord{1,2} instances could also be made to use the same convention with some extra work. What are your thoughts?

I think you're right about WrappedBifunctor. Were there issues with other types?

[RyanGlScott]

Were there issues with other types?

The thing I'm having trouble with is coming up with a consistent set of criteria that determines whether an instance's context should use FlexibleContexts or not. In the current state of this PR, whether a given instance uses FlexibleContexts or not seems to vary quite a bit depending on which class one is looking at (e.g., Functor/Foldable/Traversable versus Read1/Show1 versus Eq1/Ord1). In the end, I don't really have a strong preference towards, say, (T2 p, T a) versus (T1 (p a)), but it would be nice if there were an easy way to know under what circumstances we pick one or the other.

[treeowl]

From what I can tell, the status quo is rather arbitrary as well. Could you point to a few of the inconsistencies you notice?

On Wed, Sep 8, 2021, 6:47 PM Ryan Scott @.***> wrote:

Were there issues with other types?

The thing I'm having trouble with is coming up with a consistent set of criteria that determines whether an instance's context should use FlexibleContexts or not. In the current state of this PR, whether a given instance uses FlexibleContexts or not seems to vary quite a bit depending on which class one is looking at (e.g., Functor/Foldable/Traversable versus Read1/Show1 versus Eq1/Ord1). In the end, I don't really have a strong preference towards, say, (T2 p, T a) versus (T1 (p a)), but it would be nice if there were an easy way to know under what circumstances we pick one or the other.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ekmett/bifunctors/pull/98#issuecomment-915623699, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOOF7NEAIFAYXV7CUTCRG3UA7RXNANCNFSM5DSA5ORA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[RyanGlScott]

Yes, a fair point. Now that I look more closely, the existing Read/Show instances tend to be using FlexibleContexts, while the Read{1,2}/Show{1,2} instances did not. What a mess.

In that case, I think we should decide on a convention to use and work towards that. I propose that we work towards eventually making all of the {Eq,Ord,Read,Show}{,1,2} instances use the flexible context style employed in this PR. (In this sense, this PR is a step towards that goal.) I'm somewhat arbitrarily leaving out Functor/Bifunctor/etc. instances from this convention since, as far as I can tell, one of this library's raisons d'être is to provide "transformers-like" instances for these classes, and the transformers library tends to use the non-flexible style. The {Eq,Ord,Read,Show}{,1,2} instances in this library are a pretty recent innovation, however, and I don't think we necessarily need to stick to the same style for that part of the library.

Do you agree with this approach? If not, I'm open to suggestions on how to improve it.

[treeowl]

I think FlexibleContexts generally give a better user experience. We mostly have the 1 and 2 classes to

1. Work around the lack of FlexibleContexts in Report Haskell.
2. Work around the lack of QuantifiedConstraints in Report Haskell when working in a polymorphic-recursive context.

The main oddball is WrappedBifunctor, whose primary purpose is to turn a Bifunctor into a Functor. What should the rest of its instances look like? I'm not really sure.

[treeowl]

The situation for WrappedBifunctor is so inconsistent already. Eq, Ord, Read, and Show are flexible. Eq1, Ord1, Read1, and Show1 use Eq2, Ord2, Read2, and Show2. If you tell me what you want there, I'll just do it.

[RyanGlScott]

I thought I had recalled other examples besides WrappedBifunctor that do things like instance (Bifunctor p, ...) => Functor (T p a), but now that I take a closer look, I'm mistaken. Things like Product, Sum, and Biap already define instances like instance Functor (p a) => Functor (T p a). So you're right, WrappedBifunctor really is the only exception.

In that case, I'm fine with the direction of travel in this PR. My apologies for misremembering the code in this library so many times, but I think this was a helpful discussion to have.

[treeowl]

I've gone to FlexibleContexts for a slew of Eq1 and Ord1 instances, and made a few more improvements, all in an (as yet) separate commit. I don't think it makes sense to make Functor and Applicative instances special exceptions to the usual approach. The only place I think is really special is WrappedBifunctor. We know what Functor instance we want that to have. Do we want all its instances to lean on the 2-versions, including Eq, Eq1, etc.? I dunno. Looks like a big judgement call.

[treeowl]

There is one other type that's a bit odd: Flip. Currently, we give it Functor, Eq1, etc., instances based on Bifunctor, Eq2, etc. ones. I think that's probably the right thing for a bifunctors package. That said, for a general-purpose flip, one might instead wish to either use special-purpose auxiliary classes for these, like

class Eq1Flip p c where
  liftEqFlip :: (a -> b -> Bool) -> p a c -> p b c -> Bool
instance Eq1Flip p c => Eq1 (Flip p c) where
  liftEq f (Flip pac) (Flip pbc) = liftEqFlip f pac pbc

or, alternatively, to instantiate Eq1 (Flip p c) separately for each bifunctor p.

[RyanGlScott]

Yes, I agree with your assessments about WrappedBifunctor and Flip. It may be worth leaving comments above the relevant instances explaining why they differ from their counterparts in other modules.

[treeowl]

@RyanGlScott, there's a lot here, so I can't guarantee I've hit everything squarely, but I think this is probably worth another review.

[treeowl]

Thanks!