Generic Deriving #1138

gbaz · 2015-06-03T21:23:39Z

Addresses issue #647 among other features.

Works hand in hand with userland library code as in https://gist.github.com/gbaz/a2c867b6c07731a403eb

gbaz · 2015-06-03T21:28:53Z

(Also note the silliness in how I had to write the Proxy type in the gist. This may be fixed in 7, but in 6.9.5 doing the obvious phantom type thing failed)

joneshf · 2015-06-04T01:45:09Z

This is awesome!

gbaz · 2015-06-04T02:15:58Z

The Gist is now mainly updated to work with purescript 0.7. Either the typechecker got better or the explicit ascription trick for Proxy worked or both, as now I can do things the sane way.

Since it was a very quick translation, the code for genericShow is now especially ugly -- but it was always included simply as a demonstration and not a proper implementation anyway.

…ng-generic-patch

gbaz · 2015-06-25T19:40:02Z

The library component to go with this now lives here, btw: purescript-deprecated/purescript-generics#3

paf31 · 2015-06-28T23:12:34Z

src/Language/PureScript/TypeChecker/Deriving.hs

+lamCase s = lam s . Case [mkVar s]
+
+liftApplicative :: Expr -> [Expr] -> Expr
+liftApplicative = foldl' (\x e -> App (App (mkPrelVar "ap") x) e)


Maybe use apply here to avoid an indirection via Monad in the generated code?

paf31 · 2015-08-08T18:23:08Z

Sorry this PR has been left unmerged for a while. Here are my thoughts on it:

If it's merged, I'd like to fix the couple of minor issues I mentioned above.
I'm nervous about modifying the typechecker to add new features at this point. I wouldn't be so nervous if the typechecker were better, so maybe this will no longer be an issue after it's updated (whenever that happens). Ideally though, I'd find a solution which doesn't involve the new member placeholder.
I feel like this would be a good example of an external tool, and doesn't necessarily need to be added to the compiler. Support for specific libraries probably doesn't belong in the compiler if we have a nice way to run external tools as part of the build. Template programing would be ideal, but maybe there is something simpler which can do the job? Even something as simple as a step in a Makefile which runs the tool and modifies the source files. Maybe see purescript-derive-lenses as an example of that approach (although, I admit it's slightly different because you can't create another module here, without creating orphan instances)
Is there a reasonable variant of this and purescript-generics which doesn't use type classes? Something like

newtype Generic a = Generic 
  { toSpine :: a -> Spine 
  , toSignature :: Proxy a -> GenericSignature
  , fromSpine :: GenericSpine -> Maybe a
  }

showGeneric :: forall a. Generic a -> a -> String

This way, wrapping up Generic in a type class would be simple, writing instances would be trivial, and we could move this PR into an external tool without worrying about generating orphans.

gbaz · 2015-08-10T17:10:57Z

I'd be happy to make the changes requested if we can agree that then this could be merged :-)

On the concern that this modifies the typechecker... I understand this. However, bear in mind that the only change to the typechecker proper is a "one liner" as follows:

+check' e@TypeClassInstanceMemberFunction{} ty = flip check' ty =<< elaborateInstance e

This should be really robust to virtually any refactor. If there was an "elaboration" phase where we had more information available but prior to the checking phase, then the checker wouldn't need to be changed at all, imho.

I've been thinking about this for a few ways now, and given a desire to not have orphans, I don't see how this can be a standalone tool 😢

In particular, we don't just want to do "instance Generic Foo" but then also "instance Show Foo where show = genericShow" etc.

So being able to derive generic in the same file as the definition isn't just about putting it to work elsewhere, but then piggybacking on that to derive a lot of other instances. And since those other instances need to be "real" instances not handed a manual dictionary -- i.e. so that the Show instance can be used by something that shows pairs of things, etc, then there isn't much of a way around this...

I think the power/weight ratio remains good here, since the idea is that by deriving Generic, then "once and forall" a whole bunch of userland things become possible at once, and the net effect is to take a whole bunch of other potential feature requests and allow them to be undertaken in userland instead.

So if you'll agree to merge once I make the requested changes, I'll proceed?

paf31 · 2015-08-10T18:45:00Z

src/Language/PureScript/Sugar/TypeClasses.hs

+  let missingDecls = mapMaybe declName tyDecls \\ mapMaybe declName decls
+  let mkMissingDecl cls fun =
+          case tys of
+            [TypeConstructor t] | unQualify className == ProperName cls && Ident fun `elem` missingDecls -> return [ValueDeclaration (Ident fun) Public [] (Right $ TypeClassInstanceMemberFunction (Ident fun) className t)]


I think we'd want to match the whole qualified name, since having something called Generic in a different not-generics module shouldn't apply, right?

gbaz · 2015-08-10T19:47:56Z

I take it these additional comments indicate a willingness to pull?

The qualified names choice was deliberate, but is open to discussion.

The idea of pulling whatever "Generic" is in scope is to let users swap in whatever Generic class they want, if they don't like the version in that particular package -- as long as instances can be given in the same way, you could write something that worked over an alternate sort of skeleton datatype, etc.

I wanted to be as general-purpose and agnostic as possible.

If you think that this will be more confusing than useful, then I can of course change it to specify.

paf31 · 2015-08-10T20:11:00Z

I think you're right about the power-to-weight ratio being well worth it. I put a couple of other minor comments on the code above.

Also, I guess we'd need to update the purescript-generics library again for 0.7?

I don't think the qualified name thing is about "what's in scope" so much as, what if someone doesn't know this feature exists and defines a Generic class, then they'll get errors because we tried to define members which didn't exist? We can always just switch out the implementation of Data.Generic.Generic by replacing the package entirely, but maybe that's a versioning nightmare waiting to happen. Another ugly solution is to add a command line argument for the Generics module you want to use, with a default of Data.Generic.

And yes regarding willingness to pull 😄

Could you please add a link to the MIT license to the top of your new file as well? We're probably going to go through and do this for other files too. I know it says LICENSE: MIT right now, but since the copyright ascription is different from the other files, it seems to make sense to be clear about how this file in particular is licensed.

gbaz · 2015-08-10T20:19:27Z

Ok. So I'll go ahead and try to clean all the above. And I'll go ahead and add qualified names for now to point to "Data.Generic.Generic". We can always revisit and generalize later.

…ng-generic-patch

paf31 · 2015-08-10T22:28:46Z

src/Language/PureScript/TypeChecker/Deriving.hs

+
+      toSpineFun :: Expr -> Type -> Expr
+      toSpineFun i (ObjectType rec) =
+          lam "q" . recordConstructor . ArrayLiteral .


Might this q shadow some other name? Why not use a NullBinder?

didn't knwo about nullbinders is why 😄. That said, the shadowing won't hurt afaik because there shouldn't be any names inside the thing we're building... But I'll go put in null binders where appropriate.

true but you might shadow something at the top level.

Not that it matters, but it will generate a warning.

gbaz · 2015-08-10T22:37:58Z

Ok, changed things vis a vis constants and null binders. Still have yet to force only using "Data.Generics" as the modulename, but otherwise things seem to still be working.

gbaz · 2015-08-11T17:53:50Z

I think I've made all requested changes and tested. To integrate with the main testsuite it would make sense to have the new generic module on bower since that's where the testsuite seems to pull the modules from, so that can be a second pass.

paf31 · 2015-08-11T18:01:05Z

I'll merge your purescript-generic changes soon so that we can add a test to the test suite here.

paf31 · 2015-08-11T18:01:11Z

Also, thanks! 👍

paf31 · 2015-08-11T20:32:51Z

@gbaz I'm getting ready to merge this, thanks. From above:

Could you please add a link to the MIT license to the top of your new file as well? We're probably going to go through and do this for other files too. I know it says LICENSE: MIT right now, but since the copyright ascription is different from the other files, it seems to make sense to be clear about how this file in particular is licensed.

gbaz · 2015-08-11T21:29:55Z

License link added.

I haven't added something to use proxy from the proxy library yet. To be honest I'd just like to tie off this pull request as is and do that as a separate patch next :-)

paf31 · 2015-08-11T21:30:52Z

Yep, sounds good. Thanks 👍

paf31 · 2015-08-11T21:31:07Z

I'll merge this and then work on adding tests later today.

paf31 · 2015-08-11T21:35:10Z

Hmm, CI build is failing on 7.6.3 because of PatternSynonyms. I think we'd prefer to keep 7.6.3 compatibility if we can. Could these patterns be inlined, or could we use PatternGuards or something else instead if they're too unwieldy?

…warnings

gbaz · 2015-08-11T22:10:06Z

done

Generic Deriving

paf31 · 2015-08-12T02:45:41Z

Let's see what CI says this time...

Generic Deriving

e2e78ca

U-CIQDEV\gbazerman added 4 commits June 8, 2015 13:31

use 7.8 compat error import

2dd990f

Merge branch 'master' of github.com:purescript/purescript into derivi…

74a8dc0

…ng-generic-patch

use new NameKind constructor, add some missing Functor constraints

2156fdc

Another missing constraint for 7.8

d84e2c1

paf31 reviewed Jun 28, 2015
View reviewed changes

paf31 reviewed Aug 10, 2015
View reviewed changes

Merge branch 'master' of github.com:purescript/purescript into derivi…

fd82591

…ng-generic-patch

paf31 reviewed Aug 10, 2015
View reviewed changes

cleanup PR as per comments. fixing the module name outstanding.

0f9d2f7

tweak binders, always use fixed Data.Generic module

e28d22f

add link to license

0611c05

pattern synonyms --> pattern guards, also fix some missing signature …

04616c7

…warnings

paf31 added a commit that referenced this pull request Aug 12, 2015

Merge pull request #1138 from gbaz/deriving-generic-patch

396aa3b

Generic Deriving

paf31 merged commit 396aa3b into purescript:master Aug 12, 2015

gbaz deleted the deriving-generic-patch branch August 13, 2015 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic Deriving #1138

Generic Deriving #1138

gbaz commented Jun 3, 2015

gbaz commented Jun 3, 2015

joneshf commented Jun 4, 2015

gbaz commented Jun 4, 2015

gbaz commented Jun 25, 2015

paf31 Jun 28, 2015

paf31 commented Aug 8, 2015

gbaz commented Aug 10, 2015

paf31 Aug 10, 2015

gbaz commented Aug 10, 2015

paf31 commented Aug 10, 2015

gbaz commented Aug 10, 2015

paf31 Aug 10, 2015

gbaz Aug 10, 2015

paf31 Aug 10, 2015

paf31 Aug 10, 2015

gbaz commented Aug 10, 2015

gbaz commented Aug 11, 2015

paf31 commented Aug 11, 2015

paf31 commented Aug 11, 2015

paf31 commented Aug 11, 2015

gbaz commented Aug 11, 2015

paf31 commented Aug 11, 2015

paf31 commented Aug 11, 2015

paf31 commented Aug 11, 2015

gbaz commented Aug 11, 2015

paf31 commented Aug 12, 2015

Generic Deriving #1138

Generic Deriving #1138

Conversation

gbaz commented Jun 3, 2015

gbaz commented Jun 3, 2015

joneshf commented Jun 4, 2015

gbaz commented Jun 4, 2015

gbaz commented Jun 25, 2015

paf31 Jun 28, 2015

Choose a reason for hiding this comment

paf31 commented Aug 8, 2015

gbaz commented Aug 10, 2015

paf31 Aug 10, 2015

Choose a reason for hiding this comment

gbaz commented Aug 10, 2015

paf31 commented Aug 10, 2015

gbaz commented Aug 10, 2015

paf31 Aug 10, 2015

Choose a reason for hiding this comment

gbaz Aug 10, 2015

Choose a reason for hiding this comment

paf31 Aug 10, 2015

Choose a reason for hiding this comment

paf31 Aug 10, 2015

Choose a reason for hiding this comment

gbaz commented Aug 10, 2015

gbaz commented Aug 11, 2015

paf31 commented Aug 11, 2015

paf31 commented Aug 11, 2015

paf31 commented Aug 11, 2015

gbaz commented Aug 11, 2015

paf31 commented Aug 11, 2015

paf31 commented Aug 11, 2015

paf31 commented Aug 11, 2015

gbaz commented Aug 11, 2015

paf31 commented Aug 12, 2015