Skip to content

Make {P,S}Eq less magical, don't make DefaultEq the default#458

Merged
RyanGlScott merged 1 commit intomasterfrom
make-eq-less-magical
Apr 29, 2020
Merged

Make {P,S}Eq less magical, don't make DefaultEq the default#458
RyanGlScott merged 1 commit intomasterfrom
make-eq-less-magical

Conversation

@RyanGlScott
Copy link
Copy Markdown
Collaborator

@RyanGlScott RyanGlScott commented Apr 28, 2020

Currently, the PEq and SEq classes are implemented by hand in
D.S.Prelude.Eq due to the default implementation, DefaultEq. But
as discussed in #457, this default isn't actually wise for most
singleton types. This patch removes DefaultEq as the default, which
allows PEq and SEq to be generated with Template Haskell, just
like other classes in the D.S.Prelude.* module namespace. As a
result, this fixes #457.

This refactoring led me to discover some other unusual treatment of
PEq and SEq. Namely, derived instances of PEq and SEq are
very magical in that they are handled entirely outside of
D.S.Partition.partitionDecs. But there isn't much of a good reason
to do so—we can just as well generate a normal Eq instance, then
promote and single it using the existing TH machinery. The only
magical thing about deriving Eq is that it also produces
SDecide/TestEquality/TestCoercion instances, but that can be
done independently of PEq and SEq.

This patch also refactors the treatment of deriving Eq to remove
the special casing for PEq and SEq:

  • The D.S.Promote.Eq and D.S.Single.Eq modules are now gone,
    having been subsumed by the new D.S.Deriving.Eq module, which
    generates an instance just like singletons would for any other
    stock-derivable class. What remains of D.S.Single.Eq is now
    located in D.S.Single.Decide, which only exists to generate
    instances of SDecide, TestEquality, and TestCoercion.
  • D.S.TH had strange functions singEqInstanceOnly and
    singEqInstancesOnly that generated instances of SEq, but not
    PEq. No other stock-derivable class had -Only variants like
    this, and given that singEqInstance{s} already exist, they were
    practically useless. I decided to take the opportunity to remove
    these warts.

goldfirere
goldfirere reviewed Apr 28, 2020
View reviewed changes
Copy link
Copy Markdown
Owner

@goldfirere goldfirere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

It does strike me that we could take all of this deriving code and move it either to th-desugar or another package, perhaps th-deriving. We're deriving very routing, non-singletons-specific instances -- and even with inferred constraints! Very cool stuff that others may want to hook into.

I'm not going to run off and do this, but it does seem like a nice realignment.

@RyanGlScott
Make {P,S}Eq less magical, don't make DefaultEq the default
c57d53a 
Currently, the `PEq` and `SEq` classes are implemented by hand in
`D.S.Prelude.Eq` due to the default implementation, `DefaultEq`. But
as discussed in #457, this default isn't actually wise for most
singleton types. This patch removes `DefaultEq` as the default, which
allows `PEq` and `SEq` to be generated with Template Haskell, just
like other classes in the `D.S.Prelude.*` module namespace. As a
result, this fixes #457.

This refactoring led me to discover some other unusual treatment of
`PEq` and `SEq`. Namely, derived instances of `PEq` and `SEq` are
very magical in that they are handled entirely outside of
`D.S.Partition.partitionDecs`. But there isn't much of a good reason
to do so—we can just as well generate a normal `Eq` instance, then
promote and single it using the existing TH machinery. The only
magical thing about deriving `Eq` is that it also produces
`SDecide`/`TestEquality`/`TestCoercion` instances, but that can be
done independently of `PEq` and `SEq`.

This patch also refactors the treatment of `deriving Eq` to remove
the special casing for `PEq` and `SEq`:

* The `D.S.Promote.Eq` and `D.S.Single.Eq` modules are now gone,
  having been subsumed by the new `D.S.Deriving.Eq` module, which
  generates an instance just like `singletons` would for any other
  `stock`-derivable class. What remains of `D.S.Single.Eq` is now
  located in `D.S.Single.Decide`, which only exists to generate
  instances of `SDecide`, `TestEquality`, and `TestCoercion`.
* `D.S.TH` had strange functions `singEqInstanceOnly` and
  `singEqInstancesOnly` that generated instances of `SEq`, but not
  `PEq`. No other `stock`-derivable class had `-Only` variants like
  this, and given that `singEqInstance{s}` already exist, they were
  practically useless. I decided to take the opportunity to remove
  these warts.
@RyanGlScott RyanGlScott force-pushed the make-eq-less-magical branch from 14380cc to c57d53a Compare April 28, 2020 15:31
@RyanGlScott
Copy link
Copy Markdown
Collaborator Author

RyanGlScott commented Apr 28, 2020

We're deriving very routing, non-singletons-specific instances -- and even with inferred constraints!

Almost, but there are some aspects to D.S.Deriving.* that are unique to singletons:

  1. Derived Eq instances generate more cases in singletons than in GHC. In GHC, a derived Eq instance for Bool would look (roughly) like this:

    instance Eq Bool where
  True  == True  = True
  False == False = True
  _     == _     = False

    In singletons, however, a derived Eq instance would need to match on every possile pair of constructors:

    instance Eq Bool where
  True  == True  = True
  False == False = True
  True  == False = False
  False == True  = False

    This is ultimately due to the need to work around Don't produce overlapped patterns th-desugar#6, which is very much a singletons-specific issue.

  2. While it's true that D.S.Deriving.* does infer constraints, the way it does so is rather ugly compared to GHC. singletons simply gathers up all of the field types, sticks the class name in front of each type, and makes that the context. This leads to gnarly looking instance contexts like this one:

    instance (Eq a, Eq [a]) => Eq [a] where ...

    This is acceptable for singletons' purposes, since users have long since accepted the fact that they'll need to enable UndecidableInstances in order to TH-generated code to typecheck. GHC's own deriving, on the other hand, makes an effort to avoid inferring exotic constraints like these.

FWIW, I do maintain a deriving-compat library whose functionality largely overlaps with D.S.Deriving.*. I don't think that it would be a drop-in replacement per se, but it does allow others to hook into it at the very least.

@RyanGlScott RyanGlScott merged commit 7a1c801 into master Apr 29, 2020
@RyanGlScott RyanGlScott deleted the make-eq-less-magical branch April 29, 2020 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@goldfirere goldfirere goldfirere left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Reconsider the default implementation of PEq.==

2 participants

@RyanGlScott @goldfirere