search

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

Fix bug in math.ceil by returning integral instead of float (fixes #372) #373

Closed Algebra8 closed 3 years ago

Algebra8 commented 3 years ago

Python's math.ceil function always returns a value's ceiling as an integral, as can be seen in https://docs.python.org/3/library/math.html#math.ceil.

The previous iteration of Starlark's math.ceil, however, would always return a float, as a result of being wrapped in newUnaryBuiltin. Since ceil is different from the other wrapped methods, as described above, separate it in its own function.

It is worth noting that Python's math.ceil will also not accept non-finite values, such as nan, +inf, -inf and will return custom error messages for nan and infinities. These are also handled in the new ceil function.

Finally, tests are incorporated in testdata/math.star to reflect the changes described above.

fixes #372

Algebra8 commented 3 years ago

@adonovan

Hello!

Here is a PR to handle the bug discussed in #370.

I look forward to any feedback.

Thanks!

Algebra8 commented 3 years ago

It is also worth noting that there seems to be some issue with comparing types, as can be seen in the tests here.

Essentially, in Python:

>>> type(3) == "int"
False
>>> type(3) == int
True

But in Starlark, the opposite seems to be true. That is:

assert.eq(type(3), "int")  # ok
assert.eq(type(3), int)    # not ok

This probably has something to do with type_ returning a String, but I haven't looked to deep into it. If it is a bug I would be happy to have a go at fixing it.

adonovan commented 3 years ago

It is also worth noting that there seems to be some issue with comparing types

This is required by the spec, it's not a bug. I wouldn't have designed it this way, but it is what it is.

adonovan commented 3 years ago

(I really don't understand the theory of GitHub's PR comments UI. It won't let me respond to your comment directly.)

This seems to be because q is close enough to math.MaxInt64 that the check in finiteFloatToInt will register it as an int.

You've discovered a beautifully subtle bug which I have filed as https://github.com/google/starlark-go/issues/375 since it affects the interpreter core. I'll send a fix for that presently.

Algebra8 commented 3 years ago

@adonovan

Production code looks good. One last question about the tests! Thanks for your patience.

No, thank you for your patience and feedback! I really appreciate the time you've taken for it 🙏

It's unclear what the first constant signifies. float(1<<64)? The same thing plus or minus one? Derive the argument to make it clear.

Honestly, I think I just multiplied math.MaxInt by 2 and added 0.5, so thank you for bringing that up, it should be derived.

Is the second argument intentionally a float? It may compare equal to the int returned by ceil, but it would be better to use an expression that is actually an int.

Great point, and will do :)