Add bitwise operators after bits..

[mjgpy3]

I'm interested in this project and thought it'd be fun to tackle a "low-hanging-fruit" issue.

Fixes #1919

Sign off in commit.

[srenatus]

Nicely done! 👏 🚀 Thanks

[srenatus]

[nit]

Suggested change

	// Copyright 2019 The OPA Authors. All rights reserved.
	// Copyright 2020 The OPA Authors. All rights reserved.

I guess? 😉

[mjgpy3]

Done!

[patrick-east]

Changes look good! Just a couple comments on some test improvements we could make.

[patrick-east]

It might be good to be explicit and mention that x and y must be integers

[mjgpy3]

Do the updates look okay here?

[patrick-east]

Yep, perfect!

[patrick-east]

Please add some negative tests for the builtins with invalid types passed in both for the function signature check like passing in a string or something and then for passing in a valid ast.Number but a float that fails to convert to an integer.

[mjgpy3]

Done!

[patrick-east]

It might be worthwhile to add a test case that would cause something smaller than big.int's to overflow to ensure that we don't regress anywhere in the future by transitioning through something smaller (like golang 32 or 64 bit int) and lose bits.

[mjgpy3]

@patrick-east, I'm at a bit of an impasse here and could use assistance, if you have any recommendations.

I tested MaxInt32 shifted left by one (which would cause issues if we were just using int32s) and everything worked okay.

I went to try MaxInt64 using a test like the following

{
	"shift of max int64 doesn't overflow",
	[]string{fmt.Sprintf(`p = x { x := bits.lsh(%d, 1) }`, math.MaxInt64)},
	`18446744073709551614`,
},

However, since the ast's using json.Number under the hood (seems to just be a type definition wrapping string), math.MaxInt64 gets stored as "9.223372037e+18". Then later, when I try to parse it using NumberToInt which uses the following under-the hood

new(big.Int).SetString(string(n), 10)

big.Int can't parse it.

Any ideas? Am I missing something?

[patrick-east]

Hmmm, yea so I guess other places we always use big.Float's which has a Parse helper https://golang.org/pkg/math/big/#Float.Parse so it "just works". What we might need to do is something like:

• Get ast.Number operand (ie json.Number)
• Get big.Float by parsing the number
• Get a big.Int from the big.Float via https://golang.org/pkg/math/big/#Float.Int and validate that the returned accuracy is Exact (otherwise we didn't get a valid int to start with).

So maybe if we update NumberToInt to be more like:

func NumberToInt(n ast.Number) (*big.Int, error) {
	f := NumberToFloat(n)
	r, accuracy := f.Int(nil)
	if accuracy != big.Exact {
		return nil, fmt.Errorf("illegal value")
	}
	return r, nil
}

Does that work?

[mjgpy3]

@patrick-east that almost works.

The test I posted is outputting the following

            Got: 18446744074000000000
            Exp: 18446744073709551614

I believe that something odd is happening with big integers in general here. I totally could be wrong but I think go's JSON library is actually lossy in that it's converting big integers to exponent-notation.

For example, here's another test (just using modulus to try to recover a digit that I believe is being lost)

		{
			"big numbers JSON?",
			[]string{fmt.Sprintf(`p = x { x := [%d %% 10, %d] }`, math.MaxInt64, math.MaxInt64)},
			fmt.Sprintf("[%d, %d]", math.MaxInt64%10, math.MaxInt64),
		},

It outputs a failure of

            Got: [0 9.223372037e+18]
            Exp:
            [7 9223372036854775807]

[patrick-east]

Hmm yea for sure we do lossy things when we transition between them in the builtin helpers, for example:

opa/topdown/builtins/builtins.go

Lines 154 to 157 in db922a7

	// FloatToNumber converts f to a number.
	func FloatToNumber(f *big.Float) ast.Number {
	return ast.Number(f.String())
	}

uses the .String() method that defaults to limit the precision to 10 in https://golang.org/pkg/math/big/#Float.String when it calls https://golang.org/pkg/math/big/#Float.Text (similar for the int's)

I think the golang JSON library just keeps numbers as strings so it might be ok if we fix how we convert them and just give longer strings.

This reminded me that we have #501 which has been open for a long time.

I guess for now maybe we just add a note to document this behavior with large numbers and add test cases for the truncated results (they'll need to be updated when #501 is addressed). I don't want to block this going in for this kind of systematic problem with large numbers in opa.

[mjgpy3]

@patrick-east, that sounds good. I added that test.

[srenatus]

💭Makes me wonder if we could use binary/hexadecimal/octal literals... 0x1001 etc.

[patrick-east]

💭Makes me wonder if we could use binary/hexadecimal/octal literals... 0x1001 etc.

I think the compatibility is basically that it must be valid JSON, and AFAIK those types of numbers are not supported. You get base 10 with optional e+ stuff for bigger ones.

It might be another good "low hanging fruit" feature to extend to_number (or maybe add a new one) to be able to parse a string with more formats.

Edit: Updating the language itself to recognize them might also be useful, as-is they are parse errors if you try and drop in 0x1001 into your policy as a number.

[srenatus]

Edit: Updating the language itself to recognize them might also be useful, as-is they are parse errors if you try and drop in 0x1001 into your policy as a number.

Yup, that's what I had in mind. bits.and(flags, 0x1000) could be more natural for some use cases, I suppose? (Also octal for file permissions.)