Opened 9 years ago

Closed 9 years ago

#4335 closed bug (fixed)

fromRational broken for Ratio a

Reported by: daniel.is.fischer Owned by: simonmar
Priority: normal Milestone: 7.4.1
Component: libraries/base Version: 6.12.3
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect result at runtime Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

The Fractional instance for Ratio a in GHC.Real defines

    fromRational (x:%y) =  fromInteger x :% fromInteger y

For fixed-width Integral types, that produces invalid results:

Prelude Data.Ratio> fromRational (1 % 2^32) :: Ratio Int
1 % 0
Prelude Data.Ratio> fromRational (3 % (2^32+9)) :: Ratio Int
3 % 9

Via toRational, these can be ported back to Rational.

Ratio a is generally broken for fixed-width types:

Prelude Data.Ratio> 1 % (minBound :: Int)
(-1) % (-2147483648)

but the particular brokenness of fromRational can be alleviated by reducing:

    fromRational (x:%y) = fromInteger x % fromInteger y

{-# RULES
"fromRational/id"  fromRational = id :: Rational -> Rational
  #-}

I think it's better to throw a divide by zero error than to produce invalid results here.

Attachments (1)

fromRationalRatio.dpatch (50.6 KB) - added by daniel.is.fischer 9 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 9 years ago by igloo

Milestone: 7.2.1

Changed 9 years ago by daniel.is.fischer

Attachment: fromRationalRatio.dpatch added

comment:2 Changed 9 years ago by daniel.is.fischer

Status: newpatch

Patch with the suggested change. I don't see anything else we could do.

Please review and merge.

comment:3 Changed 9 years ago by simonmar

Owner: set to simonmar

comment:4 Changed 9 years ago by simonmar

Status: patchmerge

Applied, thanks!

Mon Oct 18 18:01:09 PDT 2010  Daniel Fischer <daniel.is.fischer@web.de>
  * FIX #4335
  fromRational :: Rational -> Ratio a produced invalid results for fixed-width
  types a. Reduce the fraction to avoid that.

comment:5 Changed 9 years ago by igloo

Resolution: fixed
Status: mergeclosed

Merged.

Note: See TracTickets for help on using tickets.