Reject class instances with type families in kinds
ClosedPublic

Authored by RyanGlScott on Aug 14 2018, 12:47 PM.

Details

Summary

GHC doesn't know how to handle type families that appear in
class instances. Unfortunately, GHC didn't reject instances where
type families appear in kinds, leading to Trac #15515. This is easily
rectified by calling checkValidTypePat on all arguments to a class
in an instance (and not just the type arguments).

Test Plan

make test TEST=T15515

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
RyanGlScott created this revision.Aug 14 2018, 12:47 PM
simonpj added inline comments.
testsuite/tests/polykinds/T11520.stderr
3–5

The previous error message is bad (Compose isn't a type family). But the new one isn't much better. The user thinks

  • What is this Any? Where did it come from? I didn't write it!

A random user probably doesn't even know that Any is a type family.

Can we do better?

testsuite/tests/typecheck/should_fail/T13909.stderr
4

Same here!

RyanGlScott added inline comments.Aug 17 2018, 6:38 AM
testsuite/tests/polykinds/T11520.stderr
3–5

I dunno. Can we do better? I honestly have no idea how to improve this, so if you have any ideas, l'd love to hear them.

RyanGlScott added inline comments.Aug 24 2018, 9:14 AM
testsuite/tests/polykinds/T11520.stderr
3–5

Any thoughts, @simonpj?

simonpj added inline comments.Aug 28 2018, 6:22 AM
testsuite/tests/polykinds/T11520.stderr
3–5

Here are some ill-formed ideas.

  1. In checkValidTypePat we check for family-freeness, but do not say what the offending family is. Maybe isTyFamFree can return the offending type family to put in the error message.
  1. In this case the offending type family occurs in an invisible place -- at least in an argument that is not usually printed. Maybe isTyFamFree can return a boolean saying whether the occurrence is in a visible place or not; then if not, advise the user to say -fprint-explicit-kinds to show the invisible arguments.
  1. If the offending tycon is Any maybe remind the user that it's a nullary type family that can't be used in an instance head.
  1. Now that we are getting visible kind application, I think we should print invisible arguments with a "@" sign.
RyanGlScott added inline comments.Aug 28 2018, 8:55 AM
testsuite/tests/polykinds/T11520.stderr
3–5

Out of all these suggestions, (2) sounds the most plausible to me. Would you be happy if I implemented that?

simonpj added inline comments.Aug 28 2018, 11:18 AM
testsuite/tests/polykinds/T11520.stderr
3–5

Yes indeed!

(You may want to do so in a separate patch, maybe even a separate ticket. Or not -- up to you.)

RyanGlScott planned changes to this revision.Aug 30 2018, 11:18 AM
RyanGlScott added inline comments.
testsuite/tests/polykinds/T11520.stderr
3–5

(You may want to do so in a separate patch, maybe even a separate ticket. Or not -- up to you.)

Well, considering that the -fprint-explicit-kinds suggestion only makes any sense if we are actually rejecting uses of type families in kinds in instances, I think I pretty much have to do both of these simultaneously.

One thing that will make this a bit awkward is that when we throw this error, we already display the offending instance below it (e.g., In the instance declaration for ‘HasName Hm’). However, because this comes from source code, -fprint-explicit-kinds has no effect on it. Therefore, if we wanted to show something like HasName Any Hm to show where the Any is sneaking in, we'd probably need to show the instance type again. Something like:

• Illegal type synonym family application (Any) in
    instance HasName Any Hm
• In the instance declaration for ‘HasName Hm’

But now we're redundantly displaying the instance twice. Do you think we should pop the error context stack to avoid showing the In the instance declaration for ‘HasName Hm’, or should I not bother with this?

should I not bother with this?

I wouldn't worry; it's an improvement already.

  • Improve them error messages
RyanGlScott marked 8 inline comments as done.Aug 30 2018, 1:45 PM

I've pushed a change which improves the overall quality of these sorts of error messages. Let me know what you think.

simonpj accepted this revision.Aug 30 2018, 3:46 PM

LGTM thanks!

This revision is now accepted and ready to land.Aug 30 2018, 3:46 PM
This revision was automatically updated to reflect the committed changes.