Superclasses must also be newtype derived

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.

(also concerned that newtype instances may not be kind checked, hm ...)

Hmm, I think there may be a bug here. We do have a check for this, the corresponding error is MissingNewtypeSuperclassInstance.

Ah maybe that only triggers for fully missing instances.

I seem to remember this ought to work and report an error, even in this case. So I think this is a bug.

Agreed 👍 my second comment was meant as a clarification on what the bug might be.

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…

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 <= Ord and be able to use Ord but not Eq ... i.e. it's not a matter of behavioral consistency, but what the axioms of the type system imply :)

Feedback welcome!

Navigation Menu

Repository navigation

Superclasses must also be newtype derived #3168

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Superclasses must also be newtype derived #3168

Description

Activity

MonoidMusician commented on Dec 11, 2017

garyb commented on Dec 11, 2017

garyb commented on Dec 11, 2017

paf31 commented on Dec 11, 2017

garyb commented on Dec 11, 2017

MonoidMusician commented on Dec 11, 2017

MonoidMusician commented on Jan 20, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions