Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#12768 closed bug (fixed)

8.0.2 derives invalid code when class method is constrained by itself

Reported by: jophish Owned by:
Priority: normal Milestone: 8.0.2
Component: Compiler Version: 8.1
Keywords: deriving Cc: simonpj, bgamari
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: GHC rejects valid program Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

GHC 8.0.1 is able to compile this without a problem and doesn't require FlexibleContexts.

{-# LANGUAGE GeneralizedNewtypeDeriving #-}
{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE ConstrainedClassMethods #-}

module A where

class C m where
  foo :: C m => m ()

newtype N m a = N (m a)
  deriving C

Compare the output of 8.0.1, 8.0.2 and 8.1. I turned on -fdefer-type-errors in order for -ddump-deriv to work.

$ ghc --version
The Glorious Glasgow Haskell Compilation System, version 8.0.1

$ ghc -ddump-deriv A.hs
[1 of 1] Compiling A                ( A.hs, A.o )

==================== Derived instances ====================
Derived instances:
  instance A.C m_aNK => A.C (A.N m_aNK) where
    A.foo = GHC.Prim.coerce (A.foo :: m_ap1 ()) :: A.N m_ap1 ()


GHC.Generics representation types:
$ ghc --version
The Glorious Glasgow Haskell Compilation System, version 8.0.2

$ ghc A.hs
[1 of 1] Compiling A                ( A.hs, A.o )

A.hs:10:12: error:
    • Couldn't match type ‘m’ with ‘N m’
        arising from the coercion of the method ‘foo’
          from type ‘C m => m ()’ to type ‘C (N m) => N m ()’
      ‘m’ is a rigid type variable bound by
        the deriving clause for ‘C (N m)’ at A.hs:10:12
    • When deriving the instance for (C (N m))

$ ghc -ddump-deriv -fdefer-type-errors A.hs
[1 of 1] Compiling A                ( A.hs, A.o )

==================== Derived instances ====================
Derived instances:
  instance A.C m_awm => A.C (A.N m_awm) where
    A.foo
      = GHC.Prim.coerce
          @(A.C m_ap0 => m_ap0 ()) @(A.C (A.N m_ap0) => A.N m_ap0 ()) A.foo


GHC.Generics representation types:



A.hs:11:12: warning: [-Wdeferred-type-errors]
    • Couldn't match type ‘m’ with ‘N m’
        arising from a use of ‘GHC.Prim.coerce’
      ‘m’ is a rigid type variable bound by
        the instance declaration at A.hs:11:12
    • In the expression:
        GHC.Prim.coerce @(C m => m ()) @(C (N m) => N m ()) foo
      In an equation for ‘foo’:
          foo = GHC.Prim.coerce @(C m => m ()) @(C (N m) => N m ()) foo
      When typechecking the code for ‘foo’
        in a derived instance for ‘C (N m)’:
        To see the code I am typechecking, use -ddump-deriv
      In the instance declaration for ‘C (N m)’
    • Relevant bindings include foo :: N m () (bound at A.hs:11:12)

There's no '8.0.2' version to report this against so I've chosen 8.1. GHC 8.1 gives very similar results:

$ ghc --version
The Glorious Glasgow Haskell Compilation System, version 8.1.20160930

$ ghc A.hs
[1 of 1] Compiling A                ( A.hs, A.o )

A.hs:11:12: error:
    • Couldn't match type ‘m’ with ‘N m’
        arising from the coercion of the method ‘foo’
          from type ‘C m => m ()’ to type ‘C (N m) => N m ()’
      ‘m’ is a rigid type variable bound by
        the deriving clause for ‘C (N m)’ at A.hs:11:12
    • When deriving the instance for (C (N m))

$ ghc -ddump-deriv -fdefer-type-errors A.hs
[1 of 1] Compiling A                ( A.hs, A.o )

==================== Derived instances ====================
Derived instances:
  instance A.C m_awK => A.C (A.N m_awK) where
    A.foo
      = GHC.Prim.coerce
          @(A.C m_app => m_app ()) @(A.C (A.N m_app) => A.N m_app ()) A.foo


GHC.Generics representation types:



A.hs:11:12: warning: [-Wsimplifiable-class-constraints]
    The constraint ‘C (N m)’ matches an instance declaration
    instance C m => C (N m) -- Defined at A.hs:11:12
    This makes type inference for inner bindings fragile;
      either use MonoLocalBinds, or simplify it using the instance

A.hs:11:12: warning: [-Wdeferred-type-errors]
    • Couldn't match type ‘m’ with ‘N m’
        arising from a use of ‘GHC.Prim.coerce’
      ‘m’ is a rigid type variable bound by
        the instance declaration at A.hs:11:12
    • In the expression:
        GHC.Prim.coerce @(C m => m ()) @(C (N m) => N m ()) foo
      In an equation for ‘foo’:
          foo = GHC.Prim.coerce @(C m => m ()) @(C (N m) => N m ()) foo
      When typechecking the code for ‘foo’
        in a derived instance for ‘C (N m)’:
        To see the code I am typechecking, use -ddump-deriv
      In the instance declaration for ‘C (N m)’
    • Relevant bindings include foo :: N m () (bound at A.hs:11:12)

Attachments (1)

A.hs (211 bytes) - added by jophish 3 years ago.
Minimal testcase

Download all attachments as: .zip

Change History (15)

Changed 3 years ago by jophish

Attachment: A.hs added

Minimal testcase

comment:1 Changed 3 years ago by rwbarton

Can't you just delete the C m => constraint on foo? Is this reduced from a real program?

comment:2 Changed 3 years ago by jophish

Yes, as 8.1 suggests, this constraint is redundant.

I encountered this in the trifecta library. It can be seen in the rend and restOfLine member functions of the DeltaParsing class here https://hackage.haskell.org/package/trifecta-1.6/docs/src/Text.Trifecta.Combinators.html#DeltaParsing (this is fixed in trifecta's HEAD now).

I imagine that this can happen when a top level constrained function is moved to be a member of a class.

comment:3 Changed 3 years ago by RyanGlScott

Cc: simonpj added
Milestone: 8.0.2

Simon, the code above stopped typechecking after one of your commits: 96d451450923a80b043b5314c5eaaa9d0eab7c56. Is this expected behavior?

comment:4 Changed 3 years ago by simonpj

There is a real difficulty here, if I'm not mistaken. Consider

class C a where
  op :: D a => a -> a

instance C [a] where
  op = opList

opList :: D [a] => [a] -> [a]
opList = ...

Now suppose we try GND:

newtype N a = MkN [a] deriving( C )

The GND is expecting to get an implementation of op for N from that for opList, something like

instance C (N a) where
  op = opN

opN :: D (N a) => N a -> N a
opN = ...opList...   -- Uses coerce

But the call to opList in the definition of opN needs (D [a]) whereas what we are given is D (N a). And these are not inter-coercible. For example we might have manually written instances

instance D [a] where ...
instance D (N a) where ...

and there is no relation between them. So in this case, the code for opN is not representationally the same as the code for opList, so GND (which is all about representational equivalence) doesn't really apply.

In the actual example, D is just C again, but I don't think we should treat it any differently.

The actual error is generated by an attempt to coerce

coerce @(C m => m ()) @(C (N m) => N m ())

which rightly fails.

So I think GND just doesn't work here.

Last edited 3 years ago by simonpj (previous) (diff)

comment:5 Changed 3 years ago by RyanGlScott

Cc: bgamari added

That's a very good point about D [a] and D (N a) not being inter-coercible in general w.r.t the class C. It sound like in order to make this work for the special case of D = C, we'd have to equip GHC with some special knowledge that the C (N a) instance was generated through GND, which sounds quite gross.

So now the question becomes: should we avoid backporting 96d451450923a80b043b5314c5eaaa9d0eab7c56 to 8.0.2 since it causes some programs which compile in 8.0.1 to fail in 8.0.2? Or should we simply add a section to the release notes explaining the scenario, and recommend the (admittedly simple) workaround?

Ben, do you have any thoughts on this?

comment:6 Changed 3 years ago by simonpj

Well I guess that

  • if we are doing GND for (C (N a)), where N is a newtype
  • then (C a) is representationally equal to (C (N a))

and that's just what we need here. But I don't see an easy way to incorporate that knowledge in the solver.

And this is very much a weird case: the (C m) constraint is entirely redundant.

I worked out how GHC 8.0 is working. We generate this:

instance C m => C (N m) where
  foo = coerce (foo :: m ()) :: N (m ())

From this instance decl we generate

dfCN :: C m => C (N m)
dfCN d = MkC ($cfooN d)

$cfooN :: C m => C (N m) => N m ()
$cfooN d _ = foo d d |> (a coercion)

Notice that in $cfooN we don't need the second dictionary argument (corresponding to the C m constrain in foo's signature in the class decl). It just so happens that the instance context can satisfy it.

But exploiting this fluke involves instantiating the call to foo, which is what I am now not doing.

In comment:4, the fluke would require that from C a (the context of the instance decl) we could prove D a (the requirement of the call in the method body), again ignoring the supplied (D (N a)).

I think we can probably change the code to exploit the same fluke; but I'm a bit disinclined to do so, until we get actual user complaints. It would make it harder to explain when GND does and does not work.

Last edited 3 years ago by simonpj (previous) (diff)

comment:7 Changed 3 years ago by RyanGlScott

I'm disinclined to fix this, too, for the reasons you articulated.

The only place I'm aware of at the moment that exploited this trick was in the trifecta library, and, as jophish noted in comment:2, the HEAD version of trifecta has already been patched to avoid this bug. But there could be other occurrences lurking in the wild, too... we'd probably need a build of Stackage with 8.0.2 to be sure.

comment:8 Changed 3 years ago by Simon Peyton Jones <simonpj@…>

In e43f05b6/ghc:

Add comments from Trac #12768

comment:9 Changed 3 years ago by Ryan Scott <ryan.gl.scott@…>

In ead83db/ghc:

Describe symptoms of (and the cure for) #12768 in 8.0.2 release notes

GHC 8.0.2 introduced a bugfix involving GeneralizedNewtypeDeriving in
96d451450923a80b043b5314c5eaaa9d0eab7c56. This made typechecking of
GND-produced code a bit stricter, and an unfortunate side effect of this was
that there were a couple of corner-case programs that stopped compiling
when transitioning from GHC 8.0.1 to 8.0.2.

Since the number of affected programs seems quite small, and since the fix
is so straightforward, we opt to simply note this discrepancy in the 8.0.2
release notes.

Resolves #12768.

comment:10 Changed 3 years ago by RyanGlScott

Resolution: fixed
Status: newclosed

After talking with bgamari about this, I've opted to simply note the difference in behavior for this program in the 8.0.2 release notes.

comment:11 Changed 3 years ago by RyanGlScott

Status: closedmerge

comment:12 Changed 3 years ago by bgamari

Status: mergeclosed

comment:13 Changed 3 years ago by simonpj

See #13293 for another example of the same issue.

comment:14 Changed 2 years ago by RyanGlScott

Keywords: deriving added
Note: See TracTickets for help on using tickets.