Implement deriving strategies
ClosedPublic

Authored by RyanGlScott on May 29 2016, 11:10 PM.

Details

Summary

Allows users to explicitly request which approach to deriving to use via
keywords, e.g.,

newtype Foo = Foo Bar
  deriving Eq
  deriving stock    Ord
  deriving newtype Show

Fixes Trac #10598.

Test Plan

./validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
arcpatch-D2280_2
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 11126
Build 13365: [GHC] Linux/amd64: Patch building
Build 13364: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Implement deriving strategies
  • Major rewrite
  • Bump haddock submodule
RyanGlScott edited edge metadata.

OK, I've rewritten the implementation to use the syntax-based approach proposed in the comments of the Trac ticket. Feedback is appreciated!

Also, here is the updated Haddock pull request.

I have looked over the changes to the syntax and the parser. It seems to me that illegal syntactic forms should be rejected in the parser rather than the renamer? Also, does -XDerivingStrategies imply -XDeriveAnyClass and/or -XGeneralisedNewtypeDeriving? It seems like it should.

I have looked over the changes to the syntax and the parser. It seems to me that illegal syntactic forms should be rejected in the parser rather than the renamer?

I deliberately didn't put these checks in the parser since I wanted illegal deriving forms to be caught in spliced-in Template Haskell ASTs as well. If they were checked only in the parser, then T10598_TH.hs would be allowed to compile without -XDerivingStrategies enabled.

Also, does -XDerivingStrategies imply -XDeriveAnyClass and/or -XGeneralisedNewtypeDeriving? It seems like it should.

No, for the same reason that -XDerivingStrategies doesn't imply all of -XDeriveFunctor, -XDeriveFoldable, -XDeriveTraversable, -XDeriveDataTypeable, -XDeriveGeneric, and -XDeriveLift. -XDerivingStrategies isn't intended to be a kitchen sink that turns on all exotic deriving forms, but instead just enables a syntactic form that gives you finer control of what choices to make when deriving. If you do choose to use an exotic deriving form with -XDervingStrategies, then you must enable the corresponding GHC extension.

A practical consequence of making -XDerivingStrategies enable both -XDeriveAnyClass and -XGeneralizedNewtypeDeriving would be how this code compiles:

{-# LANGUAGE GeneralizedNewtypeDeriving #-}
newtype T = MkT S deriving (Foo, Bar)

This code compiles just fine, and uses GeneralizedNewtypeDeriving to derive Foo and Bar. But if you turn on DerivingStrategies as well, suddenly the above code will emit a warning about both`GeneralizedNewtypeDeriving` and DeriveAnyClass being on, and defaulting to DeriveAnyClass! This goes against what I want DerivingStrategies to accomplish: it should allow strictly more code to compile, and it shouldn't change the semantics of any existing code.

Having DerivingStrategies enable GeneralizedNewtypeDeriving would also have Safe Haskell repercussions, since you can't use Safe in combination with GeneralizedNewtypeDeriving.

RyanGlScott updated this revision to Diff 7971.Jun 15 2016, 8:39 AM
  • Update parser conflict numbers
simonpj accepted this revision.Jun 20 2016, 6:08 AM
simonpj edited edge metadata.

I have not reviewed every line, but this looks thorough to me. Thanks!

compiler/basicTypes/BasicTypes.hs
491

Worth saying here that DerivBulitin only works for a handful of built-in classes. It mitght be worth even listing them for clarity (at least with an e.g.) , including Functor (needs DerivingFunctor), Traversable, etc.

RyanGlScott updated this revision to Diff 8015.Jun 20 2016, 8:39 AM
RyanGlScott edited edge metadata.
  • Clarify the documentation for DerivBuiltin
RyanGlScott marked an inline comment as done.Jun 20 2016, 8:39 AM
RyanGlScott updated this revision to Diff 8055.Jun 24 2016, 1:07 PM
RyanGlScott edited edge metadata.
  • Microoptimization: use lengthExceeds instead of (> length)
RyanGlScott edited edge metadata.
  • Post-rebase cleanup

@alanz, does the API annotation-related code look OK?

alanz edited edge metadata.Jul 13 2016, 10:56 AM

A quick glance over the API Annotations does not show anything obvious.

I suggest adding some tests based on the utils/check-api-annotations tool in the GHC source tree, there is a README with it, and make sure you have examples of all the new syntax in it.

compiler/parser/Parser.y
953

In all three annotations above, you can use mj instead of mu, there are no unicode variants for any of the three keywords. Unless of course there are.

RyanGlScott updated this revision to Diff 8198.Jul 13 2016, 2:21 PM
RyanGlScott edited edge metadata.
  • Post-rebase cleanup
RyanGlScott marked an inline comment as done.Jul 13 2016, 2:22 PM

OK, I've added a test for deriving strategies' API annotations.

Does anyone else have any feedback? Otherwise, this should be ready to land.

alanz accepted this revision.Jul 13 2016, 3:22 PM
alanz edited edge metadata.

Annotations look good, on the face of it. Thanks.

simonpj accepted this revision.Jul 20 2016, 7:04 AM
simonpj edited edge metadata.

Looks good!

compiler/basicTypes/BasicTypes.hs
489

Refer to Note [Deriving strategies] in Tcderiv

compiler/hsSyn/HsDecls.hs
1048

Refer to Note [Deriving strategies] in Tcderiv

compiler/typecheck/TcDeriv.hs
1031

Push the rhs into a mk_eqn_something function, for symmetry with the others

1822

Why does mk_data_eqn not work here any more?

2447

I'd use a pattern guard

| DerivSpecNewtype rhs_ty <- mechanims
= ...

| otherwise
= ....

less indentation, clearer cases.

2567

Point to the wiki page, and Trac ticket

RyanGlScott planned changes to this revision.Aug 2 2016, 5:59 PM

This still needs to be updated following the discussion from https://mail.haskell.org/pipermail/ghc-devs/2016-July/012442.html, and also to address Simon's comments.

RyanGlScott updated this revision to Diff 8373.Aug 3 2016, 2:30 PM
RyanGlScott marked 5 inline comments as done.
RyanGlScott edited edge metadata.
  • Fix the program reported in Trac #10598
  • Rename 'builtin' to 'bespoke'
RyanGlScott added inline comments.Aug 3 2016, 2:32 PM
compiler/typecheck/TcDeriv.hs
1822

I'm not sure I understand your question. mk_data_eqn simply moved to the definition of go_for_it_other (whereas this is gor_for_it_gnd, a separate piece of code that I factored out so as to be able to use it multiple times in various places in mkNewTypeEqn).

RyanGlScott updated this object.Aug 3 2016, 2:33 PM
RyanGlScott edited edge metadata.
RyanGlScott updated this revision to Diff 8374.Aug 3 2016, 2:46 PM
  • Update parser numbers
oerjan added a comment.Aug 3 2016, 6:23 PM

The ghc-devs discussion gave me some thoughts relevant to this differential. I put them as a comment in Trac #10598.

RyanGlScott updated this revision to Diff 8383.Aug 5 2016, 8:35 AM
RyanGlScott edited edge metadata.
  • Add oerjan's test case
RyanGlScott updated this revision to Diff 8482.Aug 25 2016, 5:45 PM
RyanGlScott edited edge metadata.

Rebase on top of master

RyanGlScott updated this revision to Diff 8805.Sep 27 2016, 3:03 PM
RyanGlScott edited edge metadata.
  • Rebase on top of master
RyanGlScott edited edge metadata.
  • Rename 'bespoke' to 'stock'
RyanGlScott updated this object.Sep 29 2016, 8:09 AM
RyanGlScott edited edge metadata.
simonpj accepted this revision.Sep 29 2016, 8:52 AM
simonpj edited edge metadata.

Great. Go for it.

Simon

PS: in a subsequent commit, refactoring TcDeriv/TcGenDeriv into more modules would be good. They have both grown very large.

bgamari accepted this revision.Sep 30 2016, 1:45 PM
bgamari edited edge metadata.

Looks good to me. A couple of comments inline. It would be great if you could add some discussion to the 8.2 Migration page.

compiler/hsSyn/HsDecls.hs
1062

Typo (producd).

1532

Thanks for keeping this up to date.

compiler/typecheck/TcDeriv.hs
2479

Is this documented in the users guide?

This revision is now accepted and ready to land.Sep 30 2016, 1:45 PM
RyanGlScott updated this revision to Diff 8825.Sep 30 2016, 3:49 PM
RyanGlScott marked 3 inline comments as done.
RyanGlScott edited edge metadata.
  • Ben's suggestions

Thanks Ryan!

This revision was automatically updated to reflect the committed changes.