Derive instances in Data.Data
ClosedPublic

Authored by RyanGlScott on Jun 1 2016, 2:48 PM.

Details

Summary

Currently, none of the Data instances in Data.Data are derived,
which has resulted in hundreds of lines of laboriously hand-written Data
instances. This cleans it up by using DeriveDataTypeable to derive all of
the boring instances.

Note that previously, tcTopSrcDecls in TcRnDriver was typechecking the
variables generated in deriving statements before other top-level variables,
which causes an error when DeriveDataTypeable is used in Data.Data, since
the deriving-generated variable definitions refer to top-level definitions
in Data.Data itself. To fix this, the order in which these two groups are
typechecked was reversed.

Test Plan

./validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
RyanGlScott retitled this revision from to Derive instances in Data.Data.Jun 1 2016, 2:48 PM
RyanGlScott updated this object.
RyanGlScott edited the test plan for this revision. (Show Details)
RyanGlScott added reviewers: hvr, bgamari, austin.
austin requested changes to this revision.EditedJun 1 2016, 2:52 PM

Big win - but please put a changelog note about the constraint removal for Ratio - it can cause people warnings with the new redundant constraint machinery when people upgrade, so probably worth pointing out.

This revision now requires changes to proceed.Jun 1 2016, 2:52 PM
RyanGlScott updated this revision to Diff 7807.Jun 1 2016, 2:53 PM
  • Changelog note
rwbarton requested changes to this revision.Jun 1 2016, 2:57 PM
rwbarton added a reviewer: rwbarton.
rwbarton added a subscriber: rwbarton.
rwbarton added inline comments.
libraries/base/Data/Data.hs
1068

No, this was specifically discussed in some mailing list thread and it was decided to use a hand-written instance rather than the derived instance which breaks the abstraction barrier (you can use the derived Data instance to construct a non-reduced Ratio a value).

libraries/base/changelog.md
12–13 ↗(On Diff #7807)

And remove this too.

This revision now requires changes to proceed.Jun 1 2016, 2:57 PM
austin requested changes to this revision.Jun 1 2016, 3:04 PM

Okay, @rwbarton with the save as usual. But numeric/should_run/T10011.hs didn't catch this, which is clearly my fault somewhere, so we should look at that, too.

libraries/base/Data/Data.hs
1068

I forgot about this. For reference, this is Trac #10011.

rwbarton added inline comments.Jun 1 2016, 3:08 PM
libraries/base/Data/Data.hs
1068

Also, this would be a good place to have a comment to that effect (and maybe a link to the thread).

RyanGlScott updated this revision to Diff 7808.Jun 1 2016, 5:49 PM
RyanGlScott updated this object.Jun 1 2016, 5:50 PM
RyanGlScott marked 3 inline comments as done.Jun 1 2016, 5:52 PM

Actually, numeric/should_run/T10011.hs would have caught that, but I was too careless to bother running the full testsuite before submitting. My bad.

libraries/base/Data/Data.hs
1068

Ack, good catch. I've reverted back to the previous instance and added a comment explaining why it's implemented the way it is.

RyanGlScott updated this revision to Diff 7809.Jun 1 2016, 5:55 PM
RyanGlScott marked an inline comment as done.
  • Add comment in TcRnDriver
austin accepted this revision.Jun 3 2016, 11:01 AM

parrot of approval

parrot

This revision was automatically updated to reflect the committed changes.