purescript-contrib / purescript-quickcheck-laws

QuickCheck powered law tests for PureScript's core typeclasses.
MIT License
25 stars 18 forks source link

Euclideanring overflow #30

Closed jacereda closed 3 years ago

garyb commented 7 years ago

This looks like it's about right to me, @hdgarrood what do you think? (I'm not really sure if saying that overflow = success is a safe assumption)

(Sorry about the delays on these @jacereda, will get these enum things in ASAP now - thanks very much for working on them).

hdgarrood commented 7 years ago

Yeah I agree, this assumption seems fine for Int but it’s not really clear to me that it makes sense for a general Euclidean ring. I think this could easily mask genuine errors for types where multiplication or division is harder to implement correctly, for instance.

I think the answer is really to use a newtype which only generates pairs of values which do not overflow when multiplied. Maybe problems like this are really a sign that the quickcheck approach of using a standard generator for any type is not the best approach here.

jacereda commented 7 years ago

I guess it isn't perfect, but isn't failing the test worse?

hdgarrood commented 7 years ago

No, I think a false negative (tests pass and miss an error they should have caught) is worse than a false positive (tests fail due to overflow which we were expecting anyway), especially since we can address the latter using a newtype.

JordanMartinez commented 3 years ago

Looks like the next action for this PR is to drop the overflow check and instead wrap Int in a newtype that limits how large the initial generated Int can be, so that an overflow error cannot occur. That would fix the false positive.

JordanMartinez commented 3 years ago

Superceded by #58