Allow instances for synonyms

[garyb]

Resolves #2529 at least

Can expand it to just allow instances wherever if we prefer, in which case it'll deal with #3391 too.

[garyb]

So yeah, allowing it in this situation should be uncontroversial - but if we did expand it, what are the pros/cons? I can only think of one primarily for each case:

• Pro: there are probably some situations that the determined var isn't enough,
• Con: it might confuse people sometimes if they don't realise they're trying to define an instance for a synonym

The con will mostly that will manifest as an overlapping instance error. I guess ideally we'd include a suggestion that spots when an overlap involves a synonym and mentions that might be why, but I'm not sure we have enough info to do that when the overlap is detected.

[garyb]

Hah, if I looked at the code I just modified I'd see that we could indeed easily determine it here 😄

I'll see about improving the error in this case first, as it's still potentially helpful even if we keep the fundep restriction, as overlaps can still arise accidentally.

[garyb]

Hmm, it'll need a little more thought actually - we'd have to store the un-expanded types for instances in the environment too, to be able to report that suggestion for sure - it'd be order dependent.

[hdgarrood]

Would it be sufficient to pass zip tys tys' to checkOverlappingInstance instead of just tys'?

[hdgarrood]

Oh, I suppose not, because checkOverlappingInstance doesn't actually perform the entire overlapping instance check.

Regardless of which route we take though (i.e. whether we only allow determined arguments or whether we just allow everything) I think it would be best to not to do this until we can add a hint for the case where an overlap arises and type synonyms are involved.

[natefaubion]

Regardless of which route we take though (i.e. whether we only allow determined arguments or whether we just allow everything) I think it would be best to not to do this until we can add a hint for the case where an overlap arises and type synonyms are involved.

I think a case could be made that this is a problem in general, and we don't really have a way to reliably report any issues that might have arised due to synonyms. I think I would like to just go ahead and get this in since it's a very useful feature. What do you think?

[hdgarrood]

Yeah on reflection I agree, I'd like to get this in too. My preference would be to drop the instance synonym restriction entirely, and allow synonyms anywhere in instances as long as they are fully applied.

[garyb]

👍 I'll remove the determined-by-fundep check and it should be good to go then.

[natefaubion]

@garyb Is this something you intend to fix up?

[garyb]

Wow there, what's the hurry? It's only just been WIP for a year. 😆

Yeah, sure I'll fix it up. I hadn't forgotten about it actually, just never quite get around to it.

[garyb]

@thomashoneyman I saw that 😉

Yes, but it also needs a merge, I'm going to do it now.

[thomashoneyman]

😀I was still thinking when I reflexively hit the comment button, but I didn’t want to bug you.

[garyb]

I was getting a different error when I ran the tests locally, so wanted to throw this at CI and see how it behaved there. Figured it out now though, I needed to update my test support dependencies locally.

[thomashoneyman]

Can this be added to the v0.14.0 milestone to track it there? Or is that milestone not being curated?

[hdgarrood]

At this point I don’t want to add anything to 0.14.0 unless it’s a showstopping bug. In general I would prefer to only add issues to milestones, as if you have PRs in milestones too then you’re effectively counting things twice. Not adding this to 0.14.0 doesn’t mean it definitely won’t get merged in time for 0.14.0, it just means that it might not get merged if everything else in 0.14.0 is ready to go and it’s time to make a release.

[hdgarrood]

This looks good to me, although I have a question: what happens if a type synonym expands to something we'd otherwise disallow (because of rows)? It looks like checkTypeClassInstance is called before expanding synonyms, so I'm wondering if that might allow you to slip past this check. For example, what does this do?

module Main where

type X r = {x :: Int | r}

instance Show (X r) where
  show _ = ""

Also I guess the PR title is now inaccurate, as we're now allowing synonyms for instances everywhere?

[garyb]

Good spot! It does allow bad things like that to be defined currently.

[rhendric]

Is @hdgarrood's objection the only blocker here? Isn't it very easily resolved by just swapping the checkTypeClassInstance line with its successor (and adding a prime to the tys' in it)? I tried that locally and it seems to work fine.

[hdgarrood]

I think that ought to do it, yes. I think this should be good to merge once that change has been made and a test has been added for the example I gave.

[rhendric]

I've pushed those changes to @garyb's branch; I hope that wasn't rude! It seemed not worth making a new PR for such a small addition and you did say in #3940 that you didn't mind someone else carrying this over the finish line.

[kl0tl]

Should we try to fix #3104 with this pull request? It seems that replacing type synonyms at the top of the solver loop is enough. Should we also replace synonyms in type classes contexts?

[hdgarrood]

I think it might be nice to address #3104 in a separate PR, since I’d consider it a separate issue (that bug exists whether or not we allow synonyms in instance heads) and since it requires changes in a different part of the compiler.

[kl0tl]

My reasoning was that allowing type instances for synonyms makes this bug easier to stumble on:

data D
type S = D

class C0 a
class C0 a <= C1 a

instance c0 :: C0 D
instance c1 :: C1 S

Error found:
in module Example
at Example.purs:16:1 - 16:20 (line 16, column 1 - line 16, column 20)

  No type class instance was found for
                
    Example.C0 S
                

while checking that expression #dict C0
  has type Record (() @Type)
in value declaration c1

See https://github.com/purescript/documentation/blob/master/errors/NoInstanceFound.md for more information,
or to contribute content related to this error.

but sure, I can open another pull request.

[hdgarrood]

Right, yes, it’s definitely easier to run into that bug after this change, so I agree that it would be good to address it soon after this. Thanks for spotting this!

[garyb]

@rhendric thanks for picking it up, and no it wasn't rude at all 😄

[garyb]

(I can't give this the double-approval since I opened the PR)

[hdgarrood]

This PR and #3936 both added a test called TypeSynonyms6, so I've resolved the conflict by giving each of them a slightly more descriptive name.

[kl0tl]

I opened #3965 and #3966 to resolve some issues with the interaction of type synonyms, superclass constraints and derived newtype instances.