From 3672a3f892faed5da9de04f3699e3c24906004ab Mon Sep 17 00:00:00 2001 From: Torin Sandall Date: Wed, 14 Feb 2018 16:25:24 -0800 Subject: [PATCH] Improve call result handling Previously, if callers omitted output terms from call expressions, the result would be ignored. This was fine for most calls which would only return true if they were defined, however, for functions could return false this became confusing because an expression like "f(1)" where f(1) = false would be succeed and yield a result. With these changes, built-in functions that used to only return true always return true or false and the eval engine takes care to check if the result is false when the the caller omits the output term. This provides consistent behaviour across cases like... f(1) => undefined (previously {}) f(1,x) => {x:false} (previously {x:false}) neq(1,1,x) => {x:false} (previously undefined) neq(1,1,false) => {} (previously undefined) neq(1,1,true) => undefined (previously undefined) In the next set of changes, the Rego package will be updated to capture values for expressions like the first one above so that function calls behave like refs (i.e., their values are returned). --- ast/builtins.go | 26 ++++++++++++------------ ast/check_test.go | 18 ++++++++--------- ast/env.go | 5 +---- topdown/comparison.go | 5 +---- topdown/eval.go | 22 ++++++++++++++------ topdown/regex.go | 12 ++--------- topdown/strings.go | 18 +++-------------- topdown/topdown_test.go | 27 +++++++++++++++++++++++++ types/types.go | 45 +++-------------------------------------- types/types_test.go | 10 +-------- 10 files changed, 76 insertions(+), 112 deletions(-) diff --git a/ast/builtins.go b/ast/builtins.go index 3b183bd6a9..b18855e253 100644 --- a/ast/builtins.go +++ b/ast/builtins.go @@ -141,7 +141,7 @@ var Equality = &Builtin{ Infix: "=", Decl: types.NewFunction( types.Args(types.A, types.A), - types.T, + types.B, ), } @@ -155,7 +155,7 @@ var Assign = &Builtin{ Infix: ":=", Decl: types.NewFunction( types.Args(types.A, types.A), - types.T, + types.B, ), } @@ -169,7 +169,7 @@ var GreaterThan = &Builtin{ Infix: ">", Decl: types.NewFunction( types.Args(types.A, types.A), - types.T, + types.B, ), } @@ -179,7 +179,7 @@ var GreaterThanEq = &Builtin{ Infix: ">=", Decl: types.NewFunction( types.Args(types.A, types.A), - types.T, + types.B, ), } @@ -189,7 +189,7 @@ var LessThan = &Builtin{ Infix: "<", Decl: types.NewFunction( types.Args(types.A, types.A), - types.T, + types.B, ), } @@ -199,7 +199,7 @@ var LessThanEq = &Builtin{ Infix: "<=", Decl: types.NewFunction( types.Args(types.A, types.A), - types.T, + types.B, ), } @@ -209,7 +209,7 @@ var NotEqual = &Builtin{ Infix: "!=", Decl: types.NewFunction( types.Args(types.A, types.A), - types.T, + types.B, ), } @@ -219,7 +219,7 @@ var Equal = &Builtin{ Infix: "==", Decl: types.NewFunction( types.Args(types.A, types.A), - types.T, + types.B, ), } @@ -432,7 +432,7 @@ var RegexMatch = &Builtin{ types.S, types.S, ), - types.T, + types.B, ), } @@ -448,7 +448,7 @@ var GlobsMatch = &Builtin{ types.S, types.S, ), - types.T, + types.B, ), } @@ -517,7 +517,7 @@ var Contains = &Builtin{ types.S, types.S, ), - types.T, + types.B, ), } @@ -529,7 +529,7 @@ var StartsWith = &Builtin{ types.S, types.S, ), - types.T, + types.B, ), } @@ -541,7 +541,7 @@ var EndsWith = &Builtin{ types.S, types.S, ), - types.T, + types.B, ), } diff --git a/ast/check_test.go b/ast/check_test.go index 6b98b5a3c5..c4079e9f4e 100644 --- a/ast/check_test.go +++ b/ast/check_test.go @@ -70,7 +70,7 @@ func TestCheckInference(t *testing.T) { Var("z"): types.N, }}, {"array-nested", "[x, 1] = [true, y]", map[Var]types.Type{ - Var("x"): types.T, + Var("x"): types.B, Var("y"): types.N, }}, {"array-transitive", "y = [[2], 1]; [[x], 1] = y", map[Var]types.Type{ @@ -159,7 +159,7 @@ func TestCheckInference(t *testing.T) { 3]; x = a[1].foo[_].bar`, map[Var]types.Type{ - Var("x"): types.NewAny(types.NewNull(), types.T), + Var("x"): types.NewAny(types.NewNull(), types.B), }}, {"local-reference-var", ` @@ -191,8 +191,8 @@ func TestCheckInference(t *testing.T) { `, map[Var]types.Type{ Var("i"): types.N, Var("j"): types.S, - Var("k"): types.NewAny(types.S, types.N, types.T), - Var("x"): types.NewAny(types.S, types.N, types.T), + Var("k"): types.NewAny(types.S, types.N, types.B), + Var("x"): types.NewAny(types.S, types.N, types.B), }}, {"local-reference-var-any", ` a = [[], {}]; @@ -331,7 +331,7 @@ func TestCheckInferenceRules(t *testing.T) { ref string expected types.Type }{ - {"trivial", ruleset1, `data.a.trivial`, types.T}, + {"trivial", ruleset1, `data.a.trivial`, types.B}, {"complete-doc", ruleset1, `data.a.complete`, types.NewArray( []types.Type{types.NewObject( @@ -374,7 +374,7 @@ func TestCheckInferenceRules(t *testing.T) { types.NewAny( types.S, types.N, - types.T), + types.B), )}, {"iteration-keys", ruleset1, "data.iteration.keys", types.NewSet( @@ -400,11 +400,11 @@ func TestCheckInferenceRules(t *testing.T) { types.NewDynamicProperty(types.S, types.NewAny(types.S, types.N)), )}, - {"ref", ruleset1, "data.b.trivial_ref", types.T}, + {"ref", ruleset1, "data.b.trivial_ref", types.B}, {"ref-transitive", ruleset1, "data.b.transitive_ref", types.NewArray( []types.Type{ - types.T, + types.B, }, nil, )}, @@ -412,7 +412,7 @@ func TestCheckInferenceRules(t *testing.T) { {"prefix", ruleset1, `data.prefix.a.b`, types.NewObject( []*types.StaticProperty{{ "c", types.NewObject( - []*types.StaticProperty{{"d", types.T}}, + []*types.StaticProperty{{"d", types.B}}, types.NewDynamicProperty(types.S, types.A), ), }}, diff --git a/ast/env.go b/ast/env.go index 32ba5f3e87..f0830e5bd8 100644 --- a/ast/env.go +++ b/ast/env.go @@ -35,10 +35,7 @@ func (env *TypeEnv) Get(x interface{}) types.Type { case Null: return types.NewNull() case Boolean: - if x.Compare(Boolean(true)) == 0 { - return types.T - } - return types.F + return types.NewBoolean() case Number: return types.NewNumber() case String: diff --git a/topdown/comparison.go b/topdown/comparison.go index fd35d42744..96be984ac4 100644 --- a/topdown/comparison.go +++ b/topdown/comparison.go @@ -34,10 +34,7 @@ func compareEq(a, b ast.Value) bool { func builtinCompare(cmp compareFunc) FunctionalBuiltin2 { return func(a, b ast.Value) (ast.Value, error) { - if !cmp(a, b) { - return nil, BuiltinEmpty{} - } - return ast.Boolean(true), nil + return ast.Boolean(cmp(a, b)), nil } } diff --git a/topdown/eval.go b/topdown/eval.go index a7a3329597..c2bd2f7c85 100644 --- a/topdown/eval.go +++ b/topdown/eval.go @@ -775,7 +775,10 @@ func (e evalBuiltin) eval(iter unifyIterator) error { defer e.e.instr.startTimer(evalOpBuiltinCall) if len(operands) == numDeclArgs { - return iter() + if output.Value.Compare(ast.Boolean(false)) != 0 { + return iter() + } + return nil } return e.e.unify(e.terms[len(e.terms)-1], output, iter) }) @@ -828,24 +831,31 @@ func (e evalFunc) evalOneRule(iter unifyIterator, rule *ast.Rule, prev *ast.Term child := e.e.child(rule.Body) - b := make(ast.Array, len(e.terms)) + args := make(ast.Array, len(e.terms)) + for i := range rule.Head.Args { - b[i] = rule.Head.Args[i] + args[i] = rule.Head.Args[i] } - if len(b) == len(rule.Head.Args)+1 { - b[len(b)-1] = rule.Head.Value + if len(args) == len(rule.Head.Args)+1 { + args[len(args)-1] = rule.Head.Value } var result *ast.Term child.traceEnter(rule) - err := child.biunifyArrays(e.terms, b, e.e.bindings, child.bindings, func() error { + err := child.biunifyArrays(e.terms, args, e.e.bindings, child.bindings, func() error { return child.eval(func(child *eval) error { child.traceExit(rule) result = child.bindings.Plug(rule.Head.Value) + if len(e.terms) == len(rule.Head.Args) { + if result.Value.Compare(ast.Boolean(false)) == 0 { + return nil + } + } + // Partial evaluation should explore all rules and may not produce // a ground result so we do not perform conflict detection or // deduplication. See "ignore conflicts: functions" test case for diff --git a/topdown/regex.go b/topdown/regex.go index 426a256033..0296594611 100644 --- a/topdown/regex.go +++ b/topdown/regex.go @@ -30,10 +30,7 @@ func builtinRegexMatch(a, b ast.Value) (ast.Value, error) { if err != nil { return nil, err } - if re.Match([]byte(s2)) { - return ast.Boolean(true), nil - } - return nil, BuiltinEmpty{} + return ast.Boolean(re.Match([]byte(s2))), nil } func getRegexp(pat string) (*regexp.Regexp, error) { @@ -64,12 +61,7 @@ func builtinGlobsMatch(a, b ast.Value) (ast.Value, error) { if err != nil { return nil, err } - - if ne { - return ast.Boolean(true), nil - } - - return nil, BuiltinEmpty{} + return ast.Boolean(ne), nil } func init() { diff --git a/topdown/strings.go b/topdown/strings.go index 96f2125c1a..ef2902c4c9 100644 --- a/topdown/strings.go +++ b/topdown/strings.go @@ -134,11 +134,7 @@ func builtinContains(a, b ast.Value) (ast.Value, error) { return nil, err } - if !strings.Contains(string(s), string(substr)) { - return nil, BuiltinEmpty{} - } - - return ast.Boolean(true), nil + return ast.Boolean(strings.Contains(string(s), string(substr))), nil } func builtinStartsWith(a, b ast.Value) (ast.Value, error) { @@ -152,11 +148,7 @@ func builtinStartsWith(a, b ast.Value) (ast.Value, error) { return nil, err } - if !strings.HasPrefix(string(s), string(prefix)) { - return nil, BuiltinEmpty{} - } - - return ast.Boolean(true), nil + return ast.Boolean(strings.HasPrefix(string(s), string(prefix))), nil } func builtinEndsWith(a, b ast.Value) (ast.Value, error) { @@ -170,11 +162,7 @@ func builtinEndsWith(a, b ast.Value) (ast.Value, error) { return nil, err } - if !strings.HasSuffix(string(s), string(suffix)) { - return nil, BuiltinEmpty{} - } - - return ast.Boolean(true), nil + return ast.Boolean(strings.HasSuffix(string(s), string(suffix))), nil } func builtinLower(a ast.Value) (ast.Value, error) { diff --git a/topdown/topdown_test.go b/topdown/topdown_test.go index df68202357..ed8e5d0aab 100644 --- a/topdown/topdown_test.go +++ b/topdown/topdown_test.go @@ -1787,6 +1787,28 @@ func TestTopDownFunctions(t *testing.T) { data.test.foo(y[2], a) } + falsy_func(x) = false + + falsy_func_else(x) = true { x = 1 } else = false { true } + + falsy_undefined { + falsy_func(1) + } + + falsy_negation { + not falsy_func(1) + } + + falsy_else_value = falsy_func_else(2) + + falsy_else_undefined { + falsy_func_else(2) + } + + falsy_else_negation { + not falsy_func_else(2) + } + arrays([x, y]) = [a, b] { foo(x, a) foo(y, b) @@ -1918,6 +1940,11 @@ func TestTopDownFunctions(t *testing.T) { defer store.Abort(ctx, txn) assertTopDownWithPath(t, compiler, store, "basic call", []string{"ex", "bar", "alice"}, "", `["al", "ce"]`) + assertTopDownWithPath(t, compiler, store, "false result", []string{"ex", "falsy_undefined"}, "", ``) + assertTopDownWithPath(t, compiler, store, "false result negation", []string{"ex", "falsy_negation"}, "", `true`) + assertTopDownWithPath(t, compiler, store, "false else value", []string{"ex", "falsy_else_value"}, "", `false`) + assertTopDownWithPath(t, compiler, store, "false else undefined", []string{"ex", "falsy_else_undefined"}, "", ``) + assertTopDownWithPath(t, compiler, store, "false else negation", []string{"ex", "falsy_else_negation"}, "", `true`) assertTopDownWithPath(t, compiler, store, "chained", []string{"ex", "chain2"}, "", `["foo", "bar"]`) assertTopDownWithPath(t, compiler, store, "cross package", []string{"test", "cross"}, "", `["s f", [", my name "]]`) assertTopDownWithPath(t, compiler, store, "array params", []string{"ex", "arraysrule"}, "", `[["h", "h"], ["foo"]]`) diff --git a/types/types.go b/types/types.go index 0429f4cbcd..1847feddf6 100644 --- a/types/types.go +++ b/types/types.go @@ -60,22 +60,11 @@ func (t Null) String() string { } // Boolean represents the boolean type. -type Boolean struct { - v *bool -} +type Boolean struct{} // B represents an instance of the boolean type. var B = NewBoolean() -var t = true -var f = false - -// T represents an instance of the value true. -var T = Boolean{v: &t} - -// F represents an instance of the value false. -var F = Boolean{v: &f} - // NewBoolean returns a new Boolean type. func NewBoolean() Boolean { return Boolean{} @@ -86,20 +75,11 @@ func (t Boolean) MarshalJSON() ([]byte, error) { repr := map[string]interface{}{ "type": t.typeMarker(), } - if t.v != nil { - repr["value"] = *t.v - } return json.Marshal(repr) } func (t Boolean) String() string { - if t.v == nil { - return "boolean" - } else if *t.v { - return "true" - } else { - return "false" - } + return t.typeMarker() } // String represents the string type. @@ -537,26 +517,7 @@ func Compare(a, b Type) int { return -1 } switch a.(type) { - case nil: - return 0 - case Boolean: - bA := a.(Boolean) - bB := b.(Boolean) - if bA.v == nil && bB.v == nil { - return 0 - } else if bA.v != nil && bB.v != nil { - if *bA.v == *bB.v { - return 0 - } else if *bA.v { - return 1 - } else { - return -1 - } - } else if bA.v != nil { - return 1 - } - return -1 - case Null, Number, String: + case nil, Null, Boolean, Number, String: return 0 case *Array: arrA := a.(*Array) diff --git a/types/types_test.go b/types/types_test.go index a3f25858eb..3f3cb39f32 100644 --- a/types/types_test.go +++ b/types/types_test.go @@ -58,14 +58,6 @@ func TestCompare(t *testing.T) { {NewBoolean(), NewNull(), 1}, {NewBoolean(), NewBoolean(), 0}, {NewBoolean(), NewNumber(), -1}, - {T, B, 1}, - {B, T, -1}, - {T, T, 0}, - {F, B, 1}, - {B, F, -1}, - {F, F, 0}, - {T, F, 1}, - {F, T, -1}, {NewNumber(), NewNumber(), 0}, {NewNumber(), NewString(), -1}, {NewString(), NewString(), 0}, @@ -177,7 +169,7 @@ func TestOr(t *testing.T) { {NewAny(NewNull(), NewNumber()), NewAny(), NewAny()}, {NewAny(NewNumber(), NewString()), NewAny(NewNull(), NewBoolean()), NewAny(NewNull(), NewBoolean(), NewString(), NewNumber())}, {NewAny(NewNull(), NewNumber()), NewNull(), NewAny(NewNull(), NewNumber())}, - {NewFunction([]Type{S}, T), NewFunction([]Type{N}, T), NewFunction([]Type{NewAny(S, N)}, T)}, + {NewFunction([]Type{S}, B), NewFunction([]Type{N}, B), NewFunction([]Type{NewAny(S, N)}, B)}, } for _, tc := range tests {