google / starlark-go

Starlark in Go: the Starlark configuration language, implemented in Go
BSD 3-Clause "New" or "Revised" License
2.27k stars 204 forks source link

Bug with math.ceil returning float #372

Closed Algebra8 closed 3 years ago

Algebra8 commented 3 years ago

The current implementation of Starlark's math.ceil will always return a starlark.Float as a result of being wrapped by newUnaryBuiltin.

Python's math.ceil, however, will always return an integral, as can be seen in the docs.

Also, Python's math.ceil will return an error when called with non-finite numeric values, i.e. nan, +inf, and -inf:

>>> math.ceil(float("nan"))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: cannot convert float NaN to integer
>>> math.ceil(float("inf"))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: cannot convert float infinity to integer
>>> math.ceil(float("-inf"))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: cannot convert float infinity to integer

But Starlark's implementation allows for them, as can be seen in the tests:

...
assert.eq(math.ceil(inf), inf)
assert.eq(math.ceil(nan), nan)
...
assert.eq(math.ceil(-inf), -inf)
...

This bug was first mentioned in #370, as can be seen in this thread.

Possible solution:

Extract math.ceil from newUnaryBuiltin into its own function, ceil, that returns the result as an integral and returns an error when said result is not finite.