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

Avoid using unsafe numeric conversions fromInteger and fromIntegral #637

Open
brianhuffman opened this issue Jul 24, 2019 · 3 comments
Open
Labels
tech-debt For issues that require some internal refactoring.

Comments

@brianhuffman
Copy link
Contributor

Haskell functions fromInteger and fromIntegral are used in lots of places in the cryptol codebase. These functions are likely to cause bugs due to their wrap-around behavior when converting a larger type to a smaller one, like Integer to Int. (For example, see #636.)

We should avoid using these functions whenever possible. (I think this would be a good rule to put in our coding style guide, once we have one. Also if we're running hlint along with our automated builds, we could add rules about avoiding fromIntegral and fromInteger.)

Instead of fromInteger or fromIntegral, we should either use:

  • a wrapper function whose name explicitly highlights the wraparound behavior, for those cases when that's actually the behavior we want
  • a helper function like maybeFromInteger :: Integer -> Maybe Int that explicitly checks for overflow, for those cases where we don't want to wrap around

If there are some situations where fromInteger or fromIntegral really can't be avoided, we should at the very least add comments to each occurrence, explaining why it's OK in that particular situation.

@atomb
Copy link
Contributor

atomb commented Jul 15, 2020

We've fixed a bunch of these, but there are still some remaining. How many do we want to commit to removing before the 2.9 release?

@yav
Copy link
Member

yav commented Jul 15, 2020

I think this is more aspirational and shouldn't be a requirement for the 2.9 release

@atomb atomb removed this from the 2.9.0 milestone Jul 15, 2020
@brianhuffman
Copy link
Contributor Author

Once we add these safe numeric conversion functions, I think that will be sufficient to close this ticket.

@robdockins robdockins self-assigned this Jul 16, 2021
@robdockins robdockins removed their assignment Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech-debt For issues that require some internal refactoring.
Projects
None yet
Development

No branches or pull requests

4 participants