Opened 3 years ago

Last modified 4 weeks ago

#13154 new bug

Standalone-derived anyclass instances aren't as permissive as empty instances

Reported by: RyanGlScott Owned by:
Priority: normal Milestone: 8.10.1
Component: Compiler (Type checker) Version: 8.1
Keywords: deriving Cc: dfeuer, Iceland_jack
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: GHC rejects valid program Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D4337, Phab:D4370
Wiki Page:

Description

Note that these are accepted:

class Foo (a :: TYPE ('TupleRep '[]))
instance Foo (a :: TYPE ('TupleRep '[]))
instance Foo (##)

But if you try to write these same instances using DeriveAnyClass and StandaloneDeriving, they'll be rejected for no good reason:

deriving instance Foo (a :: TYPE ('TupleRep '[]))

{-
    • Can't make a derived instance of ‘Foo a’:
        The last argument of the instance must be a data or newtype application
-}

deriving instance Foo (##)

{-
    • Can't make a derived instance of ‘Foo (# #)’:
        The last argument of the instance cannot be an unboxed tuple
-}

The latter error is a vestige of #12512, and should be easy enough to rectify. The former error is a bit tricker to fix, since much of the code in TcDeriv/TcDerivInfer assumes that the type to which the class is applied to in the instead head is a datatype or newtype TyCon. It's apparent that this doesn't hold true in the presence of DeriveAnyClass, however, so that code should be refactored to reflect this.

For practical reasons, however, it might be best to wait until after #12144 is fixed to tackle this one, since that splits out some logic in TcDerivInfer for DeriveAnyClass so as to make it no longer rely on the aforementioned TyCon invariant.

Change History (22)

comment:1 Changed 3 years ago by RyanGlScott

Keywords: Generics added

comment:2 Changed 3 years ago by RyanGlScott

Milestone: 8.4.1

Another thing that standalone DeriveAnyClass currently doesn't allow (but bare instances do) is the use of nullary classes. That is, this is allowed:

{-# LANGUAGE MultiParamTypeClasses #-}
module Allowed where

class C
instance C

but this is not:

{-# LANGUAGE DeriveAnyClass #-}
{-# LANGUAGE MultiParamTypeClasses #-}
{-# LANGUAGE StandaloneDeriving #-}
module Bug where

class C
deriving instance C
Bug.hs:7:1: error:
    • Cannot derive instances for nullary classes
    • In the stand-alone deriving instance for ‘C’

comment:3 Changed 3 years ago by RyanGlScott

Differential Rev(s): Phab:D3337
Status: newpatch

comment:4 Changed 3 years ago by dfeuer

I'm missing something: why is this a real problem? Adding a bunch of logic to fix it seems like overkill to me, personally.

comment:5 Changed 3 years ago by dfeuer

Cc: dfeuer added

comment:6 Changed 2 years ago by RyanGlScott

Keywords: deriving added; Generics removed

comment:7 Changed 22 months ago by bgamari

Milestone: 8.4.18.6.1

This ticket won't be resolved in 8.4; remilestoning for 8.6. Do holler if you are affected by this or would otherwise like to work on it.

comment:8 Changed 22 months ago by RyanGlScott

Another component of this issue which Iceland_jack noticed is that one cannot standalone-derive any instances for primitive types, among which include (->):

{-# LANGUAGE DataKinds #-}
{-# LANGUAGE DeriveAnyClass #-}
{-# LANGUAGE KindSignatures #-}
{-# LANGUAGE MagicHash #-}
{-# LANGUAGE StandaloneDeriving #-}
module Bug where

import GHC.Exts

class C1 a
deriving instance C1 (a -> b)

class C2 (a :: TYPE 'IntRep)
deriving instance C2 Int#
$ ghc/inplace/bin/ghc-stage2 Bug.hs
[1 of 1] Compiling Bug              ( Bug.hs, Bug.o )

Bug.hs:11:1: error:
    • Can't make a derived instance of ‘C1 (a -> b)’:
        The last argument of the instance must be a data or newtype application
    • In the stand-alone deriving instance for ‘C1 (a -> b)’
   |
11 | deriving instance C1 (a -> b)
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Bug.hs:14:1: error:
    • Can't make a derived instance of ‘C2 Int#’:
        The last argument of the instance must be a data or newtype application
    • In the stand-alone deriving instance for ‘C2 Int#’
   |
14 | deriving instance C2 Int#
   | ^^^^^^^^^^^^^^^^^^^^^^^^^

I'll lump this bug under this ticket.

comment:9 Changed 22 months ago by RyanGlScott

Differential Rev(s): Phab:D3337
Owner: RyanGlScott deleted
Status: patchnew

Removing Phab:D3337 as the Diff for this, since it's been abandoned.

comment:10 Changed 22 months ago by RyanGlScott

Owner: set to RyanGlScott

See also commits:

Which have made progress towards fixing this ticket (but on whose commit messages I forgot to mention it).

comment:11 Changed 22 months ago by Iceland_jack

Cc: Iceland_jack added
Differential Rev(s): Phab:D3337

comment:12 Changed 22 months ago by Iceland_jack

Differential Rev(s): Phab:D3337

comment:13 Changed 22 months ago by RyanGlScott

Differential Rev(s): Phab:D4337
Status: newpatch

Phab:D4337 fixes the issue in comment:8. It does not fix the entire issue, as there is still the degenerate case of:

{-# LANGUAGE DeriveAnyClass #-}
{-# LANGUAGE FlexibleInstances #-}
{-# LANGUAGE StandaloneDeriving #-}

class C a
deriving instance C a

Which needs further patching to fix.

comment:14 Changed 22 months ago by Ben Gamari <ben@…>

In 1a911f21/ghc:

Sequester deriving-related validity check into cond_stdOK

Currently, any standalone-derived instance must satisfy the
property that the tycon of the data type having an instance being
derived for it must be either a normal ADT tycon or a data family
tycon. But there are several other primitive tycons—such as `(->)`,
`Int#`, and others—which cannot have standalone-derived instances
(via the `anyclass` strategy) as a result of this check! See
https://ghc.haskell.org/trac/ghc/ticket/13154#comment:8 for an
example of where this overly conservative restriction bites.

Really, this validity check only makes sense in the context of
`stock` deriving, where we need the property that the tycon is that
of a normal ADT or a data family in order to inspect its data
constructors. Other deriving strategies don't require this validity
check, so the most sensible way to fix this error is to move the
logic of this check into `cond_stdOK`, which is specific to
`stock` deriving.

This makes progress towards fixing (but does not entirely fix)

Test Plan: make test TEST=T13154a

Reviewers: bgamari

Reviewed By: bgamari

Subscribers: rwbarton, thomie, carter

GHC Trac Issues: #13154

Differential Revision: https://phabricator.haskell.org/D4337

comment:15 Changed 22 months ago by RyanGlScott

Differential Rev(s): Phab:D4337Phab:D4337, Phab:D4370

Phab:D4370 is another slight refactor that's working towards fixing this ticket.

comment:16 Changed 21 months ago by Ryan Scott <ryan.gl.scott@…>

In f4336593/ghc:

Slight refactor of stock deriving internals

Summary:
Before, the `hasStockDeriving` function, which determines
how derived bindings should be generated for stock classes, was
awkwardly separated from the `checkSideConditions` function, which
checks invariants of the same classes that `hasStockDeriving` does.
As a result, there was a fair deal of hoopla needed to actually use
`hasStockDeriving`.

But this hoopla really isn't required—we should be using
`hasStockDeriving` from within `checkSideConditions`, since they're
looking up information about the same classes! By doing this, we can
eliminate some kludgy code in the form of `mk_eqn_stock'`, which had
an unreachable `pprPanic` that was stinking up the place.

Reviewers: bgamari, dfeuer

Reviewed By: bgamari

Subscribers: dfeuer, rwbarton, thomie, carter

GHC Trac Issues: #13154

Differential Revision: https://phabricator.haskell.org/D4370

comment:17 Changed 18 months ago by bgamari

Owner: RyanGlScott deleted
Status: patchnew

Is this now fixed, RyanGlScott?

comment:18 Changed 18 months ago by RyanGlScott

No. There is a smattering of things that still aren't supported, which include (but may not be limited to):

  • Instances for bare type variables:
instance Foo a -- Works
deriving anyclass instance Foo a -- Doesn't work
  • Instances for type-level literals:
instance Bar 1 -- Works
deriving anyclass instance Bar 2 -- Doesn't work
  • Empty instances:
instance Baz -- Works
deriving instance Baz -- Doesn't work

I have an idea of how to fix these, but haven't had the time to try it out yet.

comment:19 Changed 17 months ago by bgamari

This will not be addressed in GHC 8.6.

comment:20 Changed 17 months ago by bgamari

Milestone: 8.6.18.8.1

These will not be addressed in GHC 8.6.

comment:21 Changed 11 months ago by osa1

Milestone: 8.8.18.10.1

Bumping milestones of low-priority tickets.

comment:22 Changed 4 weeks ago by Marge Bot <ben+marge-bot@…>

In cd9b945/ghc:

Refactor TcDeriv to validity-check less in anyclass/via deriving (#13154)

Due to the way `DerivEnv` is currently structured, there is an
invariant that every derived instance must consist of a class applied
to a non-empty list of argument types, where the last argument *must*
be an application of a type constructor to some arguments. This works
for many cases, but there are also some design patterns in standalone
`anyclass`/`via` deriving that are made impossible due to enforcing
this invariant, as documented in #13154.

This fixes #13154 by refactoring `TcDeriv` and friends to perform
fewer validity checks when using the `anyclass` or `via` strategies.
The highlights are as followed:

* Five fields of `DerivEnv` have been factored out into a new
  `DerivInstTys` data type. These fields only make sense for
  instances that satisfy the invariant mentioned above, so
  `DerivInstTys` is now only used in `stock` and `newtype` deriving,
  but not in other deriving strategies.
* There is now a `Note [DerivEnv and DerivSpecMechanism]` describing
  the bullet point above in more detail, as well as explaining the
  exact requirements that each deriving strategy imposes.
* I've refactored `mkEqnHelp`'s call graph to be slightly less
  complicated. Instead of the previous `mkDataTypeEqn`/`mkNewTypeEqn`
  dichotomy, there is now a single entrypoint `mk_eqn`.
* Various bits of code were tweaked so as not to use fields that are
  specific to `DerivInstTys` so that they may be used by all deriving
  strategies, since not all deriving strategies use `DerivInstTys`.
Note: See TracTickets for help on using tickets.