Default 'RuntimeRep' arguments unless otherwise specified #941

harpocrates · 2018-09-17T15:42:33Z

This adds a module level Haddock option called print-runtime-reps which behaves like the GHC -fprint-explicit-runtime-reps flag. Hopefully this will let us restore beginner-friendliness to the Prelude types, without having to compromise for simplified types in GHC.Prim.

See the discussion in #838 for more.

This means all 'RuntimeRep' tyvars will be muted, unless the user explicitly asks that they be shown by speciying {-# OPTIONS_HADDOCK print-runtime-reps #-} This design is going make it possible to have the same type be displayed differently depending on which module it is exported from (eg. the Prelude or GHC.Exts).

harpocrates · 2018-09-17T15:45:04Z

We'll have to remember to add {-# OPTIONS_HADDOCK print-runtime-reps #-} to the top of some of the internal files of base once Hi Haddock is merged.

Here is what the test cases look like.

RyanGlScott

This looks quite reasonable to me.

I forget: is there a users' guide description of all the flags that Haddock accepts? If so, it would be worth mentioning this flag in there as well.

Some minor comments inline.

RyanGlScott · 2018-09-17T16:35:16Z

haddock-api/src/Haddock/GhcUtils.hs

+      | tv `elemVarEnv` subs
+      = TyConApp liftedRepDataConTyCon []
+      | otherwise
+      = TyVarTy (updateTyVarKind (go subs) tv)


I think it's unnecessary to recurse into tv's kind here. (At least, IfaceType.defaultRuntimeRepVars doesn't do this.)

It is necessary, since we do recurse into type variable's kinds when computing the inferred order of type variables (in tyCoFVsBndr'). Perhaps we should not recurse into the type variables kind there?

Upon further thought, maybe it's better if we do recurse here.

For pretty-printing purposes, it's probably overkill, since if you have a type like:

forall (r :: RuntimeRep) (a :: TYPE r). Int -> a

Then after defaulting, it'll still display as:

forall a. Int -> a

Regardless of whether you recurse into the kind of a (in Int -> a) or not. On the other hand, if you don't recurse into the kind of a, then you have a strange situation where the kind of a in forall a is Type, but the kind of a in Int -> a is TYPE r. I suppose the only way that could ever really bite you is if you were to run Core Lint on these types (which I certainly don't imagine happens today), but for the sake of consistency, it might be best just to update the kind of a in Int -> a anyways.

RyanGlScott · 2018-09-17T16:36:00Z

haddock-api/src/Haddock/Interface/Create.hs

 parseOption "not-home"        = return (Just OptNotHome)
 parseOption "show-extensions" = return (Just OptShowExtensions)
+parseOption "print-runtime-reps" = return (Just OptPrintRuntimeRep)
+parseOption "print-explicit-runtime-reps" = return (Just OptPrintRuntimeRep)


Is there a reason for having two flags with the same effect? Why not just standardize on a single flag?

No particular reason. I want this flag to be called print-runtime-reps, but I anticipate some confusion with GHC's -fprint-explicit-runtime-reps (cf. Cabal's hyperlink-source option vs Haddock's hyperlinked-source option). Do you have an opinion one way or another?

My inclination would be to have -fprint-explicit-runtime-reps be the shared name of the flag across GHC and Haddock. I suppose I would be fine with picking -fprint-runtime-reps to be the name of the Haddock flag as well, but I definitely think we shouldn't come up with two different ways of referring to the same flag—that just seems like a recipe for confusion.

RyanGlScott · 2018-09-17T16:36:52Z

haddock-api/src/Haddock/Convert.hs

    annot_ts = zipWith3 annotHsType is_poly_tvs ts ts'
    is_poly_tvs = mkIsPolyTvs (tyConVisibleTyVars cls_tycon)
-    synifyClsIdSig = synifyIdSig DeleteTopLevelQuantification vs
+    synifyClsIdSig = synifyIdSig True DeleteTopLevelQuantification vs


Why do you hardcode PrintRuntimeReps to True here instead of consulting the flag?

Yeah. So this brings up a bigger question: should we thread through the RuntimeRep defaulting logic to everything involving Type in this module? Perhaps so, but that is going to be a fair bit of plumbing.

I think it is simpler to start with defaulting only top-level signatures, but I'm willing to be told otherwise.

If your goal is to avoid having any RuntimeRep quantification unless explicitly enabled, I think you'll eventually need to tackle class instances as well. After all, you can define levity polymorphic instances like so:

λ> class C a λ> instance forall (r :: RuntimeRep) a (b :: TYPE r). C (a -> b) λ> :i C class C (a :: k) -- Defined at <interactive>:2:1 instance [safe] forall a b. C (a -> b) -- Defined at <interactive>:3:10 λ> :set -fprint-explicit-runtime-reps λ> :i C class C (a :: k) -- Defined at <interactive>:2:1 instance [safe] forall (r :: RuntimeRep) a (b :: TYPE r). C (a -> b) -- Defined at <interactive>:3:10

Gah. Alright. Thanks for the motivating example. 😄

Upon further inspection: I can't convince Haddock to ever print out runtime reps for instances. I'm inclined to leave this be for the moment. Plus this part of the wip/hi-haddock branch, so I'm trying to stay away from far-reaching changes.

Also patched up 'synifyTyCon' to do a better job on '(->)'.

sjakobi

A few wibbles. :)

sjakobi · 2018-09-27T16:10:50Z

doc/cheatsheet/haddocks.md

 {-# OPTIONS_HADDOCK show-extensions #-}
  Show all enabled LANGUAGE extensions
+{-# OPTIONS_HADDOCK print-explicit-runtime-reps #-}
+  Don't default 'RuntimeRep' type variables


This sentence seems to be missing a print.

Oh, I missed that default is the verb here. I guess this could confuse other people too.

I like the description you used for the option parser:

Render runtime reps for this module (see the GHC @-fprint-explicit-runtime-reps@ flag)

sjakobi · 2018-09-27T16:15:07Z

haddock-api/src/Haddock/Convert.hs

+-- | Whether or not to default 'RuntimeRep' variables to 'LiftedRep'. Check
+-- out Note [Defaulting RuntimeRep variables] in IfaceType.hs for the
+-- motivation.
+type PrintRuntimeReps = Bool


Can we have a newtype or a custom type instead of the alias here?

Aliases tend to be refactored to the underlying types in my experience. Also, an explicit True or False doesn't say much about its purpose.

sjakobi · 2018-09-27T16:22:25Z

haddock-api/src/Haddock/Interface/Create.hs

  let renamer = docIdEnvRenamer (docs_id_env mod_iface_docs)

  opts <- liftErrMsg $ mkDocOpts (docs_haddock_opts mod_iface_docs) flags mdl
+  let prr = OptPrintRuntimeRep `elem` opts


I'd rather have a more meaningful variable name in this module.

sjakobi · 2018-09-29T17:17:07Z

Thanks! :)

harpocrates added 2 commits September 17, 2018 08:37

Add test cases for 'print-runtime-reps'

1ac8d35

harpocrates requested review from RyanGlScott and sjakobi September 17, 2018 15:42

RyanGlScott reviewed Sep 17, 2018

View reviewed changes

harpocrates added 2 commits September 17, 2018 10:04

Documentation + keep only one option name

0281e90

Elide 'RuntimeRep' in more places

89890cf

Also patched up 'synifyTyCon' to do a better job on '(->)'.

harpocrates added the hi-haddock label Sep 18, 2018

sjakobi suggested changes Sep 27, 2018

View reviewed changes

Address nits

7dc26bf

sjakobi merged commit d7259c5 into haskell:wip/hi-haddock Sep 29, 2018

harpocrates deleted the default-runtime-rep branch October 16, 2018 19:59

Default 'RuntimeRep' arguments unless otherwise specified #941

Default 'RuntimeRep' arguments unless otherwise specified #941

Uh oh!

Conversation

harpocrates commented Sep 17, 2018

Uh oh!

harpocrates commented Sep 17, 2018

Uh oh!

RyanGlScott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanGlScott Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjakobi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjakobi commented Sep 29, 2018

Uh oh!

Uh oh!

RyanGlScott Sep 17, 2018 •

edited

Loading