search

nick8325 / quickcheck

Automatic testing of Haskell programs.
Other
713 stars 119 forks source link

Compatibility with TH-2.17 (upcoming GHC 8.12) #298

Closed vdukhovni closed 3 years ago

vdukhovni commented 4 years ago

In TH-2.17 type variable binders get a specificity parameter

This PR adapts 'deconstructType' to handle these, and fixes an apparent bug where KindedTV n StarT was considered "plain", but the original code to extract the names only supported PlainTV n.

goldfirere commented 4 years ago

Without more context, I can't evaluate whether this is correct or not. What is the specification of deconstructType? Ignoring specificity does seem reasonable, unless QuickCheck uses type application in its TH code somewhere.

But I see another bug here: you reject forall (a :: Type -> Type). a Int but not forall a. a Int. The latter has a higher-kinded type variable, too! I know this would be hard to reject in TH code (it would need type inference), but the fact that these types are treated differently here makes me worried.

phadej commented 4 years ago

The head.hackage patch looks quite different: https://gitlab.haskell.org/ghc/head.hackage/-/blob/master/patches/QuickCheck-2.14.patch

... and I'd recommend to use head.hackage until GHC-8.12 is closer to release. Or to put it differently, let's not track moving target - which GHC-8.12 is atm.

vdukhovni commented 4 years ago

The head.hackage patch looks quite different: https://gitlab.haskell.org/ghc/head.hackage/-/blob/master/patches/QuickCheck-2.14.patch

... and I'd recommend to use head.hackage until GHC-8.12 is closer to release. Or to put it differently, let's not track moving target - which GHC-8.12 is atm.

Thanks for the pointer. I think that despite all appearances, that patch is actually doing exactly the same thing. It ignores specificities and accepts the same sufficiently simple-looking bindings. I did not know about head.hackage, and needed a work-around. The maintainer can choose whatever idiom is preferable when the dust settles... :-)

nick8325 commented 3 years ago

I ended up merging #314 instead, which is very similar. Thanks for the patch!