Fix #1004 with a pinch of dropForAlls

[RyanGlScott]

#1004 occurred because splitFunTys wouldn't work on a data type return kind that was headed by a forall, so let's drop those foralls before attempting to split it.

[harpocrates]

Do you mind perhaps copy-pasting the data declaration for Product directly into the test case (instead of re-exporting it)? Else we'll get a test case that needs to be re-accepted every time someone changes a docstring on Product (or any of its many instances) and we end up committing into the source tree a pretty huge output.

Thanks for the patch in any case, and nice catch! 👍

[RyanGlScott]

The problem is that synifyDataTyConReturnKind is a code path that is only ever reached when Haddocking things that are loaded from external packages. (If you just inlined the definition of Product, the bug doesn't surface in the first place.) Product was a convenient choice since it's in base, which is by far the most readily available external package.

Is there another way to test something from an external package that would be more stable w.r.t. changes in Haddocks? I'm not too familiar with the Haddock test suite, so if there is already a prebuilt solution for this, I'm not aware of it.

[harpocrates]

Oops! Didn't mean to close this.

You are totally right, this code path isn't currently exercised by Haddock except for stuff form external packages (and instances). It will be once Hi Haddock is merged though... Let me think some more about how to best add a test case.

[harpocrates]

Thanks for the PR! 👍

We should eventually be able to turn this test case into something more self contained (once Hi Haddock lands), but until then it is better to have an inconvenient test and a fix than nothing.

[harpocrates]

@RyanGlScott Handling some of the merge conflicts between 8.6 and 8.8 and I just realized that this may already have been fixed by aa9644f#diff-6e1cbdc9e1a19780e3e767df4a17b7bdR323. Specifically, is there any case where the change this PR added (copied here for your convenience)

synifyDataTyConReturnKind :: TyCon -> Maybe (LHsKind GhcRn)
synifyDataTyConReturnKind tc
   = case splitFunTys (dropForAlls (tyConKind tc)) of
       (_, ret_kind)
         | isLiftedTypeKind ret_kind -> Nothing -- Don't bother displaying :: *
         | otherwise                 -> Just (synifyKindSig ret_kind)

does better than the change in ghc-8.8 (also copied here for your convenience)

synifyDataTyConReturnKind :: TyCon -> Maybe (LHsKind GhcRn)
synifyDataTyConReturnKind tc
  | isLiftedTypeKind ret_kind = Nothing -- Don't bother displaying :: *
  | otherwise                 = Just (synifyKindSig ret_kind)
  where ret_kind = tyConResKind tc

?

The test case you added (Bug1004) does not indicate any, but I figured I'd ask...

[RyanGlScott]

Ah, I didn't think about using tyConResKind—that's way cleaner. Assuming that Bug1004 still passes with the tyConResKind formulation of synifyDataTyConReturnKind, I'd favor using that over the formulation in this PR.