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

Deprecate Numeric.readFloat #284

Open
wz1000 opened this issue Sep 3, 2024 · 6 comments
Open

Deprecate Numeric.readFloat #284

wz1000 opened this issue Sep 3, 2024 · 6 comments

Comments

@wz1000
Copy link

wz1000 commented Sep 3, 2024

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 using read for Float and Double instead.

@Bodigrim
Copy link
Collaborator

Bodigrim commented Sep 3, 2024

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.

@phadej
Copy link

phadej commented Sep 3, 2024

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.

@wz1000
Copy link
Author

wz1000 commented Sep 4, 2024

Which laws guarantee isInfinity works as expected for all Num instances? One counterexample is Num () or similar instances for unit types one might define.

@mixphix
Copy link
Collaborator

mixphix commented Sep 4, 2024

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.

@phadej
Copy link

phadej commented Sep 4, 2024

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.

@Bodigrim
Copy link
Collaborator

Bodigrim commented Sep 4, 2024

Which laws guarantee isInfinity works as expected for all Num 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")

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

4 participants