Eq1 / Ord1 deriving again by garyb · Pull Request #3207 · purescript/purescript

garyb · 2018-01-20T22:26:42Z

Resolves #2702. Much simpler than the previous #2703 😄.

The Eq1 part is done, will do the same for Ord1 now.

garyb · 2018-01-20T22:36:55Z

Brap!

kritzcreek

Looks good, a few style comments

kritzcreek · 2018-01-20T22:53:16Z

src/Language/PureScript/Sugar/TypeClasses/Deriving.hs

+
+deriveEq1
+  :: forall m
+   . (MonadError MultipleErrors m, MonadSupply m)


These constraints seem unnecessary?

kritzcreek · 2018-01-20T22:53:48Z

src/Language/PureScript/Sugar/TypeClasses/Deriving.hs

+
+deriveOrd1
+  :: forall m
+   . (MonadError MultipleErrors m, MonadSupply m)


kritzcreek · 2018-01-20T22:56:36Z

src/Language/PureScript/Sugar/TypeClasses/Deriving.hs

+  | className == Qualified (Just dataEq) (ProperName "Eq1")
+  = case tys of
+      [ty] | Just (Qualified mn' _, _) <- unwrapTypeConstructor ty
+           , mn == fromMaybe mn mn'


maybe True (== mn) mn' ?

I was a little confused there for a second :D There's also the more obscure all (== mn) mn' but I think that's too cute :D

I just copied exactly the form of the other matches here. There's some other cleaning up I'd like to do actually, but didn't want to mix it in this PR 😉 so I can change it, but maybe it'd be better to keep them consistent for now?

Should've looked at the whole file 🤦‍♂️

kritzcreek · 2018-01-20T22:57:52Z

src/Language/PureScript/Sugar/TypeClasses/Deriving.hs

+  | className == Qualified (Just dataOrd) (ProperName "Ord1")
+  = case tys of
+      [ty] | Just (Qualified mn' _, _) <- unwrapTypeConstructor ty
+           , mn == fromMaybe mn mn'


garyb · 2018-01-21T00:45:40Z

As evidenced by de9e5ac, this is actually a breaking change, since some things will require a constraint like Eq1 f rather than Eq (f a) when deriving now. I think that's fine though, since they're basically equivalent, and this is for 0.12 anyway.

kritzcreek · 2018-01-21T01:04:59Z

Do you think we can detect these cases and suggest the usage of Eq1?

garyb · 2018-01-21T09:45:22Z

I think the standard error message does a decent job of that anyway?

No type class instance was found for
                          
            Data.Eq.Eq1 f2

kritzcreek · 2018-01-21T13:50:39Z

If we can confirm that we can derive Eq1 for the type in question, I'd prefer us to add a hint to the error message:

The missing instance can be derived like:

`derive instance Eq1 TypeInQuestion`

garyb · 2018-01-21T14:04:19Z

The "type in question" is f a though, so we can't suggest deriving an instance for it like that.

The change is that now whenever attempting to derive Eq, if there's a type like (f _) in the current type, it will use eq1 for those values instead of eq, imposing an Eq1 f constraints rather than an Eq (f _) constraint as it would previously. It's just a case of juggling the constraints on the instance. If the _ is also a type variable, then it'll impose Eq _ too, or require that the type is Eq if it's concrete. For example:

Constructor	Original required constraints	New required constraints
`Foo (f a)`	`Eq (f a)`	`Eq1 f, Eq a`
`Bar (f Int)`	`Eq (f Int)`	`Eq1 f`

kritzcreek · 2018-01-21T14:16:05Z

Just thinking out loud here, does that mean that all our derived Eq instances that involve a type constructor get "slower" because of the indirection from Eq1 dictionary => Eq dictionary.

garyb · 2018-01-21T14:17:57Z

It doesn't apply to type constructors, only applied-type-variables. It's a pretty rare case most likely :)

kritzcreek · 2018-01-21T14:26:06Z

Well okay, if it's such a rare case I'm not too worried.

garyb · 2018-01-21T17:28:12Z

I'll merge this too if there are no other issues with it?

kritzcreek · 2018-01-21T17:45:58Z

Go for it! 🎉

garyb added 4 commits January 20, 2018 20:45

Allow Eq1 to be used when deriving Eq

4e75c67

Derive Eq1 (as eq1 = eq)

d4da7da

Allow Ord1 to be used when deriving Ord

d1e43fc

Derive Ord1 (as compare1 = compare)

60198af

garyb added the status: ready for review label Jan 20, 2018

kritzcreek approved these changes Jan 20, 2018

View reviewed changes

garyb added 2 commits January 20, 2018 23:07

Remove unnecessary constraints

3bebc61

Update DerivingFunctor test for Eq1 deriving

de9e5ac

garyb merged commit 4b23171 into purescript:master Jan 21, 2018

garyb deleted the eq1-deriving-again branch January 21, 2018 18:04

hdgarrood mentioned this pull request Jan 28, 2018

NonEmpty arrays purescript/purescript-arrays#127

Merged

garyb mentioned this pull request Apr 29, 2018

Bump version for v0.12.0-rc1 #3340

Merged

Conversation

garyb commented Jan 20, 2018

Uh oh!

garyb commented Jan 20, 2018

Uh oh!

kritzcreek left a comment

Choose a reason for hiding this comment

Uh oh!

kritzcreek Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

kritzcreek Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

kritzcreek Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

garyb Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

kritzcreek Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

kritzcreek Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

garyb commented Jan 21, 2018

Uh oh!

kritzcreek commented Jan 21, 2018

Uh oh!

garyb commented Jan 21, 2018

Uh oh!

kritzcreek commented Jan 21, 2018

Uh oh!

garyb commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kritzcreek commented Jan 21, 2018

Uh oh!

garyb commented Jan 21, 2018

Uh oh!

kritzcreek commented Jan 21, 2018

Uh oh!

garyb commented Jan 21, 2018

Uh oh!

kritzcreek commented Jan 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

garyb commented Jan 21, 2018 •

edited

Loading