More synify-ing improvements #931
Conversation
* adds space after/before the '#' marks * properly reify 'HsSumTy' in 'synifyType'
When we have a fully applied promoted tuple, we can expand it out properly.
We reconstruct promoted list literals whenever possible. That means that 'synifyType' produces '[Int, Bool, ()] instead of (Int ': (() ': (Bool ': ([] :: [Type]))))
The other types are still looked at when considering whether to make a kind signature or not.
* filter out more 'forall's in class decls/instance decls * extract default class methods too * filter out more 'forall's in pattern synonym decls
When deciding if we want to have an explicit 'forall', one of the things to check is if the deduced type variable order would match the explicit one. Turns out that we need to roll our own function for that. One example of where this got fixed in the Prelude: fst :: forall a b. (a, b) -> a turned back into fst :: (a, b) -> a
This is especially nice for bundled pattern synonyms, where the
synonyms go back to looking (almost always) like plain constructors.
In 'BundledPatterns', this renders
pattern (:>) :: a -> Vec n a -> Vec (n + 1) a
instead of
pattern (:>) :: forall a (n :: Nat). a -> Vec n a -> Vec (n + 1) a
since the kind of `n` can be inferred from its use in `Vec n a`.
Class decls produced by 'tyThingToLHsDecl' now contain associated type defaults when appropriate.
| ------------------------------------------------------------------------------- | ||
|
|
||
| -- | Get free type variables in a 'Type' in their order of appearance. | ||
| -- See [Ordering of implicit variables]. |
There was a problem hiding this comment.
I must be missing some context here. Why would we ever want to retrieve free type variables in reverse order?
There was a problem hiding this comment.
We want them in order. Unfortunately, that's not what GHC does.
unionFV starts by running the second FV on the accumulator, then passing that to the first FV. That means it adds variables from the second argument to the in_scope set before adding variables from the first argument. All this is bad news if the first and second arguments share some variables.
Here is a snippet demonstrating the somewhat surprising behaviour:
ghci -package ghc
ghci> import FV
ghci> import TysWiredIn
ghci> import Unique
ghci> import Name
ghci> import OccName
ghci> import Id
ghci> x0 = mkLocalId (mkSystemName (mkUniqueGrimily 0) (mkVarOcc "x0")) unitTy
ghci> x1 = mkLocalId (mkSystemName (mkUniqueGrimily 1) (mkVarOcc "x1")) unitTy
ghci> tvs = fvVarList (unitFV x0 `unionFV` unitFV x1 `unionFV` unitFV x0)
ghci> map (getOccString . idName) tvs
["x1","x0"]My hack here is to traverse everything in reverse order, then reverse the list of type variables at the end.
There was a problem hiding this comment.
Oh dear, that's absolutely terrible. I really thought that tyCoVarsOfTypesWellScoped would be enough to accomplish this task, but your counterexample shows that I was mistaken.
Still, I'm incredibly bothered by the fact that we have to duplicate so much machinery just to solve this problem. I've sent an email to the ghc-devs list asking about this, so I'm hoping I can get some clarification on this point.
There was a problem hiding this comment.
Based on Simon's response here, it looks like the FV-returning functions really don't guarantee any particular order (only that it's deterministic), which is a huge bummer.
Ideally, we'd rewrite tyCoFVsOfType and friends in such a way that they always return things in left-to-right order. Your hacked version accomplishes this, but at the cost of reversing the order at the very end, which feels wasteful. Is there a way to accomplish the same thing with only one pass?
There was a problem hiding this comment.
Is there a way to accomplish the same thing with only one pass?
In GHC: yes. We'd need to change unitFV to append to the var list. In order to keep that operation efficient, that means swapping occurrences of [Var] in FV for difference lists of Var. It looks like the FV code has already been carefully performance tuned, so we'd have to be very careful there.
In Haddock: not really, short of porting over most of FV (and then making the changes we'd otherwise make in GHC).
What frustrates me the most is not the reverse: it's having to rely on what is accidental behaviour in GHC.
There was a problem hiding this comment.
Ugh, this is all so nasty.
Ultimately, I think the orderedFVs hack is probably the way to go for now. I'd only request that you expand the documentation for it to include an example of why tyCoVarsOfTypeWellScoped doesn't give the behavior you desire (e.g., your example in #931 (comment)).
Ideally, we'd investigate making the FV functions in GHC return things in a reliable left-to-right order. But as you've noted, they're quite performance intensive, so I can understand your reservations about making things slower by changing the underlying data structures involved.
There was a problem hiding this comment.
I've added an example illustrating GHC's behaviour (I didn't know about alphaTyVar and betaTyVar - thanks!).
I agree we should eventually look into making FV return things in a reliable left-to-right order. I'm a bit swamped at the moment, but I'll try to circle back when all of this Hi Haddock stuff settles down.
Fixes the 'Semigroup' method: stimes :: forall b. Integral b => b -> a -> a Into stimes :: Integral b => b -> a -> a
This should make it clearer why we need to define our copies of some of these functions.
|
There's a lot of improvements mentioned in your description, but surprisingly few added tests. Do the existing tests cover this already? If not, it would be good to add some new ones. |
The existing tests cover the changes. In fact, pretty much all changes were motivated by the 20 currently failing HTML tests on the Hi Haddock branch. I'm trying to get the test outputs close enough that they can just be accepted. As far as I'm concerned, the following test cases are ready to be accepted:
There are still
|
|
Ah, I missed that this was a part of the larger Hi Haddock project (in which case your goal is to un-break existing tests). I'll have to let @sjakobi review the Hi Haddock-specific bits, but otherwise, things here LGTM. |
This fixes the empty forall on the 'pattern Blub'. Before: pattern Blub :: forall. () => forall x. Show x => x -> BlubType After: pattern Blub :: () => Show x => x -> BlubType
'synifyDataTyConReturnKind' was behaving badly with kind signatures involving foralls and kind tyvars. Thankfully, 'TyCon's already have 'tyConResKind' for figuring out the return kind of type constructors. Consider `data (a :: *) >< b = Empty` with `-XPolyKinds`. Here is the header we generate: Before: data a >< (b :: k) :: forall k. Type -> k -> Type After: data a >< (b :: k)
|
@sjakobi Thanks for the review - a week's delay is hardly late! |
This cherry-picks from
ghc-headsome of the recent changes inhaddock-api/src/Haddock/Convert.hs, then goes on to fix some more stuff on top of that:foralls on class methods and instance methods (visible improvements inBug548,Bug613, andSpuriousSuperclassConstraints)foralls on pattern synonyms (visible improvements inBundledPatternsandBundledPatterns2)Test, but also all over the docs for thePrelude)