Allow (unparenthesized) kind signatures
ClosedPublic

Authored by harpocrates on Sep 23 2018, 4:39 AM.

Details

Summary

This allows for things like [t :: MyKind], (a :: k, b), and so on.

Test Plan

make TEST=T11622 && make TEST=T8708

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.
harpocrates created this revision.Sep 23 2018, 4:39 AM

Great work! One comment inline.

compiler/hsSyn/HsTypes.hs
1476

Good catch. Now that you've pointed this out, it occurs to me that the treatment for explicit signatures at the type level is inconsistent with that of the expression and pattern level. That is to say, in hsExprNeedsParens, we have:

go (ExprWithTySig{})              = p > topPrec

And in patNeedsParens, we have :

go (SigPat {})            = p > topPrec

I wonder if both of these should actually be p >= sigPrec.

simonpj accepted this revision.Sep 23 2018, 7:42 AM
simonpj added a subscriber: simonpj.

I have not dug deep but this looks plausible to me. Thank you!

This revision is now accepted and ready to land.Sep 23 2018, 7:42 AM
  • Address nits
  • accept test case
harpocrates planned changes to this revision.Sep 23 2018, 12:58 PM

This still needs some work. Looks like some a handful of the failing tests are legitimate. Two types of problems stand out:

  • Code generated by derived instances should have extra parens when needed, so that error messages involving those types have parens.
  • There are some things that no longer parse properly. Simple example: type Representational1 m = (forall a b. Coercible a b => Coercible (m a) (m b) :: Constraint)
  • More conservative grammar
This revision is now accepted and ready to land.Sep 24 2018, 3:00 AM

I think the grammar/deriving issues are solved, but I've uncovered an annotation issue that I really don't know how to fix - perhaps someone could advise?

This is the cause of the remaining test failure in T11018.

Consider the following (valid) data declaration: data A ((x :: Type)). This will fail the check-api-annotations check, with or without this patch. It fails because the outer parens don't map to any AST node in the parsed source. The closest they have to a corresponding AST node is the tcdTyVars :: LHsQTyVars GhcPs of the data declaration. Unfortunately, each type variable in tcdTyVars can contain only one location, picking either the span of ((x :: Type)) or the span of (x :: Type) but not both. Right now, the latter is what gets picked (see how in checkTyVars any outer HsParTy's are straight up discarded).

The reason we see new test failure is because, prior to this patch, HsKindTyincluded one set of parens around it. That just happened to pass annotation tests for the usual case: data A (x :: Type).

It would be interesting to hear @alanz's opinion on https://phabricator.haskell.org/D5173#142447.

alanz added a comment.Sep 24 2018, 7:46 AM

Nested parens are generally a mess, particularly in type definitions. I will take a look.

alanz added inline comments.Sep 24 2018, 9:16 AM
compiler/parser/Parser.y
1955

The original ctype variant of this included the '(' ctype '::' kind ')' .... rule at this point which preserved the parens.

This is where the current ktype is losing them, I think.

The multiple parens case will need some more tweaking, using mkParensApiAnn as in e.g. checkTyClHdr.

  • Fix annotation bug around parens
  • Add another test case
harpocrates added inline comments.Sep 24 2018, 11:39 AM
compiler/parser/Parser.y
1955

Aha! mkParensApiAnn is exactly what I was looking for. I think the latest changes I just made will fix this this problem. Thank you for the pointer!

I ended up making checkTyVars responsible for producing the extra paren annotations.

  • 'type' -> 'kind'
RyanGlScott accepted this revision.Oct 4 2018, 9:14 AM

LGTM. I'm a bit confused that the TH_spliceGuard appears to be failing on Harbormaster, but perhaps that's just an unlucky fluke. Perhaps rebasing on top of master will make that go away?

Also, it might be worth mentioning this feature in the 8.8.1 release notes, since this allows more programs to parse than before!

harpocrates updated this revision to Diff 18216.Oct 4 2018, 2:13 PM
  • Address nits
  • accept test case
  • More conservative grammar
  • Fix annotation bug around parens
  • Add another test case
  • 'type' -> 'kind'
  • Update user guide notes
This revision was automatically updated to reflect the committed changes.