Repository navigation
Fail instead of returning inf when an int is too large to convert to float - #254
Closed
Dilshad-Tanvi wants to merge 1 commit into
Closed
Dilshad-Tanvi wants to merge 1 commit into
Dilshad-Tanvi wants to merge 1 commit into
Conversation
…float NumRef::as_float converts a big int with BigInt::to_f64, which yields +/-inf outside the f64 range, so float(1 << 1024), mixed int/float arithmetic and %f formatting silently returned inf. The spec says these conversions fail, and float() of the same number as a string already does. Convert through a checked NumRef::as_finite_float instead, and stop filtering the Go conformance assertions that cover it.
meta-cla
Bot
added
the
CLA Signed
Contributor
|
This pull request has been imported. If you are a Meta employee, you can view this in D126649355. (Because this pull request was imported automatically, there will not be any future comments.) |
JakobDegen
approved these changes
Oct 21, 2026
JakobDegen
left a comment
JakobDegen
left a comment
Contributor
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
meta-codesync Bot
pushed a commit
to facebook/buck2
that referenced
this pull request
Oct 21, 2026
…float Summary: Converting an int to a float goes through `NumRef::as_float`, which uses `BigInt::to_f64` and gives `+inf`/`-inf` once the int is outside the f64 range. So `float(1 << 1024)` returns `+inf`, `(1 << 1024) + 0.5`, `* 1.0`, `//` and `%` all quietly come out as `inf`, `(1 << 1100) / (1 << 1000)` is `+inf` instead of `1.2676506002282294e+30`, and `"%f" % (1 << 1024)` prints `+inf`. The spec says `float(x)` "fails if x is too large to represent as a finite float" and that the implicit conversion in mixed arithmetic fails the same way. starlark-go reports `int too large to convert to float`, and `float()` of the same number written as a string already errors here. The fix adds `NumRef::as_finite_float`, which errors for an int whose conversion is infinite, and uses it in `float()`, the mixed `+ - * / // %` paths and `%e/%f/%g`. Int/int arithmetic, comparisons and hashing are unchanged. `testcases/eval/go/float.star` already has the starlark-go assertions for these cases, but `tests/go.rs` filtered them out. Those lines now run and pass, and they fail without the fix. `cargo test` and `cargo clippy` are clean. X-link: facebook/starlark-rust#254 Reviewed By: JakobDegen Differential Revision: D126649355 fbshipit-source-id: 52ab66d4d0c365f3254dd8b0f1481f949d049d23
Contributor
|
This pull request has been merged in 89d4a04. |
Contributor
Author
|
Thanks for landing this, glad the float conversion is strict now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Converting an int to a float goes through
NumRef::as_float, which usesBigInt::to_f64and gives+inf/-infonce the int is outside the f64 range. Sofloat(1 << 1024)returns+inf,(1 << 1024) + 0.5,* 1.0,//and%all quietly come out asinf,(1 << 1100) / (1 << 1000)is+infinstead of1.2676506002282294e+30, and"%f" % (1 << 1024)prints+inf. The spec saysfloat(x)"fails if x is too large to represent as a finite float" and that the implicit conversion in mixed arithmetic fails the same way. starlark-go reportsint too large to convert to float, andfloat()of the same number written as a string already errors here.The fix adds
NumRef::as_finite_float, which errors for an int whose conversion is infinite, and uses it infloat(), the mixed+ - * / // %paths and%e/%f/%g. Int/int arithmetic, comparisons and hashing are unchanged.testcases/eval/go/float.staralready has the starlark-go assertions for these cases, buttests/go.rsfiltered them out. Those lines now run and pass, and they fail without the fix.cargo testandcargo clippyare clean.