Open wz1000 opened 3 weeks ago
As I mentioned in https://gitlab.haskell.org/ghc/ghc/-/issues/23538#note_556813, it is possible to fix the bug without changing the type signature, even if in an ugly way.
If we deprecate a function, what do we recommend to use instead? If we just push users to reimplement readFloat
themselves, they are likely to come up with a vulnerable implementation as well.
If we deprecate a function, what do we recommend to use instead? I
we should deprecate it and point users towards using read for Float and Double instead.
Which laws guarantee isInfinity
works as expected for all Num
instances? One counterexample is Num ()
or similar instances for unit types one might define.
The current documentation already points to read
, but readFloat
is used in a ReadS
context that read
can't replicate. It's better to fix the function to continue allowing parsing floating points as ReadS
, however more quickly, than to deprecate the functionality and frustrate a ReadS
user with having to write their own floating point parser.
I understand read
to mean Read
instance and utilities in general (read
is not a member of Read
class, but defined using the members), which has readsPrec :: Int -> ReadS a
and other (better) ways to read.
I feel that readFloat
is just a bad function. I don't understand why CLC is so reluctant. There were no problem to adding a warning to head
, I feel this in the same ballpark: dangerous function, don't use, there are better alternatives.
Which laws guarantee
isInfinity
works as expected for allNum
instances?
When you fold a binary (or decimal) string into a number, you do either \acc -> 2 * acc
or \acc -> 2 * acc + 1
. If acc
is an absorbing element both for addition and multiplication (which is exactly what isInfinite x = x == x - 1 && x == x * 2
checks for), you can shortcut folding early and return acc
as is.
Maybe it would be clearer to rename isInfinite
to isAbsorbingForAddAndMul
.
One counterexample is
Num ()
or similar instances for unit types one might define.
The proposed approach works fine for ()
: we have isInfinite () = True
, so we can shortcut and return ()
immediately, which is correct behaviour.
I don't understand why CLC is so reluctant. There were no problem to adding a warning to
head
No one except me voiced their opinions so far. I don't think that even I am "so reluctant", maybe just not very eager. (Adding warning to head
took 100+ comments and 2 months of heated discussion, I would not describe it as "no problem")
In GHC #2358 it was discovered that
Numeric.readFloat
takes time linear in the size of the denoted number. This also resulted in the HSEC-2023-0007 advisory.Since it is not possible to fix this bug while maintaining the same type signature as
readFloat
, we should deprecate it and point users towards usingread
forFloat
andDouble
instead.