Opened 19 months ago

Closed 19 months ago

Last modified 19 months ago

#15125 closed bug (duplicate)

Typeclass instance selection depends on the optimisation level

Reported by: nicuveo Owned by:
Priority: normal Milestone: 8.6.1
Component: Compiler Version: 8.2.2
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: #14434 Differential Rev(s):
Wiki Page:

Description (last modified by nicuveo)

(See the attached files for a minimal case.)

A file A defines a typeclass, and gives an incoherent instance for all types a, and exports a function relying on said typeclass. A file B defines some data types, makes them specific instances of that class, and uses the function defined in A. Which instance ends up being picked for B depends on the optimisation level those files are compiled with.

A.hs

class A a where
  someValue :: a -> Maybe Int

instance {-# INCOHERENT #-} A a where
  someValue = const Nothing

getInt :: A a => a -> Int
getInt x = fromMaybe 0 $ someValue x

B.hs

data B = B Int

instance A B where
  someValue (B x) = Just x

getBInt :: Int
getBInt = getInt $ B 42

Main.hs

main = print getBInt

Upon compiling with -O0, this prints 42; upon compiling with -O2, this prints 0.

(Interestingly, if the redundant class constraint is removed from getInt, then it always prints 0.)

Attachments (1)

poc.tar.bz2 (933 bytes) - added by nicuveo 19 months ago.
A small proof of concept, with a script showing how to replicate the issue.

Download all attachments as: .zip

Change History (12)

Changed 19 months ago by nicuveo

Attachment: poc.tar.bz2 added

A small proof of concept, with a script showing how to replicate the issue.

comment:1 Changed 19 months ago by nicuveo

Description: modified (diff)

comment:2 Changed 19 months ago by nicuveo

I tested with 8.4.2: it prints 0 in both cases. This seems inconsistent with the documented behaviour of incoherent instances?

comment:3 Changed 19 months ago by AntC

This seems inconsistent with the ​documented behaviour of incoherent instances?

The naming is deliberate: only use INCOHERENT to diagnose behaviour you don't understand; never rely on ghc consistently picking an instance.

The behaviour you're seeing in 8.4.2 is following the documentation. The critical part of that doco is

Eliminate any candidate IX for which both of the following hold:

The second sub-bullet doesn't hold, because neither instance is marked overlapping/overlappable.

So I'd expect to always pick the more general instance, marked INCOHERENT.

Then I agree the optimisation level shouldn't influence instance selection. I guess that in optimising for module B, ghc forces the instance selection. Never the less, just don't use INCOHERENT.

comment:4 Changed 19 months ago by nicuveo

I do agree with you on never relying on INCOHERENT! I beg to disagree on the interpretation of the documentation, however. Sure, the INCOHERENT instance does not get eliminated, but the very next bullet point reads:

If exactly one non-incoherent candidate remains, select it. If all remaining candidates are incoherent, select an arbitrary one. Otherwise the search fails (i.e. when more than one surviving candidate is not incoherent).

Since, out of the two candidates that remain, only one is not incoherent, the compiler should only pick that one. My understanding is therefore that this code should always print 42?

EDIT: the behaviour is still the same (on 8.2.2) if either A is marked as OVERLAPPABLE instead of INCOHERENT or if A is unmarked and B is OVERLAPPING; in both cases none of the instances are INCOHERENT.

Last edited 19 months ago by nicuveo (previous) (diff)

comment:5 Changed 19 months ago by simonpj

I investigated. Turns out that this is a near-dup of #14434 -- a bug in the "short-cut solver" for type classes. Happily, it was fixed six months ago, and the fix is in 8.4.

I also agree with comment:4.

So I'll just close this. I don't think it's worth a new regression test.

If anyone reading this has a concrete suggestion for making the user manual clearer, please do re-open and off your proposed text. The user manual can always do with improvement!

comment:6 Changed 19 months ago by nicuveo

I am afraid I still see this behaviour in 8.4.2...

I replaced INCOHERENT with OVERLAPPABLE, built my small example with --resolver nightly-2018-05-09, and in both cases it shortcuts and picks the general instance and prints 0.

[EDIT]
I also have a (perhaps naive) question: I see your patch prevents the short-cut solver from picking an instance if it's marked as OVERLAPPABLE or OVERLAPS. But it would also be valid, according to the documentation, for the generic instance to be unmarked, and the specific instance to be marked as OVERLAPPING or OVERLAPS, wouldn't it?

Either IX is overlappable, or IY is overlapping. (This “either/or” design, rather than a “both/and” design, allow a client to deliberately override an instance from a library, without requiring a change to the library.)

But if the shortcut solver always picks the unmarked instance, it will break this, won't it?

Last edited 19 months ago by nicuveo (previous) (diff)

comment:7 Changed 19 months ago by nicuveo

Since in 8.4.2 the behaviour does not depend on the optimization level, should I maybe close this bug and reopen ticket:14434? Or should I rewrite this one and create a better minimal test case?

comment:8 Changed 19 months ago by RyanGlScott

I'm quite confused, as I can't seem to trigger this bug in GHC 8.4.2 (or HEAD).

First, it's worth noting that the tarball you provided has a slightly different program than the one you gave in the original description. The modules in the tarball are:

{-# LANGUAGE FlexibleInstances #-}
{-# OPTIONS_GHC -fno-warn-simplifiable-class-constraints #-}

module A where

import           Data.Maybe

class A a where
  someValue :: a -> Maybe Int

instance {-# INCOHERENT #-} A a where
  someValue = const Nothing

getInt :: A a => a -> Int
getInt x = fromMaybe 0 $ someValue x
module B where

import           A

data B = B Int
data C = C Int

instance A B where
  someValue (B x) = Just x

getBInt :: Int
getBInt = getInt $ B 42

getCInt :: Int
getCInt = getInt $ C 42
-- Main.hs
import           B

main :: IO ()
main = do
  putStrLn "=========================================="
  putStrLn $ "B: " ++ show getBInt
  putStrLn $ "C: " ++ show getCInt

I'll use these, since the programs in the original description do not compile.

Second, when I compile and run these with GHC 8.4.2, I get the same answer, regardless of optimization level:

$ /opt/ghc/8.4.2/bin/ghc -O0 -fforce-recomp Main.hs
[1 of 3] Compiling A                ( A.hs, A.o )
[2 of 3] Compiling B                ( B.hs, B.o )
[3 of 3] Compiling Main             ( Main.hs, Main.o )
Linking Main ...

$ ./Main 
==========================================
B: 42
C: 0

$ /opt/ghc/8.4.2/bin/ghc -O2 -fforce-recomp Main.hs
[1 of 3] Compiling A                ( A.hs, A.o )
[2 of 3] Compiling B                ( B.hs, B.o )
[3 of 3] Compiling Main             ( Main.hs, Main.o )
Linking Main ...

$ ./Main 
==========================================
B: 42
C: 0

So unless you're using a yet further different version of this program, I'm inclined to agree that there is no bug here.

Last edited 19 months ago by RyanGlScott (previous) (diff)

comment:9 Changed 19 months ago by nicuveo

Yes, I'm using the version in the tarball; I simplified it for the purpose of the bug description. And my apologies: I double-checked, and indeed, the code in the tarball works for 8.4.2; my stack setup was wrong. Sorry about that!

...however, sadly, there is the other bug I highlight in comment:6.

If you replace A.hs to read:

instance A a where
  someValue = const Nothing

and B.hs to read:

instance {-# OVERLAPPING #-} A B where
  someValue (B x) = Just x

Then the same behaviour is back with 8.4.2: with -O0, it prints 42, with -O2, it prints 0.

EDIT: but that's a different bug, and I can (should?) create a new bug / reword that one.

Last edited 19 months ago by nicuveo (previous) (diff)

comment:10 Changed 19 months ago by RyanGlScott

Resolution: duplicate
Status: newclosed

Indeed, that is a separate bug, so I'd recommend opening a new ticket so that it does not get lost in here.

Some advice for the future:

  • Make sure to post the complete code. The files in the original description leave off several details like the language extensions and module names. Without these, we have to reverse-engineer them, which is quite annoying.
  • Don't bother posting stack.yaml files or other extraneous things like that. An ideal reproducing test case is one where you can simply invoke GHC on some files to produce the issue. And you already have such a test case!

comment:11 Changed 19 months ago by nicuveo

Duly noted. Thanks!

Note: See TracTickets for help on using tickets.