Repository navigation
Superclasses must also be newtype derived #3168
Description
As I understand it now, we can newtype a type and derive instances of classes for it, without necessarily having all of the instances derived going up the chain. Which is fine, it won't crash any code.
But the compiler still allows me to write my own instance for farther up the chain, which will introduce consistency issues. For example, the following will access different functor instances:
module Main where
import Prelude
import Data.Const
import Control.Monad.Eff.Console (logShow)
newtype S a = S (Const String a)
derive newtype instance eqS :: Eq (S a)
derive newtype instance applyS :: Apply S
instance functorS :: Functor S where
map _ _ = S (Const "unlawful")
oneMap :: forall f a b. Apply f => (a -> b) -> f a -> f b
oneMap = map
otherMap :: forall f a b. Functor f => (a -> b) -> f a -> f b
otherMap = map
main = logShow $ let v = S (Const "lawful") in oneMap (_+0) v == otherMap (_+0) v
So I would propose we need to require superclass instances to also be newtype-derived.
Activity
MonoidMusician commentedon Dec 11, 2017
(also concerned that newtype instances may not be kind checked, hm ...)
garyb commentedon Dec 11, 2017
Hmm, I think there may be a bug here. We do have a check for this, the corresponding error is
MissingNewtypeSuperclassInstance.garyb commentedon Dec 11, 2017
Ah maybe that only triggers for fully missing instances.
paf31 commentedon Dec 11, 2017
I seem to remember this ought to work and report an error, even in this case. So I think this is a bug.
garyb commentedon Dec 11, 2017
Agreed 👍 my second comment was meant as a clarification on what the bug might be.
MonoidMusician commentedon Dec 11, 2017
Right, I think this warning might be triggered: https://github.com/purescript/purescript/blob/master/src/Language/PureScript/Sugar/TypeClasses/Deriving.hs#L240
But I think it should be raised to an error in this case, where the superclass instance is given but not newtype derived, due to this potential for inconsistency.
Maybe I can see if this can be improved. It might not even be recursive right now.
Exclude non-newtype derived instances from superclass check (purescri…
MonoidMusician commentedon Jan 20, 2018
Okay so it looks like we were pulling in all the instances, so I changed it to match on
NewtypeInstance... but that only affects local instances, it doesn't like that information is stored in externs. Do we want to extend the check to externs? or call this good enough?Also I upgraded most of the existing cases from warnings to errors. Technically they won't introduce a major inconsistency like the bug I found, but it is a little weird to have
Eq <= Ordand be able to useOrdbut notEq... i.e. it's not a matter of behavioral consistency, but what the axioms of the type system imply :)Feedback welcome!