Migrate more of Data.Functor.* from transformers to base.

changed milestone to %8.0.1

changed weight to 10

added Ttask Trac import core libraries labels

assigned to @trac-strake888

Trac metadata

Trac field	Value
Differential revisions	- → D1543, D1544

Herbert thinks that the patch is incomplete, although 90% done. It'll need to be finished in the next 6 days, including coordination with the transformers package, if it's to get into GHC 8.0.

Ryan, Ross: how much do you care? Do you want to make it happen?

Simon

added upstream label

I'm preoccupied with other tickets that I absolutely want to make it in before the 8.0 release, so I doubt I'll be able to get this one in time (especially since transformers-related changes require several rounds of back-and-forth between Ross, someone with GHC push access, and myself).

Sorry about this, I thought it would be a simple operation to patch transformers before the RC.

I've made the necessary changes to transformers in my repo, and can push them when base is changed.

Ross: I would like to see this change get into GHC 8.0, time permitting.

There's one thing that bothers me about the most recent changes to transformers, however. After introducing Show1/Show2/et al, you changed the Show instances to use them. For instance, the Show instance for Backwards was:

instance (Show1 f, Show a) => Show (Backwards f a) where
    showsPrec d (Backwards x) = showsUnary1 "Backwards" d x

But now it's:

instance (Show1 f, Show a) => Show (Backwards f a) where showsPrec = showsPrec1

The result is that the output of show changes. Before, the output of show (Backwards "hello") would be "Backwards \"hello\"", but with the latest changes, it's "Backwards ['h','e','l','l','o']"! This is because the Show1 [] instance has to be parametric over its argument, so it can't tell that Char has a special Show instance that causes it to be shown differently.

I feel like we should revert all the Eq/Ord/Read/Show instance in transformers back to the way they were (and keep the Show1/Show2/etc. instances separate) to prevent subtle changes like this. Do you agree?

Attached file transformers.diff (download).

I have a patch, now enclosed, to move these to legacy in transformers, but some difficulty deriving Data and Typeable instances so far.

..and i ought to read prior comments first

Ryan: I'm really very negative on the idea of reverting to the old approach.

We rather deliberately changed them in HEAD months ago to make it so that they can be used in more situations without requiring the Functor constraint a few months back, the old approach had rendered these classes useless for a lot of real world situations around GADTs and the like, in a way that simultaneously made implementations inefficient and less often applicable.

The old transformers approach winds up requiring

instance (Functor f, Eq1 f, Eq1 g) => Eq1 (Compose f g)

while the current approach lets you use

instance (Eq1 f, Eq1 g) => Eq1 (Compose f g)

and the code works in one pass regardless of whether or not f and g are known.

The price of admission is a manual implementation. The current compromise Ross has implemented gets a design that is readily implemented and implementable for a wide arrange of things.

Now, we could modify the way we pass in the "manual dictionary" version of 'Show/Read' to allow the showList/readList trick to work. It'd admittedly make the classes harder to implement and use. I could get behind an alternative API that passed in more stuff to readsPrecWith and showsPrecWith, or one that added a second combinator so that most people didn't have to deal with the noise.

I'd be okay with either proceeding with the status quo, or even more comfortable proceeding with a fix that modifies the methods in the Show1 and Read1 classes to pass more information, but it'd take a lot of convincing to bring me around to the old version in transformers, especially when we very consciously changed to get away from it!

Replying to [ticket:11135#comment:111370 ekmett]:

Ryan: I'm really very negative on the idea of reverting to the old approach.

We rather deliberately changed them in HEAD months ago to make it so that they can be used in more situations without requiring the Functor constraint a few months back, the old approach had rendered these classes useless for a lot of real world situations around GADTs and the like, in a way that simultaneously made implementations inefficient and less often applicable.

The old transformers approach winds up requiring
instance (Functor f, Eq1 f, Eq1 g) => Eq1 (Compose f g)
while the current approach lets you use
instance (Eq1 f, Eq1 g) => Eq1 (Compose f g)
and the code works in one pass regardless of whether or not f and g are known.

The price of admission is a manual implementation. The current compromise Ross has implemented gets a design that is readily implemented and implementable for a wide arrange of things.

Now, we could modify the way we pass in the "manual dictionary" version of 'Show/Read' to allow the showList/readList trick to work. It'd admittedly make the classes harder to implement and use. I could get behind an alternative API that passed in more stuff to readsPrecWith and showsPrecWith, or one that added a second combinator so that most people didn't have to deal with the noise.

I'd be okay with either proceeding with the status quo, or even more comfortable proceeding with a fix that modifies the methods in the Show1 and Read1 classes to pass more information, but it'd take a lot of convincing to bring me around to the old version in transformers, especially when we very consciously changed to get away from it!

Yes, I'd come to the same conclusion, and have already made the necessary changes for Show1, and am now working through Read1. Note that the version before the changes also gave the wrong output on show (Compose (Just "abc")).

I've made a patch for splitting Data.Functor.Const into its own module, which is more or less orthogonal to migrating the functors from transformers into base.

Trac metadata

Trac field	Value
Differential revisions	D1543, D1544 → D1543, D1544, D1630

I believe I've fixed the Show1/Read1 issue.

Replying to [ticket:11135#comment:111391 ross]:

I believe I've fixed the Show1/Read1 issue.

Thanks, Ross!

There may be time to make this in for GHC 8.0 after all. I've prepared a patch for transformers which does the appropriate legacy711 migration of the Data.Functor.* modules, and backports Typeable/Data/Generic/Generic1 instances that will be introduced in base.

Note that there are some bad interactions between derived classes and PolyKinds, so I had to employ a few tricks to get the backported instances to work on old versions of GHC. Nevertheless, I tested my changes against every major version of GHC from 7.0–7.10, so this shouldn't break anything.

mentioned in commit e0e03d5b

While I can see how the Eq1, Ord1, Read1 and Show1 classes could be useful in their own right, is it set in stone that the Eq, Ord, Read and Show instances of the various functors must use them? Why can't the Eq instance for Compose just be Eq (f (g a)) => Eq (Compose f g a)? Do we care about Haskell98 compatibility in base?

I don't have a strong opinion either way for Eq, Ord, Read and Show, but I would slightly prefer instances of the form I have above. What I'm really interested in is the other type classes for which the functors could have instances. Do we need to make a Semigroup1, a Monoid1 if we want to make Compose an instance of Semigroup and Monoid? What about a Storable1? Or can we just make an instance Storable (f (g a)) => Eq (Compose f g a)?

I would basically like to make as many of these instances as possible, because I'm working on something at the moment that uses a lot of functors and potentially compositions of functors, but I want to "keep" as many of the instances of my base types as possible.

I'd be happy to do up a patch that made as many of these instances as possible, but I'm wondering would I be expected to make all those extra classes, or could I just make the non-Haskell98 instances? And if so, is there any good reason not to just do the same for Eq, Ord, Read and Show?

@duairc:

The benefit of having Eq1, Ord1, etc. in the first place is that the resulting instances are fully Haskell 98 and do not require language extensions at all.

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

requires FlexibleContexts.

This means that transformers which works on *every* compiler since Haskell 98 without any CPP simply could not accept instances of this form.

Even allowing for that, it is beneficial to allow things like

instance (Eq1 f, Eq1 g) => Eq1 (Compose f g)

because instances like that are more useful in the presence of polymorphic recursion than the at-first-glance simpler pointwise Eq instance, because it says something fundamental about how the instances are defined: that it isn't able to do something hinky with the argument type, and do something fundamentally different for Compose Foo [] Int vs. Compose Foo [] Double, that both have to use the Eq or Ord or Read or Show instance for 'Int' or Double in a homogeneous way, not separate FlexibleInstances. This is confidence you do not get from the pointwise definitions.

I understand that. Like I said, I can see why Eq1, etc. are useful in their own right. But the questions I was really asking are:

Do we care about Haskell 98 compatibility in base?
If not, could I add an instance e.g., Semigroup (f (g a)) => Semigroup (Compose f g a)? There are a bunch of non-Haskell 98 instances I would like to add, especially for Compose. Some of these could be made into Haskell 98-compatible instances with more Eq1-style helper type classes, but some could not.

Up until now we've avoided adding instances that would require extensions of that sort to base as they aren't suitable for describing in a language report without extending the language, and we lack a suitable precedent.

Ultimately, we had to move the classes from Data.Functor.Classes when we moved the other Data.Functor.* data types or we'd have wound up with orphan instances things that were previously perfectly sound and that are already seeing use. In the original Data.Functor migration proposal the inclusion of Data.Functor.Classes was offered up as a reluctant way to avoid random bikeshedding and loss of functionality during the migration of data types that truly belong further up the hierarchy, where we can avoid duplication of them and permit nicer extensions.

On the other hand, we don't exactly have to go fishing for 3 dozen new classes to describe every other class 'one argument up'. There are viable solutions for folks who don't care about Haskell 98 (so long as they also don't care about the Functor concerns brought up above.)

E.g. in my constraints package, using ConstraintKinds and PolyKinds, I supply

data Dict p where Dict :: p => Dict p
newtype p :- q = Sub (p => Dict q)
class Lifting p t where
  lifting :: p a :- p (t a)

Then everything from Lifting Eq, Lifting Monad, to Lifting Monoid all "just work". However, while more universal they aren't as effective as the manual dictionary passing done here. Such a pattern rules out some nice usecases, while simultaneously requiring a ridiculous number of instances to be defined by all users and a fair bit of sophistication to use.

Consequently, I'm very much not advocating for something like that more radical approach.

As for Semigroup (Compose f g a) -- we have an Applicative (Compose f g) because there is a canonical construction for nesting them. However, the choice of Semigroup here is quite open. You can't reason usefully about the instance you propose except instance by instance and there are multiple viable candidates. Let's consider Monoid:

Given the existence of both:

instance (Applicative f, Applicative g, Monoid a) => Monoid (Compose f g a)
instance Applicative (f (g a)) => Monoid (Compose f g a)

and the lack of a compelling motivation to pick one over the other, while one forces us to incur a flexible context, and the other requires a needless unit in the applicative structure when weakened to give the Semigroup constraint, I'd say it is better to supply neither instance.

We've been meaning towards supplying instances when they are unambiguous and non-controversial. I'd say that such an instance for Semigroup (Compose f g a) would pass neither test.

For right now I see the migration of the Data.Functor.Classes from transformers to base as more of a necessary evil than as a pattern to emulate, and trying to hit a decent compromise point, and I'd rather see if they see any sort of meaningful adoption outside of the transformers class hierarchy before we go and double/triple down on this design immediately.

That said, as Monoid (now being in Prelude) and Semigroup in a few releases become more ingrained in our culture the idea of a Monoid1 and Semigroup1 to extend the consistency of this module might take better hold, but deciding that we want to do so probably belongs in the context of a much broader discussion in the libraries@ mailing list, rather than a rapidfire amendment in the last days before a release.

E.g. is Monoid1 sufficiently distinguished from Applicative to be worth defining? After all, you can likely prove that you need full parametricity in the argument, which would enable you to define an Applicative structure instead, meaning it likely doesn't add any expressive power, which would morally place Semigroup1 at the same level as semi-applicative / Apply in the class hierarchy -- with nearly equivalent parametricity requirements. I personally find such structures useful, but I don't think there is community will to support a finer-grained AMP at this time.

tl;dr My answer to your two questions could best be summaried as

) Insofar as it prevents people from writing portable code if they want to and would break such code that already exists, then I think the answer is yes.
) Even if you don't buy my reasoning in (1), unilaterally adding such an instance would rule out a potentially much more interesting discussion that should be had on libraries@ that I at least personally think would be more valuable.

Thanks for the detailed answer!

mentioned in commit edcf17bd

closed

Trac metadata

Trac field	Value
Resolution	Unresolved → ResolvedFixed

removed upstream label

Could we add this?

import System.Mem.StableName

instance Eq1 StableName where
 liftEq _ = eqStableName

I didn't see it in https://phabricator.haskell.org/D1543

We definitely need to add a lot more Eq1/Eq2/etc. instances than what are currently provided. I didn't include any instances that weren't from transformers in D1543 to keep the migration simple. Another reason I held off on new instances is that I'd like to be able to derive Eq1/Eq2/etc. with a new language extension (for which I haven't though a good name yet—DeriveLiftedClasses perhaps?) to eliminate a lot of the inevitable boilerplate.

If you really need an Eq1 StableName instance right now, feel free to submit a patch to Phabricator—but I doubt I'll get around to it until later.

mentioned in issue #12358 (closed)

added Phighest label

Trac field	Value
Version	7.10.2
Type	Task
TypeOfFailure	OtherFailure
Priority	highest
Resolution	Unresolved
Component	libraries/base
Test case
Differential revisions
BlockedBy
Related
Blocking
CC	ekmett
Operating system
Architecture

Migrate more of Data.Functor.* from transformers to base.

Child items 0

Activity