Fix panic for modulo on floating-point numbers #1247
Conversation
topdown/arithmetic_test.go
Outdated
| "testing" | ||
| ) | ||
|
|
||
| func TestBuiltinRem(t *testing.T) { |
There was a problem hiding this comment.
Nit: we have test cases for the mod operator in the topdown_test.go file (search for "remainder")
topdown/arithmetic_test.go
Outdated
| rules []string | ||
| expected interface{} | ||
| }{ | ||
| {"integer modulo remainder", []string{`p[x] { x := 1 % 1 }`}, "[0]"}, |
There was a problem hiding this comment.
We should have test cases for error paths.
topdown/arithmetic.go
Outdated
| _, ok2 = n2.Int() | ||
|
|
||
| if ok1 && ok2 { | ||
| i, err := arithRem(builtins.NumberToInt(n1), builtins.NumberToInt(n2)) |
There was a problem hiding this comment.
The code above is converting to int64 but then this is converting to big.Int and using big.Int's remainder operation (which is a bit odd.)
It doesn't look like like Go's big number package supports remainder operations on big.Float. Implementing the latter requires care. In the short term, let's return an error for mod on floating point numbers and make sure mod on big integers (e.g., numbers that exceed int64 range) produce correct answers.
There was a problem hiding this comment.
Updated code to return error if non-integer mod operation is performed.
ashutosh-narkar
force-pushed
the
modulo-panic
branch
from
f30c70a to
389bf08
Compare
topdown/arithmetic.go
Outdated
| op2, err2 := builtins.NumberToInt(n2) | ||
|
|
||
| if err1 != nil || err2 != nil { | ||
| return nil, fmt.Errorf("mod operation supported on integers only") |
There was a problem hiding this comment.
It might just be me, but I think this error reads a bit strange.
If you run this in the REPL, this is what you get:
> 1.1 % 2
1.1 % 2: eval_internal_error: rem: mod operation supported on integers only
modulo on floating-point number
One thing to note is that because we parse integers using big.Float in all cases, large integer values get represented in scientific notation which big.Int doesn't parse. We should file a separate bug to fix that.
There was a problem hiding this comment.
Updated the error message.
Fixes open-policy-agent#1245 Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
ashutosh-narkar
force-pushed
the
modulo-panic
branch
from
2a9c0a1 to
10a9aca
Compare
tsandall
changed the title
Fixes #1245
Currently modulo operator (%) panics when the input is a floating point number. This change adds support to perform floating-point remainder operation on floating point numbers.
Signed-off-by: Ashutosh Narkar anarkar4387@gmail.com