Proposal: Relax instances for Functor combinators; put superclasses on <class>1 to make less-breaking

[Ericson2314]

Implementation https://gitlab.haskell.org/ghc/ghc/-/merge_requests/4727

The first change makes the Eq, Ord, Show, and Read instances for Sum, Product, and Compose match those for :+:, :*:, and :.:. These have the proper flexible contexts that are exactly what the instance needs:

For example, instead of

instance (Eq1 f, Eq1 g, Eq a) => Eq (Compose f g a) where
  (==) = eq1

we do

deriving instance Eq (f (g a)) => Eq (Compose f g a)

But, that change alone is rather breaking, because until now Eq (f a) and Eq1 f (and respectively the other classes and their *1 equivalents too) are incomparable constraints. This has always been an annoyance of working with the *1 classes, and now it would rear it's head one last time as an pesky migration.

Instead, we give the *1 classes superclasses, like so:

(forall a. Eq a => Eq (f a)) => Eq1 f

along with some laws that canonicity is preserved, like:

liftEq (==) = (==)

and likewise for *2 classes

(forall a. Eq a => Eq1 (f a)) => Eq2 f

along with some laws that canonicity is preserved, like:

liftEq2 (==) = liftEq1

The *1 classes also have default methods using the *2 classes where possible.

What this means, as explained in the docs in my implementation, is that *1 classes really are generations of the regular classes, indicating that the methods can be split into a canonical lifting combined with a canonical inner, with the super class "witnessing" the laws[1] in a fashion.

Circling back to the pragmatics of migrating, note that the superclass means evidence for the old Sum, Product, and Compose instances is (more than) sufficient, so breakage is less likely --- as long no instances are "missing", existing polymorphic code will continue to work.

Breakage can occur when a datatype implements the *1 class but not the corresponding regular class, but this is almost certainly an oversight. For example, containers made that mistake for Tree and Ord, which I fixed in haskell/containers#761, but fixing the issue by adding Ord1 was extremely uncontroversial.

---

[1]: In fact, someday, when the laws are part of the language and not
only documentation, we might be able to drop the superclass field of the
dictionary by using the laws to recover the superclass in an
instance-agnostic manner, e.g. with a non-overloaded function with
type:

DictEq1 f -> DictEq a -> DictEq (f a)

But I don't wish to get into optomizations now, just demonstrate the
close relationship between the law and the superclass.

[Ericson2314]

When I rebased this, I found that Generically1 was missing Eq, Ord, Show, and Read instances, even though it had the *1 versions. Classic!

[Ericson2314]

haskellari/some#21 This is an example of an downstream library improvement that is blocked on this change.

GShow f = forall a. Show (f a) for all intents and purposes, but we cannot make a superclass to indicate (half of) that because the GShow instances for Sum and Product conflict with the overly-conservative Show1-based instances we have today, instead of the strictly more general instances proposed here.

[gwils]

The canonicity law looks good to me -- I'm actually surprised that Eq1 and co didn't already have something like this. The quantified constraint superclass also looks fine and sensible to me.
If we're going to use FlexibleContexts-style Eq (f a) constraints around the place after this change, what reason do Eq1 and co still have for existing? I thought their main purpose was to avoid such FlexibleContexts-style code.

Do we know how much of hackage this breaks? It looks like there was some effort to run this against head.hackage but nobody understood the outcome?

[Ericson2314]

If we're going to use FlexibleContexts-style Eq (f a) constraints around the place after this change, what reason do Eq1 and co still have for existing? I thought their main purpose was to avoid such FlexibleContexts-style code.

Besides back-compat (just getting rid of the classes altogether would be far more breaking), the classes are still meaningful if one wants to lift a non-canonical function on the inside. I am not sure how often that is done, but it's theoretically useful enough that I don't feel inspired to rid of this.

Do we know how much of hackage this breaks? It looks like there was some effort to run this against head.hackage but nobody understood the outcome?

I will try running it again. I think it was just broken then, but might work now.

[Bodigrim]

Eq1 class is still meaningful, because it allows to check heterogeneous equality like "does f a matches f b?".

[gwils]

Thanks, I now agree that Eq1 and friends still have reason to exist.
@Ericson2314 did the GHC CI help you with an impact analysis?

[Bodigrim]

along with some laws that canonicity is preserved

Are these laws unconditional or are they expecting that an underlying (==) is lawful? (I'm thinking about instance Ord1 Down issues)

[cgibbard]

This is something I'd also love to see some impact analysis on, but I can imagine most users of the *1 classes being happy about the improvement, even if it might involve knocking out some forgotten instances. I'd also personally be happy about the fixes for Compose. So this is a tentative +1 from me, unless we somehow see that the world would be covered in tiny fires.

[Bodigrim]

I'm tentatively in favor of this proposal, it reflects my experience with *1 classes and QuantifiedContraints. But we really need an impact assessment to understand what we are voting for.

[Bodigrim]

@phadej could you possible please share instructions how to test changes to base against Stackage, as you did in #3 (comment) ?

[phadej]

The canonicity law looks good to me -- I'm actually surprised that Eq1 and co didn't already have something like this. The quantified constraint superclass also looks fine and sensible to me.

The Eq1 class today doesn't have any relationship with Eq, thus stating the liftEq (==) = (==) law is awkward. Quantified super constraint adds necessary bits so the law always type-checks.

the classes are still meaningful if one wants to lift a non-canonical function on the inside. I am not sure how often that is done, but it's theoretically useful enough that I don't feel inspired to rid of this.

E.g. QuickCheck listOf combinator is very much used, it's a special case of liftArbitrary. i also use liftShowsPrec quite a lot to show lists of things which don't (or cannot) have good Show instance (or have bad one).

Are these laws unconditional or are they expecting that an underlying (==) is lawful? (I'm thinking about instance Ord1 Down issues)

I'd say it's fair to assume that (==) :: a -> a -> Bool is lawful. We can say that for every reflexive, transitive f, liftEq f is also reflexive and transitive: That is relaxed version as liftEq f _ _ = True is trivially reflexive and transitive for any f, but otoh it's stricter as liftEq implementation may assume that f is reflexive and transitive.

could you possible please share instructions how to test changes to base against Stackage

I'll try to do that asap. (They aren't complicated, but not entirely trivial). A good starting point is to have a patch applied to a version of GHC and base which has stackage snapshot (e.g. 9.0.1 or 8.10.x). Migrating stackage to ghc-head is too much work.

[Ericson2314]

Thanks for mentioning @phadej that the laws are awkward to have without the superclass. That is an added benefit I forgot.

The MR had a error I think I just figured out --- a test needed to be updated, but it was in the haddock submodule so my git diff missed it. When it rebuilds I think there is a button I can hit to make it do a head.hackage run.

[Ericson2314]

Are these laws unconditional or are they expecting that an underlying (==) is lawful?

I think they need not relay on a sane underlying (==). e.g. [NaN, Nan] :: [Double] should not be equal to itself, right?

In fact,

liftEq f = and . liftA2 f
liftCompare f = foldr thenCmp EQ . liftA2 f

might be valid laws too. But I don't know what the cost of those super-classes would be, and especially thenCmp not being associative, so I rather punt on that until later :).

[phadej]

There are also wrappers in other core-ish libraries, e.g. Reverse and Backwards:

• https://hackage.haskell.org/package/transformers-0.6.0.2/docs/Data-Functor-Reverse.html
• https://hackage.haskell.org/package/transformers-0.6.0.2/docs/Control-Applicative-Backwards.html

Would be nice if instances were uniform for these.

[phadej]

Does this proposal break or not code like:

class Eq1 t => Hashable1 t where ...
instance (Hashable1 f, Hashable1 g) => Hashable1 (Compose f g) where ...

I have to think about this, so probably a good idea to explicitly write the answer down in the proposal.

[chessai]

I'm also tentatively +1

[Bodigrim]

@phadej just a gentle reminder about #10 (comment)

@Ericson2314 any chance to test against head.hackage?

[phadej]

@Bodigrim as I said, the first step is to have a patch against a GHC version which has a stackage snapshot. GHC master and having to use head.hackage just adds to much variables, that it's not feasible IMO.

Then the simple approach is to create a cabal package with

• a cabal.config (add it to a url of a snapshot e.g. https://www.stackage.org/nightly-2021-12-17/cabal.config) changing
  • constraints to build-depends
  • removing installed packages
• the cabal build
  • some packages are executable only. You may remove them or move to build-tool-depends (but then you need to figure out the executable names, which usually are the same as package ...)

This approach also ignores flags as cabal.config doesn't have them. (It could add constraints: foo +flag, but it doesn't, those can be added to cabal.project then).

This all can be automated, by

• using YAML snapshot specs from https://github.com/commercialhaskell/stackage-snapshots
  • this would allow to pin flags too.
  • possibly add diagnostics when revisions are made
• using cabal-install-parsers to read the index (to figure out which packages are executables, diagnostics mentioned above)

And one general tip is to do 100 packages at the time, by first doing cabal build --dry: Solver in all cabal installs is slow when there are many direct
dependencies (i think it's improved in master), as build plans will fail and
you'll need to figure out missing native dependencies or just disable these
packages. Such metadata would be great to have up front, and the tool would
figure that out directly as here we are not really solving anything.
It might be it's slow because I didn't constraint flags,
so solver tries to flip them.

[Ericson2314]

The Hashable instance is fine. See how the Eq1 instance for Compose is unaffected on the patch. I would recommend those packages adjust the definition of Hashable itself analogously, but it's not mandatory.

[Ericson2314]

Hmm I thought this testing stuff was already automated, but I guess not.

[Bodigrim]

@Ericson2314 any chance for impact assessment please? I'd like to resolve this one way or another.

[Ericson2314]

Sorry, I have not blocked out the chunk of time needed to get through that fairly manual process yet. I will try to bump this forward in my queue of things to do.

[Bodigrim]

@Ericson2314 I've created https://github.com/Bodigrim/clc-stackage, which is a meta-package for the majority of libraries in Stackage Nightly. Essentially all you need to do is to backport your changes to base-4.15, compile GHC from 9.0 branch and run cabal build -w ghc-9.0.your_patch. You can further provide fixes for affected packages via local copies, referenced from cabal.project. This should give a rough idea how much is broken.

[Ericson2314]

Thanks @Bodigrim. I am hoping after haskellfoundation/tech-proposals#27 goes though this process will be quite polished, and then I will give this a shot.

---

Separately, doing

git log libraries/base/Data/Functor/Classes.hs

turned up ghc/ghc@e0e03d5 which linked https://mail.haskell.org/pipermail/libraries/2015-July/026014.html

It looks like these *1 classes were only added to base based of the data types that use that had these instances. With this (or #35), those data types no longer need those *1 classes to implement the more basic ones. What that means is that we can invert the dependency and make the modules with *1 classes depend on those data types instead.

This makes me also want to just remove the Ord1 classes from base entirely! So

• Generalise the instances of Eq, Ord, Read and Show for Compose #35 is the plan for base
• Create new functor-classes package

The question remains, should this be done right away, or as a successive step in a longer deprecation cycle?

I propose that https://gitlab.haskell.org/ghc/ghc/-/issues/20647 / https://discourse.haskell.org/t/re-pre-pre-hftp-decoupling-base-and-ghc/4269/3 gives us an alternative.

1. • Do this proposal as originally written
   • Move base to ghc-base in the GHC repo, with base a trivial package (in another repo) that reexports ghc-base.

2. Fork out functor-classes, making it depend on ghc-base, and make base reexport functor-classes too.

3. After a deprecation cycle, get rid of the functor-classes re-export.

This means we still get a smooth migration, but the benefit of making obscure stuff out of tree immediately. This woks best if https://gitlab.haskell.org/ghc/ghc/-/issues/4879 / https://github.com/ghc-proposals/ghc-proposals/blob/master/proposals/0134-deprecating-exports-proposal.rst is finally implemented, so those reexports can be deprecated immediately.

edit actually ghc-proposals/ghc-proposals#489 is what is needed since these are module not definition reexports, and we want base and functor-classes to not export technically different modules with overlapping names,

[Icelandjack]

Several instances were omitted from Generically1, both to simplify its inclusion and in an effort to make deriving X via Generically(1) D match deriving stock X as closely as possible. The Read(1) and Show(1) instances were not part of that selected instances. I will make an issue proposing the addition to Eq and Ord but they are subtly different. With data D = D deriving stock Eq we get undefined == D = undefined. However deriving via Generically1 D gives undefined == D = True.

[Ericson2314]

Opened #84 with the migration guide.

[mpickering]

It appears there are some packages on stackage which need changes due to this proposal but weren't flagged in the impact assessment. Do you know why they were missed?

For example:

dual:

Control/Category/Dual.hs:23:10: error: [GHC-39999]
    • Could not deduce ‘Eq1 (Dual k a)’
        arising from the head of a quantified constraint
        arising from the superclasses of an instance declaration
      from the context: Eq2 k
        bound by the instance declaration
        at Control/Category/Dual.hs:23:10-30
      or from: Eq a
        bound by a quantified context at Control/Category/Dual.hs:23:10-30
    • In the instance declaration for ‘Eq2 (Dual k)’
   |
23 | instance Eq2 k => Eq2 (Dual k) where liftEq2 f g (Dual x) (Dual y) = liftEq2 g f x y

selective:

src/Control/Selective/Trans/Except.hs:51:7: error: [GHC-39999]
    • Could not deduce ‘Selective m’
        arising from the head of a quantified constraint
        arising from the 'deriving' clause of a data type declaration
      from the context: Monad m
        bound by a quantified context
        at src/Control/Selective/Trans/Except.hs:51:7-16
      Possible fix:
        add (Selective m) to the context of a quantified context
    • When deriving the instance for (MonadTrans (ExceptT e))
   |
51 |     , MonadTrans, MonadFix, MonadFail, MonadZip, MonadIO, MonadPlus, Eq1, Ord1
   | 

bencoding

src/Data/BEncode.hs:671:35: error: [GHC-39999]
    • No instance for ‘MonadPlus (Either String)’
        arising from the 'deriving' clause of a data type declaration
      Possible fix:
        use a standalone 'deriving instance' declaration,
          so you can specify the instance context yourself
    • When deriving the instance for (Alternative Get)
    |
671 |   deriving (Functor, Applicative, Alternative)
    |    

either-both:

Data/Either/Both.hs:17:10: error: [GHC-39999]
    • No instance for ‘Functor (Either' a)’
        arising from the head of a quantified constraint
        arising from the superclasses of an instance declaration
    • In the instance declaration for ‘Bifunctor Either'’
   |
17 | instance Bifunctor Either' where
   |          ^^^^^^^^^^^^^^^^^

quickcheck-state-machine:

[ 5 of 21] Compiling Test.StateMachine.Types.References ( src/Test/StateMachine/Types/References.hs, dist/
build/Test/StateMachine/Types/References.o, dist/build/Test/StateMachine/Types/References.dyn_o )

src/Test/StateMachine/Types/References.hs:94:10: error: [GHC-39999]
    • Could not deduce ‘Eq (Concrete a)’
        arising from the head of a quantified constraint
        arising from the superclasses of an instance declaration
      from the context: Eq a
        bound by a quantified context
        at src/Test/StateMachine/Types/References.hs:94:10-21
    • In the instance declaration for ‘Eq1 Concrete’
   |
94 | instance Eq1 Concrete where
   |   

[phadej]

Do you know why they were missed?

Because impact assessment is done in june of 2022? And probably against older Stackage LTS, which had different package set or/and older major versions.

---

In case of dual the fix is straight-forward, yet maybe not that satisfactory:

--- a/Control/Category/Dual.hs
+++ b/Control/Category/Dual.hs
@@ -1,7 +1,7 @@
 {-# LANGUAGE DerivingVia #-}
 module Control.Category.Dual where

-import Prelude (Eq, Ord, Read, Show, Bounded, ($))
+import Prelude (Eq ((==)), Ord (compare), Read, Show, Bounded, ($))

 import Control.Category
 import Data.Bifunctor
@@ -20,6 +20,9 @@ instance Category k => Category (Dual k) where
     id = Dual id
     Dual f . Dual g = Dual (g . f)

+instance (Eq2 k, Eq a) => Eq1 (Dual k a) where liftEq f (Dual x) (Dual y) = liftEq2 f (==) x y
+instance (Ord2 k, Ord a) => Ord1 (Dual k a) where liftCompare f (Dual x) (Dual y) = liftCompare2 f compare x y
+
 instance Eq2 k => Eq2 (Dual k) where liftEq2 f g (Dual x) (Dual y) = liftEq2 g f x y
 instance Ord2 k => Ord2 (Dual k) where liftCompare2 f g (Dual x) (Dual y) = liftCompare2 g f x y

@@ -31,6 +34,9 @@ instance Show2 k => Show2 (Dual k) where
     liftShowsPrec2 asp asl bsp bsl n =
         showsUnaryWith (liftShowsPrec2 bsp bsl asp asl) "Dual" n . dual

+instance Bifunctor k => Functor (Dual k a) where
+    fmap f = Dual . bimap f id . dual
+
 instance Bifunctor k => Bifunctor (Dual k) where
     bimap f g = Dual . bimap g f . dual

The problem is that there is no classes like

class FunctorOn2 p where
   anotherFirst :: (a -> b) -> p a x -> p b x

so constraints for missing superclasses (Eq1, ..., Functor) will have overly restricted constraints. But at least we can write them, so it's not a blocker. (And because Functor (Dual k a) instance is "ugly" is probably a reason why it was omitted by the author).

For the reference Bifunctor superclass change is proposal #91 change

---

For quickchekc-state-machine the fix is

diff --git a/src/Test/StateMachine/Types/References.hs b/src/Test/StateMachine/Types/References.hs
index 9e4200d..89b59da 100644
--- a/src/Test/StateMachine/Types/References.hs
+++ b/src/Test/StateMachine/Types/References.hs
@@ -81,6 +81,8 @@ instance Ord1 Symbolic where
 data Concrete a where
   Concrete :: Typeable a => a -> Concrete a

+deriving stock instance Eq a => Eq (Concrete a)
+deriving stock instance Ord a => Ord (Concrete a)
 deriving stock instance Show a => Show (Concrete a)

 instance Show1 Concrete where

---

Especially the latter case is a problem of rolling stone uphill: without enforcement to have Eq a => Foo (Eq a) when you write Eq1 Foo instances, they will be omitted (cause humans makes mistakes). No different than Generically1 case #10 (comment), these just happen until the change is released.

[Bodigrim]

• dual is a deprecated package, not much point to patch.

• selective failure does not seem related, it is probably caused by changes to superclasses of MonadTrans in transformers-0.6.

• bencoding is again unrelated, it's just that transformers-0.6 stopped providing instance MonadPlus (Either String).

• either-both is again deprecated.

• quickcheck-state-machines was not a part of Stackage Nightly 2022-06-17 which was used for impact assessment.

[chshersh]

I'm trying to summarise the state of this proposal as part of my volunteering effort to track the progress of all approved CLC proposals.

Field	Value
Author	@Ericson2314
Status	merged
base version	4.18.0.0
Merge Request (MR)	https://gitlab.haskell.org/ghc/ghc/-/merge_requests/4727
Blocked by	nothing
CHANGELOG entry	present
Migration guide	https://github.com/haskell/core-libraries-committee/blob/main/guides/functor-combinator-instances-and-class1s.md

Please, let me know if you find any mistakes 🙂