diff --git a/internal/compiler/wasm/wasm.go b/internal/compiler/wasm/wasm.go index 83ae97b538..f5d6e81e30 100644 --- a/internal/compiler/wasm/wasm.go +++ b/internal/compiler/wasm/wasm.go @@ -664,6 +664,8 @@ func (c *Compiler) compileBlock(block *ir.Block) ([]instruction.Instruction, err if err := c.compileScan(stmt, &instrs); err != nil { return nil, err } + case *ir.NopStmt: + instrs = append(instrs, instruction.Nop{}) case *ir.NotStmt: if err := c.compileNot(stmt, &instrs); err != nil { return nil, err @@ -776,6 +778,9 @@ func (c *Compiler) compileBlock(block *ir.Block) ([]instruction.Instruction, err instrs = append(instrs, instruction.I32Const{Value: 0}) instrs = append(instrs, instruction.I32Ne{}) instrs = append(instrs, instruction.BrIf{Index: 0}) + case *ir.ResetLocalStmt: + instrs = append(instrs, instruction.I32Const{Value: 0}) + instrs = append(instrs, instruction.SetLocal{Index: c.local(stmt.Target)}) case *ir.IsDefinedStmt: instrs = append(instrs, instruction.GetLocal{Index: c.local(stmt.Source)}) instrs = append(instrs, instruction.I32Eqz{}) diff --git a/internal/ir/ir.go b/internal/ir/ir.go index 9a79be8a81..791a915425 100644 --- a/internal/ir/ir.go +++ b/internal/ir/ir.go @@ -267,6 +267,13 @@ type AssignVarOnceStmt struct { Location } +// ResetLocalStmt resets a local variable to 0. +type ResetLocalStmt struct { + Target Local + + Location +} + // MakeStringStmt constructs a local variable that refers to a string constant. type MakeStringStmt struct { Index int @@ -476,6 +483,11 @@ type WithStmt struct { Location } +// NopStmt adds a nop instruction. Useful during development and debugging only. +type NopStmt struct { + Location +} + // ResultSetAdd adds a value into the result set returned by the query plan. type ResultSetAdd struct { Value Local diff --git a/internal/planner/planner.go b/internal/planner/planner.go index d431d69140..0d583c8f2b 100644 --- a/internal/planner/planner.go +++ b/internal/planner/planner.go @@ -177,8 +177,8 @@ func (p *Planner) planRules(rules []*ast.Rule) (string, error) { params := fn.Params[2:] - // Initialize return value for partial set/object rules. Complete docs do - // not require their return value to be initialized. + // Initialize return value for partial set/object rules. Complete document + // rules assign directly to `fn.Return`. switch rules[0].Head.DocKind() { case ast.PartialObjectDoc: fn.Blocks = append(fn.Blocks, p.blockWithStmt(&ir.MakeObjectStmt{Target: fn.Return})) @@ -186,6 +186,12 @@ func (p *Planner) planRules(rules []*ast.Rule) (string, error) { fn.Blocks = append(fn.Blocks, p.blockWithStmt(&ir.MakeSetStmt{Target: fn.Return})) } + // For complete document rules, allocate one local variable for output + // of the rule body + else branches. + // It is used to let ordered rules (else blocks) check if the previous + // rule body returned a value. + lresult := p.newLocal() + // At this point the locals for the params and return value have been // allocated. This will be the first local that can be used in each block. lnext := p.lnext @@ -208,7 +214,7 @@ func (p *Planner) planRules(rules []*ast.Rule) (string, error) { } // Ordered rules are nested inside an additional block so that execution - // can short-circuit. For unordered rules blocks can be added directly + // can short-circuit. For unordered rules, blocks can be added directly // to the function. var blocks *[]*ir.Block @@ -243,11 +249,14 @@ func (p *Planner) planRules(rules []*ast.Rule) (string, error) { if prev != nil { // Ordered rules are handled by short circuiting execution. The // plan will jump out to the extra block that was planned above. - p.appendStmt(&ir.IsUndefinedStmt{Source: fn.Return}) + p.appendStmt(&ir.IsUndefinedStmt{Source: lresult}) + } else { + // The first rule body resets the local, so it can be reused. + p.appendStmt(&ir.ResetLocalStmt{Target: lresult}) } // Complete and partial rules are treated as special cases of - // functions. If there are args, the first step is a no-op. + // functions. If there are no args, the first step is a no-op. err := p.planFuncParams(params, rule.Head.Args, 0, func() error { // Run planner on the rule body. @@ -258,7 +267,7 @@ func (p *Planner) planRules(rules []*ast.Rule) (string, error) { case ast.CompleteDoc: return p.planTerm(rule.Head.Value, func() error { p.appendStmt(&ir.AssignVarOnceStmt{ - Target: fn.Return, + Target: lresult, Source: p.ltarget, }) return nil @@ -300,6 +309,19 @@ func (p *Planner) planRules(rules []*ast.Rule) (string, error) { return "", err } } + + // rule[i] and its else-rule(s), if present, are done + if rules[i].Head.DocKind() == ast.CompleteDoc { + end := &ir.Block{} + p.appendStmtToBlock(&ir.IsDefinedStmt{Source: lresult}, end) + p.appendStmtToBlock( + &ir.AssignVarOnceStmt{ + Target: fn.Return, + Source: lresult, + }, + end) + *blocks = append(*blocks, end) + } } // Default rules execute if the return is undefined. diff --git a/internal/planner/planner_test.go b/internal/planner/planner_test.go index 80534bf567..a0a02dc0f9 100644 --- a/internal/planner/planner_test.go +++ b/internal/planner/planner_test.go @@ -224,6 +224,77 @@ func TestPlannerHelloWorld(t *testing.T) { note: "relation unify", queries: []string{`walk(input, [["foo", y], x])`}, }, + { + note: "else conflict-1", + queries: []string{`data.p.q`}, + modules: []string{ + `package p + + q { + false + } + else = true { + true + } + q = false + `, + }, + }, + { + note: "else conflict-2", + queries: []string{`data.p.q`}, + modules: []string{ + `package p + + q { + false + } + else = false { + true + } + q { + false + } + else = true { + true + }`, + }, + }, + { + note: "multiple function outputs (single)", + queries: []string{`data.p.r`}, + modules: []string{ + `package p + + p(a) = y { + y = a[_] + } + + r = y { + data.p.p([1, 2, 3], y) + } + `, + }, + }, + { + note: "multiple function outputs (multiple)", + queries: []string{`data.p.r`}, + modules: []string{ + `package p + + p(1, a) = y { + y = a + } + p(x, y) = z { + z = x + } + + r = y { + data.p.p(1, 0, y) + } + `, + }, + }, } for _, tc := range tests { @@ -261,6 +332,7 @@ func TestPlannerHelloWorld(t *testing.T) { type cmpWalker struct { needle interface{} loc string + found bool // stop comparing after first found needle } func (*cmpWalker) Before(interface{}) {} @@ -276,7 +348,8 @@ func (*cmpWalker) After(interface{}) {} // returned. This trap can be avoided by starting with a failing test, // and proceeding with caution. ;) func (f *cmpWalker) Visit(x interface{}) (ir.Visitor, error) { - if reflect.TypeOf(f.needle) == reflect.TypeOf(x) { + if !f.found && reflect.TypeOf(f.needle) == reflect.TypeOf(x) { + f.found = true expLoc := f.loc actLoc := getLocation(x) if expLoc != actLoc { @@ -402,7 +475,7 @@ p = x { &ir.AssignVarOnceStmt{}: `module-0.rego:3:9: p = {"foo": "bar"}`, }, where: func(p *ir.Policy) interface{} { - return p.Funcs.Funcs[0].Blocks[1] // default rule block + return p.Funcs.Funcs[0].Blocks[2] // default rule block }, }, { diff --git a/internal/wasm/sdk/opa/opa_test.go b/internal/wasm/sdk/opa/opa_test.go index fddc0b2f30..c798a8fc55 100644 --- a/internal/wasm/sdk/opa/opa_test.go +++ b/internal/wasm/sdk/opa/opa_test.go @@ -121,6 +121,39 @@ a = "c" { input > 2 }`, }, WantErr: "module.rego:3:1: var assignment conflict: internal error", }, + { + Description: "Runtime error/else conflict-1", + Query: `data.p.q`, + Policy: ` + q { + false + } + else = true { + true + } + q = false`, + Evals: []Eval{{}}, + WantErr: "module.rego:9:5: var assignment conflict: internal error", + }, + { + Description: "Runtime error/else conflict-2", + Query: `data.p.q`, + Policy: ` + q { + false + } + else = false { + true + } + q { + false + } + else = true { + true + }`, + Evals: []Eval{{}}, + WantErr: "module.rego:12:5: var assignment conflict: internal error", + }, // NOTE(sr): The next two test cases were used to replicate issue // https://github.com/open-policy-agent/opa/issues/2962 -- their raison d'être // is thus questionable, but it might be good to keep them around a bit. diff --git a/internal/wasm/sdk/test/e2e/exceptions.yaml b/internal/wasm/sdk/test/e2e/exceptions.yaml index 0ad98c8167..f6925e5ac0 100644 --- a/internal/wasm/sdk/test/e2e/exceptions.yaml +++ b/internal/wasm/sdk/test/e2e/exceptions.yaml @@ -1,4 +1,3 @@ # Exception Format is : "jsonpatch/set": "unexpected panic or evaluation error - https://github.com/open-policy-agent/opa/issues/2949" "jsonpatch/json_patch_tests": "unexpected panic or evaluation error - https://github.com/open-policy-agent/opa/issues/2949" -"elsekeyword/conflict-2": "expected error missing - https://github.com/open-policy-agent/opa/issues/2954" diff --git a/internal/wasm/sdk/test/e2e/external_test.go b/internal/wasm/sdk/test/e2e/external_test.go index 1d37a54353..45ff65b41f 100644 --- a/internal/wasm/sdk/test/e2e/external_test.go +++ b/internal/wasm/sdk/test/e2e/external_test.go @@ -131,19 +131,13 @@ func assert(t *testing.T, tc cases.TestCase, result *opa.Result, err error) { return } if err == nil { + if result != nil { + t.Fatalf("expected error, got result %s", result.Result) + } t.Fatal("expected error") } - // TODO: implement more specific error checking, for now log results and skip the test - if tc.WantErrorCode != nil { - t.Logf("\nExpected Code: %s\nGot Err: %s\n", *tc.WantErrorCode, err) - } - - if tc.WantError != nil { - t.Logf("\nExpected Err: %s\nGot Err: %s\n", *tc.WantError, err) - } - - t.Skip("Skipping test case: Error validation not supported") + assertErrorCode(t, *tc.WantErrorCode, err) } } @@ -204,7 +198,26 @@ func assertResultSet(t *testing.T, want []map[string]interface{}, sortBindings b if !a.Equal(b) { t.Fatalf("expected %v but got %v", a, b) } +} +func assertErrorCode(t *testing.T, expected string, actual error) { + t.Helper() + switch expected { + case "eval_conflict_error": + exps := []string{"var assignment conflict", "object insert conflict"} + found := false + for _, exp := range exps { + if strings.Contains(actual.Error(), exp) { + found = true + break + } + } + if !found { + t.Errorf("expected %q to contain one of %v", actual, exps) + } + default: + t.Errorf("unmatched error: %v (expected %s)", actual, expected) + } } func toAST(a interface{}) *ast.Term { diff --git a/test/cases/testdata/functionerrors/test-functionerrors-1012.yaml b/test/cases/testdata/functionerrors/test-functionerrors-1012.yaml index c9fabfbc6b..e4f0533f54 100644 --- a/test/cases/testdata/functionerrors/test-functionerrors-1012.yaml +++ b/test/cases/testdata/functionerrors/test-functionerrors-1012.yaml @@ -1,132 +1,16 @@ cases: - data: - a: - - 1 - - 2 - - 3 - - 4 - b: - v1: hello - v2: goodbye - c: - - x: - - true - - false - - foo - "y": - - null - - 3.14159 - z: - p: true - q: false - d: - e: - - bar - - baz - f: - - xs: - - 1 - ys: - - 2 - - xs: - - 2 - ys: - - 3 - g: - a: - - 1 - - 0 - - 0 - - 0 - b: - - 0 - - 2 - - 0 - - 0 - c: - - 0 - - 0 - - 0 - - 4 - h: - - - 1 - - 2 - - 3 - - - 2 - - 3 - - 4 - l: - - a: bob - b: -1 - c: - - 1 - - 2 - - 3 - - 4 - - a: alice - b: 1 - c: - - 2 - - 3 - - 4 - - 5 - d: null - m: [] - numbers: - - "1" - - "2" - - "3" - - "4" - strings: - bar: 2 - baz: 3 - foo: 1 - three: 3 modules: - | package test1 - p(__local0__) = y { - y = __local0__[_] + p(a) = y { + y = a[_] } r = y { data.test1.p([1, 2, 3], y) } - - | - package test2 - - p(1, __local1__) = y { - y = __local1__ - } - - p(2, __local2__) = y { - __local7__ = __local2__ + 1 - y = __local7__ - } - - r = y { - data.test2.p(3, 0, y) - } - - | - package test3 - - p(1, __local3__) = y { - y = __local3__ - } - - p(2, __local4__) = y { - __local8__ = __local4__ + 1 - y = __local8__ - } - - p(__local5__, __local6__) = z { - z = __local5__ - } - - r = y { - data.test3.p(1, 0, y) - } note: functionerrors/function output conflict single query: data.test1.r = x want_error: functions must not produce multiple outputs for same inputs diff --git a/test/cases/testdata/functionerrors/test-functionerrors-1013.yaml b/test/cases/testdata/functionerrors/test-functionerrors-1013.yaml index ce79ac8311..84c56bcca8 100644 --- a/test/cases/testdata/functionerrors/test-functionerrors-1013.yaml +++ b/test/cases/testdata/functionerrors/test-functionerrors-1013.yaml @@ -1,127 +1,15 @@ cases: - data: - a: - - 1 - - 2 - - 3 - - 4 - b: - v1: hello - v2: goodbye - c: - - x: - - true - - false - - foo - "y": - - null - - 3.14159 - z: - p: true - q: false - d: - e: - - bar - - baz - f: - - xs: - - 1 - ys: - - 2 - - xs: - - 2 - ys: - - 3 - g: - a: - - 1 - - 0 - - 0 - - 0 - b: - - 0 - - 2 - - 0 - - 0 - c: - - 0 - - 0 - - 0 - - 4 - h: - - - 1 - - 2 - - 3 - - - 2 - - 3 - - 4 - l: - - a: bob - b: -1 - c: - - 1 - - 2 - - 3 - - 4 - - a: alice - b: 1 - c: - - 2 - - 3 - - 4 - - 5 - d: null - m: [] - numbers: - - "1" - - "2" - - "3" - - "4" - strings: - bar: 2 - baz: 3 - foo: 1 - three: 3 modules: - - | - package test3 - - p(1, __local3__) = y { - y = __local3__ - } - - p(2, __local4__) = y { - __local8__ = __local4__ + 1 - y = __local8__ - } - - p(__local5__, __local6__) = z { - z = __local5__ - } - - r = y { - data.test3.p(1, 0, y) - } - - | - package test1 - - p(__local0__) = y { - y = __local0__[_] - } - - r = y { - data.test1.p([1, 2, 3], y) - } - | package test2 - p(1, __local1__) = y { - y = __local1__ + p(1, a) = y { + y = a } - p(2, __local2__) = y { - __local7__ = __local2__ + 1 - y = __local7__ + p(2, b) = y { + y = b + 1 } r = y { diff --git a/test/cases/testdata/functionerrors/test-functionerrors-1014.yaml b/test/cases/testdata/functionerrors/test-functionerrors-1014.yaml index 1e464c2bed..14a4ed116e 100644 --- a/test/cases/testdata/functionerrors/test-functionerrors-1014.yaml +++ b/test/cases/testdata/functionerrors/test-functionerrors-1014.yaml @@ -1,132 +1,20 @@ cases: - data: - a: - - 1 - - 2 - - 3 - - 4 - b: - v1: hello - v2: goodbye - c: - - x: - - true - - false - - foo - "y": - - null - - 3.14159 - z: - p: true - q: false - d: - e: - - bar - - baz - f: - - xs: - - 1 - ys: - - 2 - - xs: - - 2 - ys: - - 3 - g: - a: - - 1 - - 0 - - 0 - - 0 - b: - - 0 - - 2 - - 0 - - 0 - c: - - 0 - - 0 - - 0 - - 4 - h: - - - 1 - - 2 - - 3 - - - 2 - - 3 - - 4 - l: - - a: bob - b: -1 - c: - - 1 - - 2 - - 3 - - 4 - - a: alice - b: 1 - c: - - 2 - - 3 - - 4 - - 5 - d: null - m: [] - numbers: - - "1" - - "2" - - "3" - - "4" - strings: - bar: 2 - baz: 3 - foo: 1 - three: 3 modules: - | package test3 - p(1, __local3__) = y { - y = __local3__ + p(1, a) = y { + y = a } - p(2, __local4__) = y { - __local8__ = __local4__ + 1 - y = __local8__ - } - - p(__local5__, __local6__) = z { - z = __local5__ + p(x, y) = z { + z = x } r = y { data.test3.p(1, 0, y) } - - | - package test1 - - p(__local0__) = y { - y = __local0__[_] - } - - r = y { - data.test1.p([1, 2, 3], y) - } - - | - package test2 - - p(1, __local1__) = y { - y = __local1__ - } - - p(2, __local2__) = y { - __local7__ = __local2__ + 1 - y = __local7__ - } - - r = y { - data.test2.p(3, 0, y) - } note: functionerrors/function output conflict multiple query: data.test3.r = x want_error: functions must not produce multiple outputs for same inputs