Implement bits.* builtins#149
Conversation
…n-policy-agent#127) Signed-off-by: Muhammad Umer Hammad <umerhammad010@gmail.com>
sspaink
left a comment
There was a problem hiding this comment.
The bitwise math matches OPA's big.Int semantics and the negative-shift / type-error message wording matches the reference. Two operand-validation gaps make it diverge from OPA on integer-valued and fractional floats, and one edge case throws an uncaught exception. All verified against the OPA binary and v1/topdown/bits.go. Findings 1 and 2 share one fix: the integer check should reject only non-integral/infinite values (Double.isFinite(d) && d == Math.rint(d)) and be applied to both x and the shift amount s — matching OPA, which accepts 2.0 but rejects 2.5.
| RegoNumber s = getArg(args, 1, RegoNumber.class); | ||
| requireInteger(x, "bits.lsh", 1); | ||
| requireUnsigned(s, "bits.lsh"); | ||
| return new RegoBigInt(x.getBigIntValue().shiftLeft(s.getBigIntValue().intValueExact())); |
There was a problem hiding this comment.
Shift operand s is never validated as an integer, so fractional shifts are silently truncated instead of erroring. lsh/rsh call requireInteger on x but only requireUnsigned on s — never requireInteger(s, ...). For bits.lsh(1, 2.5), RegoDecimal.getBigIntValue() does BigInteger.valueOf((long) 2.5) = 2, so this returns 4. OPA raises bits.lsh: operand 2 must be integer number but got floating-point number (undefined without --strict-builtin-errors, eval_type_error with it) — confirmed on the OPA binary. Reachable via JSON input {"s": 2.5}. Same issue in rsh at line 120.
| return new RegoBigInt(x.getBigIntValue().shiftRight(s.getBigIntValue().intValueExact())); | ||
| } | ||
|
|
||
| private static void requireInteger(RegoNumber n, String fn, int operandIndex) { |
There was a problem hiding this comment.
Integer-valued floats are wrongly rejected across all six functions. requireInteger throws for any isDecimal(), but OPA accepts floats that are exactly integral (its NumberToInt only fails when the value isn't big.Exact). Confirmed on the OPA binary: bits.and(2.0, 3) → 2, bits.negate(5.0) → -6, bits.lsh(1, 2.0) → 4, but all throw TypeError here. Reachable via JSON input {"n": 2.0}, which Jackson maps to RegoDecimal (RegoValueModule.java:182-183). Static Rego literals like 2.0 are safe (TypeUtils.parseStringToNumber folds whole floats to integer types), so only input-supplied values trigger it. Suggest checking integrality (Double.isFinite(d) && d == Math.rint(d)) rather than isDecimal().
| RegoNumber s = getArg(args, 1, RegoNumber.class); | ||
| requireInteger(x, "bits.lsh", 1); | ||
| requireUnsigned(s, "bits.lsh"); | ||
| return new RegoBigInt(x.getBigIntValue().shiftLeft(s.getBigIntValue().intValueExact())); |
There was a problem hiding this comment.
intValueExact() throws an uncaught ArithmeticException for shift amounts above Integer.MAX_VALUE. s.getBigIntValue().intValueExact() throws ArithmeticException (not a TypeError/BuiltinError) for e.g. bits.lsh(1, 2147483648), so it escapes the builtin error path as an unexpected exception. OPA instead does uint(b.Uint64()) and attempts the shift. Lower severity (huge shifts are impractical either way), but the failure mode differs from a clean Rego error. Also applies to rsh at line 120.
Summary
Implements bits.and, bits.or, bits.xor, bits.negate, bits.lsh, and bits.rsh as described in #127.
Changes
Testing
Closes #127.