Implement #5462 (deriving clause for arbitrary classes)
ClosedPublic

Authored by dreixel on Nov 14 2014, 4:45 PM.

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.
hvr updated this revision to Diff 1443.Nov 14 2014, 4:45 PM
hvr retitled this revision from to Implement #5462 (deriving clause for arbitrary classes).
hvr updated this object.
hvr edited the test plan for this revision. (Show Details)
hvr added reviewers: austin, simonpj.
hvr updated the Trac tickets for this revision.
hvr added a subscriber: dreixel.
dreixel commandeered this revision.Nov 14 2014, 4:57 PM
dreixel added a reviewer: hvr.
hvr edited edge metadata.Nov 15 2014, 5:47 AM

Fyi, I just tried this against the just updated in-tree deepseq package, and the following simple example works just fine:

{-# LANGUAGE DeriveGeneric, DeriveAnyClass #-}

import GHC.Generics (Generic)
import Control.DeepSeq

data Foo a = Foo0 a String
           | Foo1
           deriving (Eq, Generic, NFData)

data Colour = Red | Green | Blue
            deriving (Generic, NFData)

data Void deriving (Generic, NFData)

f :: Foo Colour -> ()
f = rnf
simonpj accepted this revision.Nov 17 2014, 4:19 PM
simonpj edited edge metadata.

Generally fine. I've added quite a few comments/questions

Simon

compiler/typecheck/TcDeriv.lhs
1006

I'd write out the two cases, rather than comment it. The RHS is so small!

1117

What's going on here? How is it related to DeriveAnyClass? Surely worth at least a careful Note and an example?

1175

Trivial: we don't usually add a blank line after the ~~~~~~~~

1199

Ah this is where the functor-like thing kicks in. Link to this Note from the fuctor_like comment. Also can you give an example here. (Maybe even worth making it a separate Note [Context for a DeriveAnyClass deriving].)

You say there's a rule for * and one for (* -> *) but actually it's one rule for (* -> *) and one for everything else. Or is DeriveAnyClass restricted to kinds * and (* -> *)? If so that needs documenting. If not, what happens in the higher kinded (or polykinded) situations?

1537

The logic of this test, and the one that follows on lines 1570ff, is far from obvious. (It wasn't clear before, but it's worse now.) Could you add a Note to explain, with examples, when we pick which alternatives?

1586

Delete

compiler/typecheck/TcGenDeriv.lhs
110

If we get here we KNOW that canDeriveAnyClass returns Nothing. So make it an ASSERT, and drop the panic equation next. No sense in testing things we know to be true, except in asertion-checking mode.

142

This comment belongs at the top, as the spec of the functoin

This revision is now accepted and ready to land.Nov 17 2014, 4:19 PM
dreixel updated this revision to Diff 1487.EditedNov 18 2014, 3:54 AM
dreixel edited edge metadata.

Whitespace changes, plus address some of Simon's remarks.

dreixel updated this revision to Diff 1490.Nov 18 2014, 5:11 AM
  • Fix two mistakes
austin accepted this revision.Nov 20 2014, 10:34 PM
austin edited edge metadata.

LGTM.

Closed by commit rGHC7ed482d90955: Implement #5462 (deriving clause for arbitrary classes) (authored by Jose Pedro Magalhaes <jpm@cs.ox.ac.uk>, committed by austin). · Explain WhyNov 20 2014, 10:45 PM
This revision was automatically updated to reflect the committed changes.
hvr added a subscriber: nomeata.EditedNov 21 2014, 5:04 AM

jfyi, @nomeata reworded the [Deriving any class] note at rGHCb0dd34756613210873ed940d58027f61a492aeda