Repository navigation
Derive instances when data types use type synonyms - #2516
Conversation
garyb
left a comment
garyb
left a comment
There was a problem hiding this comment.
Hell yeah! 🎉 🎆 🎊
This approach seems fine to me too, what would have been the advantage of moving it to the typechecker?
|
|
||
| -- | Replace fully applied type synonyms | ||
| replaceAllTypeSynonyms' | ||
| :: M.Map (Qualified (ProperName 'TypeName)) ([(Text, Maybe Kind)], Type) |
There was a problem hiding this comment.
Could move the SynonymMap type into this module, and then use it here?
| => SynonymMap | ||
| -> Type | ||
| -> m Type | ||
| replaceAllTypeSynonymsM syns = either throwError pure . replaceAllTypeSynonyms' syns |
There was a problem hiding this comment.
Maybe this should go in Language.PureScript.TypeChecker.Synonyms too?
| -> Declaration | ||
| -> m Declaration | ||
| deriveInstance mn ds (TypeInstanceDeclaration nm deps className tys@[ty] DerivedInstance) | ||
| deriveInstance mn _ ds (TypeInstanceDeclaration nm deps className tys@[ty] DerivedInstance) |
There was a problem hiding this comment.
I think these need to do synonym substitution too. They inspect data constructor arguments; for example, deriveOrd calls objectType to check for a record.
There was a problem hiding this comment.
The following fail on 0.10.3:
type L = {}
data X = X L
derive instance eqX :: Eq X
type M = {}
data Y = Y {foo :: M}
derive instance eqY :: Eq Y
type N = {}
data Z = Z N
derive instance eqZ :: Eq ZThere was a problem hiding this comment.
Yes, I realized this last night after I finished working on this 😄
I'll try to finish this up later.
There was a problem hiding this comment.
Liam's right. Nested synonyms also fail, which is pretty much a necessity to fix it for the record case where it commonly arises:
type Foo = String
type Bar = { foo :: Foo }
type Baz = { baz :: Bar }
newtype T = T Baz
derive instance eqT :: Eq T
derive instance ordT :: Ord T
derive instance genericT :: Generic TAll of the above instances fail, although Newtype works.
edit: example is redundant, since they're covered in Liam's cases... but you get the idea 😉
|
This is ready for review again, those issues should now be fixed. |
|
Looks good to me 👍 |
|
Ok, I'll merge this since @LiamGoodacre has signed off and the tests are passing. |
|
This still doesn't work for record aliases. I can't get the @garyb example to compile with v0.10.5, it fails with |
|
That's a different issue though. It means that we don't derive |
|
@paf31 This minimal, non-nested example fails too:
|
|
Then that is a bug, thanks for the report 😄 |
4 participants
Fixes #2481
Fixes #2416
Fixes #1443
I would have liked to fix this by moving deriving into the type checker, but then we'd have to rearrange a whole bunch of stuff, so instead this just uses the externs to build a map of type synonym data, and then reuses the existing
replaceAllTypeSynonyms'function to eliminate any synonyms in the instance head.