Opened 10 months ago

Last modified 2 months ago

#9043 infoneeded feature request

Add missing type class instances for data types in GHC.Generics

Reported by: ocharles Owned by: dreixel
Priority: normal Milestone: 7.12.1
Component: Core Libraries Version: 7.8.2
Keywords: Cc: bgamari, hvr, ekmett, core-libraries-committee@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: #9766 Blocking:
Related Tickets: #8778 Differential Revisions:

Description

GHC.Generics offers a bunch of data types which have valid instances of various type classes in base, such as Functor. There's no good reason not to have these instances, so they should be added.

Change History (30)

comment:1 Changed 10 months ago by ocharles

  • Owner set to ocharles

comment:2 Changed 10 months ago by ekmett

Makes a lot of sense to me.

The missing Functor/Foldable/Traversable instances for these have bitten me before and the instances are entirely determined by their laws / convention.

There are a few other instances that make sense uniformly:

e.g. U1, Par1, Rec1, and (:*:), and M1 can also be made instances of Applicative, Monad, MonadZip in a completely unambiguous way determined by the laws just by lifting them from their parts.

(:.:) admits an unambiguous Applicative, but not the others.

It might also make sense to look into making the types in there instances of Generic / Generic1 for bootstrapping purposes, and they are currently missing Typeable and Data instances as well.

The instances probably have to live out mostly where the typeclasses come from rather than in the module itself to avoid cycles, but otherwise I don't see any headaches, and they are entirely determined by the definitions of the classes involved, so there is no bikeshedding opportunity.

comment:3 Changed 10 months ago by dreixel

  • Owner changed from ocharles to dreixel

comment:4 Changed 10 months ago by ocharles

  • Status changed from new to patch

I have a branch that adds almost all the above mentioned type clasess at https://github.com/ocharles/ghc/tree/generics-instances. I didn't do Data, mostly because I ran out of steam - if people feel strongly about that I can come back and add it.

What are the next steps to take?

comment:5 Changed 10 months ago by ekmett

For the Data instances can we just get away with using DeriveDataTypeable / standalone deriving?

comment:6 Changed 10 months ago by hvr

@ocharles I have to nitpick: could you please separate whitespace changes occurring in parts other than the lines you change into a separate commit?

comment:7 Changed 10 months ago by ocharles

hvr - as mentioned on Github, I was somewhat intending to squash this down into a single commit when I'm done. If you'd like, I could simply omit whitespace changes from this. My editor does it for me, so I sometimes forget it is happening :)

comment:8 Changed 10 months ago by ocharles

Alright, I've responded to some comments and added more instances. Could people please have another look? I wrote out Data instances, but I have never actually used Data.Data, so I may have got that wrong. Is it possible to write Data instances for (:+:) and friends? My initial attempt was no, but I may have missed something. Latest code is at https://github.com/ocharles/ghc/tree/generics-instances (as before).

comment:9 Changed 10 months ago by ekmett

I'll show you how to write the instances for (:+:) and the like on IRC when I get a chance.

comment:10 follow-up: Changed 9 months ago by ocharles

Ok, my branch has been updated with the Data instances. I think we're all done here! Here's the final diff that needs reviewing:

https://github.com/ocharles/ghc/compare/master...generics-instances

If people are happy with this, I will probably rewrite the branch to be a single commit, unless people feel the history here is relevant.

comment:11 Changed 9 months ago by simonpj

I'm not following the details here, but if the core-libraries commmitte are happy please turn it into one comment and change to 'patch' status so that Austin can merge it. Thanks!

Simon

comment:12 in reply to: ↑ 10 Changed 9 months ago by hvr

Replying to ocharles:

If people are happy with this, I will probably rewrite the branch to be a single commit, unless people feel the history here is relevant.

...please put whitespace changes in a separate commit though :-)

comment:13 Changed 9 months ago by dreixel

I've had a look at this, and all seems fine to me. One caveat however:

instance (Data p, Data (f p), Typeable c, Typeable i, Typeable f) => Data (M1 i c f p) where

This Typeable c constraint will require changing the generation of Generic instances in GHC, because these cs are compiler-generated empty datatypes, which currently do not derive Typeable.

Simon: what's the best way to do this? We need Typeable instances for the stuff generated by mkBindsMetaD in TcGenGenerics.

comment:14 follow-up: Changed 9 months ago by simonpj

Well I suppose you'd need to derive Typeable for these generated data types.

Mind you, IIRC now that we have DataKinds I think the Generics stuff could be done much more neatly, dramatically reducing the number of generated data types.

This is all code you wrote, Pedro, isn't it? So you are definitely free to improve it!

Simon

comment:15 in reply to: ↑ 14 Changed 9 months ago by dreixel

Replying to simonpj:

Well I suppose you'd need to derive Typeable for these generated data types.

Ok, I'll do that; I hope it doesn't require much plumbing.

Replying to simonpj:

Mind you, IIRC now that we have DataKinds I think the Generics stuff could be done much more neatly, dramatically reducing the number of generated data types.

This is all code you wrote, Pedro, isn't it? So you are definitely free to improve it!

Sure. All I'm worried about is backwards-compatibility, and the lack of #6024 doesn't help.

comment:16 Changed 9 months ago by simonpj

I'm all for #6024 if you want to tackle that :-)!

Simon

comment:17 Changed 9 months ago by ocharles

I'd be interested in helping tidy up the stuff in GHC.Generics to use data kinds, btw. So dreixel, if that's something that you think is somewhat obvious and wouldn't mind me doing it - just let me know!

comment:18 follow-up: Changed 9 months ago by ocharles

Ok folks, I've squashed this down to just two commits now - one with instances and one with whitespace changes. I think we may be all ready to go, and we can derive typeable in a separate commit. It does mean that without that the Data instances are fairly useless.

Thoughts?

comment:19 in reply to: ↑ 18 Changed 9 months ago by dreixel

Sounds good to me. Please leave the ticket open; it might still take me a few weeks to implementing the changes in GHC, but I think that's not a problem.

comment:20 Changed 7 months ago by thoughtpolice

  • Milestone set to 7.10.1
  • Status changed from patch to infoneeded

Pedro, any updates on this? Should Ollie's changes be merged or do you have some other plans here?

comment:21 Changed 7 months ago by dreixel

Sorry for the delay; it's still in my to do list. His changes can be merged, though; perhaps it's best to do that earlier rather than later, to avoid conflicts. But please keep the ticket open; I'll close it once I make sure that the datatypes generated when deriving Generic have Typeable instances.

comment:22 Changed 5 months ago by thoughtpolice

  • Component changed from libraries/base to Core Libraries

Moving over to new owning component 'Core Libraries'.

comment:23 Changed 5 months ago by dreixel

  • Cc core-libraries-committee@… added

Just a heads-up: I want to implement the changes described in the following wiki page, which would mean there are no longer internally generated datatypes for which we then need Typeable instances:

https://ghc.haskell.org/trac/ghc/wiki/Commentary/Compiler/GenericDeriving#Amoreconservativefirstapproachtothisproblem

comment:24 Changed 5 months ago by ekmett

So then we'd just be piggybacking on typeability of Symbols (and instances for MetaCons/MetaData) to get the Typeable instance for things like M1 i c p?

I have vague recollections of Richard mentioning that having every Symbol/Nat be Typeable. I don't recall what came of it though. Do we have that now in HEAD?

comment:25 Changed 5 months ago by dreixel

I think so...

comment:26 Changed 4 months ago by dreixel

  • Blocked By 9766 added

comment:27 Changed 3 months ago by bgamari

What can we do to make sure this makes it in for 7.10? Perhaps it would make sense to split out the Data and Typeable changes so the other instances can be merged independently.

comment:28 Changed 3 months ago by bgamari

  • Cc bgamari added

comment:29 Changed 3 months ago by dreixel

#9766 is now ready for review. If it all goes well, then this patch can be merged straight after.

comment:30 Changed 2 months ago by thoughtpolice

  • Milestone changed from 7.10.1 to 7.12.1

Moving to 7.12.1 milestone; if you feel this is an error and should be addressed sooner, please move it back to the 7.10.1 milestone.

Note: See TracTickets for help on using tickets.