Fix #16002 by moving a validity check to the renamer
ClosedPublic

Authored by RyanGlScott on Dec 6 2018, 9:54 AM.

Details

Summary

The validity check which rejected things like:

type family B x where
  A x = x

Used to live in the typechecker. But it turns out that this validity
check was only being run on closed type families without CUSKs!
This meant that GHC would silently accept something like this:

type family B (x :: *) :: * where
  A x = x

This patch fixes the issue by moving this validity check to the
renamer, where we can be sure that the check will always be run.

Test Plan

make test TEST=T16002

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.Dec 6 2018, 9:54 AM
simonpj added inline comments.Dec 6 2018, 4:12 PM
compiler/rename/RnSource.hs
1888

This is odd. Would it not be easier to do this check in rnTyFamInstEqn? Rather than making a second pass over the equations?

RyanGlScott added inline comments.Dec 6 2018, 4:28 PM
compiler/rename/RnSource.hs
1888

I didn't want to put this check in rnTyFamInstEqn for two reasons:

  1. This is a validity check that is only necessary for closed type family equations. For other type family equations, the name of the equation will always correspond to the name of the type family by construction, so this validity check would be entirely redundant in non-closed type family equations.
  2. The name of the type family is not readily available in rnTyFamInstEqn (only the name of the equation), so I would have to plumb an extra argument through to rnTyFamInstEqn to enable checking this property there (actually, two arguments if you include the Located RdrName of the type family, which I need to pass to TyFamilyCtx).
RyanGlScott added inline comments.Dec 11 2018, 5:18 AM
compiler/rename/RnSource.hs
1888

Thoughts on the above, @simonpj?

RyanGlScott added inline comments.Dec 18 2018, 6:07 AM
compiler/rename/RnSource.hs
1888

Thoughts on the above, @simonpj?

simonpj added inline comments.Dec 18 2018, 11:29 AM
compiler/rename/RnSource.hs
1888

I think it'd be just one argument, of type Maybe Name giving, for a closed type family, the name of the tycon we we check against. It's like the Maybe (Name,[Name]) argument we already have.

I don't feel too strongly -- happy for you to do as you think best.

Sorry to have been slow

1890

Put this under the checkTc for consistency with the withHsDocContext?

RyanGlScott marked 5 inline comments as done.
compiler/rename/RnSource.hs
1888

Ah, I see what you mean. Now that you point it out, I think the Maybe Name is approach is cleaner. I went ahead and conjured up a custom data type for this purpose (ClosedTyFamInfo) since (1) I also need to pass the Located RdrName of the type family name as well, and (2) there's already enough Maybe values floating around.

1890

Good catch. Thankfully, this is moot now that I've moved this check to rnTyFamInstEqn.

I think I've resolved all of @simonpj's concerns here—if no one says otherwise, I'll land this some time next week.

simonpj accepted this revision.Dec 20 2018, 11:45 AM

Lovely

compiler/rename/RnSource.hs
818–819

It'd be consistent to give the Maybe (Name, [Name]) argument its own data type too. But it's an orthogonal refactoring.

This revision is now accepted and ready to land.Dec 20 2018, 11:45 AM
RyanGlScott added inline comments.Dec 20 2018, 12:31 PM
compiler/rename/RnSource.hs
818–819

Mm, good observation. This is almost an AssocInstInfo, except that AssocInstInfo has information that you only learn during typechecking (whereas this is in the renamer). I wonder what we should call the data type that we would use here?

simonpj added inline comments.Dec 20 2018, 3:04 PM
compiler/rename/RnSource.hs
818–819

Similar but not identical. You'd end up having to parameterise over both Class and TyVar, and one field wouldn't make sense...

I'd just define a simple new local data type for the purpose here in RnSource. I guess you could use the same type name, or add Rn?

This revision was automatically updated to reflect the committed changes.
RyanGlScott added inline comments.Dec 20 2018, 10:29 PM
compiler/rename/RnSource.hs
818–819

I decided against this in the end, if only because the function that actually consumes this Maybe (Name, [Name]) information, rnFamInstEqn, makes heavy use of the fact that it's inside a Maybe, since:

  1. It uses the Functor instance of Maybe in several places.
  2. It passes the value of type Maybe (Name, [Name]) to lookupFamInstName and newTyVarNameRn. Moreover, these functions are used in other places besides rnFamInstEqn, so it's not straightforward to change their type signatures to accept, say, AssocInstInfoRn instead of Maybe a.

I could convert the AssocInstInfoRn value to a Maybe (Name, [Name]) in rnFamInstEqn, but then there wouldn't be much point to defining a custom data type for it.