Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arbitrary instance for CUSeconds doesn't work on x86_32 with QuickCheck >= 2.10.1 #201

Closed
arybczak opened this issue Jan 26, 2018 · 4 comments

Comments

@arybczak
Copy link

Consider the following:

module Main where

import Foreign.C.Types
import Test.QuickCheck

prop_eq :: CUSeconds -> Bool
prop_eq x = x == x

main :: IO ()
main = quickCheck prop_eq

When compiled on multilib x86_64 Gentoo Linux with 64bit GHC-8.0.2 and QuickCheck-2.11.3, the test passes:

+++ OK, passed 100 tests.

However, when compiled with 32bit GHC-8.0.2 and QuickCheck-2.11.3, it fails:

*** Failed! Exception while generating shrink-list: 'Enum.fromEnum{Word32}: value (4294967295) is outside of I
nt's bounds (-2147483648,2147483647)' (after 1 test):                                                        
Exception thrown while showing test case: 'Enum.fromEnum{Word32}: value (4294967295) is outside of Int's bound
s (-2147483648,2147483647)' 

Note that QuickCheck-2.10.0.1 works fine with both 32bit and 64bit GHC versions. It seems that Arbitrary instance for CUSeconds was broken in 2.10.1.

@nick8325
Copy link
Owner

Oh, interesting! We use fromEnum on CUSeconds, but this is actually a partial function on 32-bit GHC (because a CUSeconds is really a Word32 which does not fit in an Int), hence the crash.

Not immediately sure what to do about this, but it should be fixable.

The change in question was made to support GHC 7.2, which defines CUSeconds but doesn't export its constructor, meaning that we have to write the Arbitrary instance in an indirect way. One option would be just to drop this support and change back to the previous Arbitrary instance.

@arybczak
Copy link
Author

arybczak commented Feb 9, 2018

I'm all for dropping support for GHC 7.2, it's 6 years old.

@sol
Copy link
Contributor

sol commented Feb 10, 2018

The change in question was made to support GHC 7.2

If this is for 7.2 specifically, then I think there is no big benefit in having this. 7.2 was a tech preview which is/was not widely used for real world stuff. If it's also for 7.0.* then that is a separate consideration.

@nick8325 I assume this is not what you suggested, but for me it would be fine to drop support for 7.2.* and older all together. What I care about is that >= 7.4.1 works, that is what hspec still supports (AFAIK same for tasty).

@nick8325
Copy link
Owner

7.2 was a tech preview which is/was not widely used for real world stuff.

Ah, I'd forgotten about that. In that case I agree it makes sense to drop support for 7.2 altogether.

RyanGlScott added a commit to RyanGlScott/text-show that referenced this issue Apr 25, 2018
This is all needed due to a QuickCheck bug
(nick8325/quickcheck#201), the fix for
which hasn't landed on Hackage yet. See
#36.
RyanGlScott added a commit to RyanGlScott/text-show that referenced this issue Sep 3, 2018
QuickCheck-2.12 contains a fix for nick8325/quickcheck#201, which was
the culprit behind #36. Now that QuickCheck-2.12 is released, we can
properly fix #36 by depending on QuickCheck-2.12 as the minimum.
RyanGlScott added a commit to RyanGlScott/text-show-instances that referenced this issue Sep 3, 2018
I'm not sure if text-show-instances is affected by
nick8325/quickcheck#201 or not. But text-show definitely was (see
RyanGlScott/text-show#36), and since text-show-instances defines some
of the same Arbitrary instances for data types in Foreign.C.Types
that have different underlying representations based on OS and
architecture, it's better to be safe than sorry.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants