Fix bug in math.ceil by returning integral instead of float (fixes go…

…ogle#372) (google#373)

* Fix bug in math.ceil by returning integral instead of float

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.

* Move ceil function signature to one line

* Prevent result of ceil from being lossy and abstract errors

Unnecessary conversions were occuring due to immediately wrapping
any input to ceil as a floatOrInt. By changing the expected input to accept
a starlark Value, an Int type can be returned immediately.

Also, MakeInt64 being called on the result of a float input could result in
the conversion of the result to be lossy. This is handled by abstracting the
conversion with NumberToInt.

NumberToInt also returns the exact error messages that were included previously,
so it is not required to rewrite them.

* Add test for float value larger than int64

* Fix bugs with floor function returning floats

The bugs that affected Starlark's math.ceil also affect math.floor. Therefore,
a new floor function is introduced that handles those bugs. Note floor is similar
to ceil except that it calls Go's Floor instead of Ceil.

* Add tests to floor that represent fixes

* Make float return one line

* Make minor refactorings: join lines

* Make minor refactorings: remove unnecessary comments

* Derive float args for ceil and floor

* Use bit shifting and same pattern for floor, ceil big float test

lib/math/math.go

@@ -67,10 +67,10 @@ import (
 var Module = &starlarkstruct.Module{
 	Name: "math",
 	Members: starlark.StringDict{
-		"ceil":      newUnaryBuiltin("ceil", math.Ceil),
+		"ceil":      starlark.NewBuiltin("ceil", ceil),
 		"copysign":  newBinaryBuiltin("copysign", math.Copysign),
 		"fabs":      newUnaryBuiltin("fabs", math.Abs),
-		"floor":     newUnaryBuiltin("floor", math.Floor),
+		"floor":     starlark.NewBuiltin("floor", floor),
 		"mod":       newBinaryBuiltin("round", math.Mod),
 		"pow":       newBinaryBuiltin("pow", math.Pow),
 		"remainder": newBinaryBuiltin("remainder", math.Remainder),
@@ -162,6 +162,40 @@ func log(thread *starlark.Thread, b *starlark.Builtin, args starlark.Tuple, kwar
 	return starlark.Float(math.Log(float64(x)) / math.Log(float64(base))), nil
 }
 
+func ceil(thread *starlark.Thread, _ *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) {
+	var x starlark.Value
+
+	if err := starlark.UnpackPositionalArgs("ceil", args, kwargs, 1, &x); err != nil {
+		return nil, err
+	}
+
+	switch t := x.(type) {
+	case starlark.Int:
+		return t, nil
+	case starlark.Float:
+		return starlark.NumberToInt(starlark.Float(math.Ceil(float64(t))))
+	}
+
+	return nil, fmt.Errorf("got %s, want float or int", x.Type())
+}
+
+func floor(thread *starlark.Thread, _ *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) {
+	var x starlark.Value
+
+	if err := starlark.UnpackPositionalArgs("floor", args, kwargs, 1, &x); err != nil {
+		return nil, err
+	}
+
+	switch t := x.(type) {
+	case starlark.Int:
+		return t, nil
+	case starlark.Float:
+		return starlark.NumberToInt(starlark.Float(math.Floor(float64(t))))
+	}
+
+	return nil, fmt.Errorf("got %s, want float or int", x.Type())
+}
+
 func degrees(x float64) float64 {
 	return 360 * x / (2 * math.Pi)
 }

starlark/testdata/math.star

@@ -17,16 +17,25 @@ assert.eq(math.ceil(10.0), 10.0)
 assert.eq(math.ceil(0), 0.0)
 assert.eq(math.ceil(1), 1.0)
 assert.eq(math.ceil(10), 10.0)
-assert.eq(math.ceil(inf), inf)
-assert.eq(math.ceil(nan), nan)
 assert.eq(math.ceil(-0.0), 0.0)
 assert.eq(math.ceil(-0.4), 0.0)
 assert.eq(math.ceil(-0.5), 0.0)
 assert.eq(math.ceil(-1.0), -1.0)
 assert.eq(math.ceil(-10.0), -10.0)
 assert.eq(math.ceil(-1), -1.0)
 assert.eq(math.ceil(-10), -10.0)
-assert.eq(math.ceil(-inf), -inf)
+assert.eq(type(math.ceil(0)), "int")
+assert.eq(type(math.ceil(0.4)), "int")
+assert.eq(type(math.ceil(10)), "int")
+assert.eq(type(math.ceil(-10.0)), "int")
+assert.eq(type(math.ceil(-0.5)), "int")
+assert.eq(math.ceil((1<<63) + 0.5), int(float((1<<63) + 1)))
+assert.fails(
+  lambda: math.ceil(inf), "cannot convert float infinity to integer")
+assert.fails(
+  lambda: math.ceil(-inf), "cannot convert float infinity to integer")
+assert.fails(
+  lambda: math.ceil(nan), "cannot convert float NaN to integer")
 assert.fails(lambda: math.ceil("0"), "got string, want float or int")
 # fabs
 assert.eq(math.fabs(2.0), 2.0)
@@ -45,14 +54,23 @@ assert.eq(math.floor(0.4), 0.0)
 assert.eq(math.floor(0.5), 0.0)
 assert.eq(math.floor(1.0), 1.0)
 assert.eq(math.floor(10.0), 10.0)
-assert.eq(math.floor(inf), inf)
-assert.eq(math.floor(nan), nan)
 assert.eq(math.floor(-0.0), 0.0)
 assert.eq(math.floor(-0.4), -1.0)
 assert.eq(math.floor(-0.5), -1.0)
 assert.eq(math.floor(-1.0), -1.0)
 assert.eq(math.floor(-10.0), -10.0)
-assert.eq(math.floor(-inf), -inf)
+assert.eq(type(math.floor(0)), "int")
+assert.eq(type(math.floor(0.4)), "int")
+assert.eq(type(math.floor(10)), "int")
+assert.eq(type(math.floor(-10.0)), "int")
+assert.eq(type(math.floor(-0.5)), "int")
+assert.eq(math.floor((1<<63) + 0.5), int(float(1<<63)))
+assert.fails(
+  lambda: math.floor(inf), "cannot convert float infinity to integer")
+assert.fails(
+  lambda: math.floor(-inf), "cannot convert float infinity to integer")
+assert.fails(
+  lambda: math.floor(nan), "cannot convert float NaN to integer")
 assert.fails(lambda: math.floor("0"), "got string, want float or int")
 # mod
 assert.eq(math.mod(5, 3), 2)