Show lifted list and tuple typereps properly
Needs RevisionPublic

Authored by dfeuer on Oct 10 2017, 7:20 PM.

Details

Summary
  • Previously, we used special sugar to show typereps for tuple and list types (i.e., [k] rather than [] k and (a,b) rather than (,) a b), but we did not use it to show lifted lists and tuples, such as '[Int,Char] or '( 'True, 3). Fix that.
  • Don't show redundant parentheses in tuple fields.
dfeuer created this revision.Oct 10 2017, 7:20 PM
RyanGlScott requested changes to this revision.Oct 10 2017, 8:52 PM
RyanGlScott added a subscriber: RyanGlScott.

Seems reasonable, but can you add some tests to verify that this does what it claims?

This revision now requires changes to proceed.Oct 10 2017, 8:52 PM
dfeuer updated this revision to Diff 14328.Oct 11 2017, 2:37 PM

Add tests

dfeuer updated the Trac tickets for this revision.Oct 11 2017, 2:50 PM

Seems reasonable, but can you add some tests to verify that this does what it claims?

Done. I realized that there's still one way this instance is broken (see part 3 of Trac #14341), but that shouldn't get in the way of fixing these bits, I don't think.

RyanGlScott accepted this revision.Oct 11 2017, 3:04 PM

LGTM, thanks!

I've left one suggestion inline.

libraries/base/Data/Typeable/Internal.hs
531–533

We're accumulating quite a large number of special cases here, and it took me a bit to figure out which cases correspond to which types. Having comments before each guard like:

-- Promoted tuples '(42, "hello")
| isTickedTupleTyCon tc =
-
-- Promoted lists '[42, 27]
| App lst ks <- typeRepKind rep

-- etc.

Would go a long way in making this more readable, I think.

This revision is now accepted and ready to land.Oct 11 2017, 3:04 PM
dfeuer updated this revision to Diff 14329.Oct 11 2017, 6:03 PM
  • Improve the handling of type operators
  • Improve the handling of kind arguments
dfeuer requested review of this revision.Oct 11 2017, 6:04 PM

@RyanGlScott I added a bunch of comments, but I also added some more special cases for your enjoyment.
Would you mind taking another look?

dfeuer marked an inline comment as done.Oct 11 2017, 6:04 PM
dfeuer updated this revision to Diff 14331.Oct 11 2017, 9:54 PM

Identify more operators

dfeuer updated this revision to Diff 14332.Oct 11 2017, 10:02 PM

Update test results

@RyanGlScott Should I move the tests into tests/typecheck/should_run/TypeRep? Those seem to be more focused on the actual Typeable deriving and such than on how things look, but maybe it makes sense to put them all in one bucket....

Question: should I omit kind arguments for fully applied constructors (i.e., one whose kind is not a TrFun)? That would mean Proxy :: Nat -> Type would give us "Proxy @Nat", but Proxy 3 would give us "Proxy 3".

@RyanGlScott Should I move the tests into tests/typecheck/should_run/TypeRep? Those seem to be more focused on the actual Typeable deriving and such than on how things look, but maybe it makes sense to put them all in one bucket....

It's up to you. No one's ever accused GHC's test suite of being particularly organized in the first place, but feel free to put these tests somewhere else if it makes you sleep easier at night.

Question: should I omit kind arguments for fully applied constructors (i.e., one whose kind is not a TrFun)? That would mean Proxy :: Nat -> Type would give us "Proxy @Nat", but Proxy 3 would give us "Proxy 3".

I think ultimately this is a question of correctness vs. convenience. For one thing, visible kind application is technically not legal syntax in the first place, but at the same time, I think I'd prefer looking at Proxy @Nat to Proxy :: Nat -> Type. After all, this output is purely for the programmer's benefit, and we make no claim that the Show output for any particular TypeRep is guaranteed to be legal syntax anyways, so we are free to take some creative liberties here.

As for whether we should display @Nat altogether? My gut reaction is to omit visible kind applications for applied type constructors, since one should (in principle) be able to infer these from the arguments' kinds.

This is reminiscent of the approach we take in Template Haskell (from the commentary here) when deciding whether to annotate a reified type with its kind. The situation here is much simpler than in Template Haskell though, since (a) we're only dealing with type constructors (and not things like type synonyms or type families), so every argument position is injective, and (b) we're always dealing with monomorphic kinds due to the nature of Typeable. Thus, I think the only time we'd need extra kind information is during partial tycon applications, as you've suggested.

libraries/base/Data/Typeable/Internal.hs
582

Perhaps reference the Trac ticket that requests adding precedence information to TyCons.

648

If you really want to catch Unicode stuff, we could consider moving GHC.Lexeme from ghc-boot-th to base. Then we could just say:

startsVarSym symb || startsConSym symb

Since startsVarSym is Unicode-aware.

As for whether we should display @Nat altogether? My gut reaction is to omit visible kind applications for applied type constructors, since one should (in principle) be able to infer these from the arguments' kinds.

Yes, this sounds reasonable to me.

bgamari requested changes to this revision.Nov 2 2017, 11:09 AM

@dfeuer, did you want to make the type app change?

This revision now requires changes to proceed.Nov 2 2017, 11:09 AM
austin resigned from this revision.Nov 9 2017, 5:41 PM