DerivingVia (and perhaps even GND) works badly with DeriveGeneric

DerivingVia together with DeriveGeneric can generate wrong instances for Generic.

Consider the following:

{-# LANGUAGE DeriveGeneric, DerivingStrategies, DerivingVia, GADTs          #-}
{-# LANGUAGE GeneralizedNewtypeDeriving, TypeFamilies, UndecidableInstances #-}
module Data.Foldable.Bad where
import GHC.Generics

newtype Bad a = Bad a deriving (Generic)
data Foo = Foo Int
  deriving (Read, Show, Eq, Ord)
  deriving (Generic) via Bad Foo

which gives the following representation, which is considered to be wrong for Foo:

ghci> from $ Foo 12
M1 {unM1 = M1 {unM1 = M1 {unM1 = K1 {unK1 = Foo 12}}}}
ghci> :t it
it
  :: D1
       ('MetaData "Bad" "Data.Foldable.Bad" "main" 'True)
       (C1
          ('MetaCons "Bad" 'PrefixI 'False)
          (S1
             ('MetaSel
                'Nothing 'NoSourceUnpackedness 'NoSourceStrictness 'DecidedLazy)
             (Rec0 Foo)))
       x

Also, DerivingStrategies + GND + DeriveGeneric already can generate wrong instance:

newtype Bad2 = Bad2 Bool
  deriving newtype (Generic)

{-

ghci> from $ Bad2 False
M1 {unM1 = L1 (M1 {unM1 = U1})}
ghci> :t it
it
  :: D1
       ('MetaData "Bool" "GHC.Types" "ghc-prim" 'False)
       (C1 ('MetaCons "False" 'PrefixI 'False) U1
        :+: C1 ('MetaCons "True" 'PrefixI 'False) U1)
       x
-}

I tested this against GHC 8.6.1-alpha1.

Trac metadata

Trac field	Value
Version
Type	Bug
TypeOfFailure	OtherFailure
Priority	normal
Resolution	Unresolved
Component	Compiler
Test case
Differential revisions
BlockedBy
Related
Blocking
CC
Operating system
Architecture

changed milestone to %8.6.1

changed weight to 5

added Tbug Trac import labels

I'm not sure what the bug is here. You're explicitly requesting that you reuse the Generic instance for Bad Foo, and lo and behold, GHC gives you it. If you don't like this behavior, don't combine DerivingVia with Generic—the former wasn't meant to be combined with the latter (indeed, this code will give an error if combined with the Safe extension).

added deriving label

You're explicitly requesting that you reuse the Generic instance for Bad Foo, and lo and behold, GHC gives you it.

I can't imagine the situation where Generic should give the different information than its true structure.

Although the code above is simple enough, I think it might be error-prone to allow writing Generic in DerivingVia-clause. As for GND, deriving strategy newtype is used as a prefix for the deriving clause, so one can easily notice the mistake; but as for DerivingVia, since via-clause is placed after constraints, one can easily overlook that the current deriving strategy.

this code will give an error if combined with the Safe extension

Hence, I think this behaviour should be enabled by default, even without Safe extension.

As a counterpoint, I quite dislike the idea of GHC demanding that users write instances a certain way. In fact, one of the very reasons I authored DerivingStrategies in the first place was because one couldn't derive classes like Read or Show by any means except stock deriving. True, 90% of the time, this is what you want, but for the other 10%, having explicit control through DerivingStrategies is quite handy.

Replying to [ticket:15434#comment:157314 konn]:

I can't imagine the situation where Generic should give the different information than its true structure.

There are folks out there who write Generic instances which differ from what stock deriving would give you for various reasons. One reason is that some people like to define data types abstractly and only allow creating/matching on values of that data type through pattern synonyms.

While I don't have any examples of people newtype/via-deriving Generic instances on hand, it's not inconceivable that folks might want to do this.

Hence, I think this behaviour should be enabled by default, even without Safe extension.

I used to share this opinion, but I no longer do. I don't think GHC (or Safe) should be in the business of enforcing the structure of Generic instances. You'll never segfault from using a hand-written Generic instance (unlike, say, Typeable).

I think what you want is for GHC to be able to verify that a particular Generic instance's structure actually matches the data type it was derived for. This is a reasonable desire, since there are certain properties about data types that can easily be discerned from a Rep instance, but only if you have confidence that the Rep instance isn't "lying". dfeuer (cc'd) has toyed with some ideas for how this could be achieved in GHC—perhaps he could chime in here.

Replying to [ticket:15434#comment:157326 RyanGlScott]:

True, 90% of the time, this is what you want, but for the other 10%, having explicit control through DerivingStrategies is quite handy.

I agree. In general, it is desirable to have the flexibility on the choice of a deriving strategy.

There are folks out there who write Generic instances which differ from what stock deriving would give you for various reasons. One reason is that some people like to define data types abstractly and only allow creating/matching on values of that data type through pattern synonyms.

I've just taken a simple survey searching instance Generic in codes on GitHub and count how many custom instances for Generic are defined, using this code. It reported that there are 2 out of 892 modules with custom Generic instance and have 12 custom instances as a total; it turns out that Generic instances in one of the modules are not related to GHC's Generics. One instance is something like mapping the Rep of Map k v to that of [(k, v)]. Since there are some modules that cannot be parsed by haskell-src-exts and not counted in my survey, there might be more custom instances. Anyway, there is at least one use-case and there might be other custom instances doing the similar things.

I think what you want is for GHC to be able to verify that a particular Generic instance's structure actually matches the data type it was derived for. This is a reasonable desire, since there are certain properties about data types that can easily be discerned from a Rep instance, but only if you have confidence that the Rep instance isn't "lying". dfeuer (cc'd) has toyed with some ideas for how this could be achieved in GHC—perhaps he could chime in here.

I find this option is reasonable. If GHC can warn that one can avoid unintended derived instance, and if one really wants to write custom instance, then one can turn off that warning.

changed milestone to %8.8.1

These won't be fixed for in GHC 8.6.

changed milestone to %8.10.1

Bumping milestones of low-priority tickets.

added Pnormal label

changed milestone to %9.0.1

changed milestone to %9.2.1

This will not be fixed in time for %9.2.1

changed milestone to %9.4.1

I agree with Ryan's original analysis that this isn't a bug.

closed