From edab7bcc7fe08e48f81222a397a1373060d0cff1 Mon Sep 17 00:00:00 2001 From: Torin Sandall Date: Mon, 27 Jun 2016 14:06:18 -0700 Subject: [PATCH 1/4] Update parser extension to take filename --- ast/compile_test.go | 5 +-- ast/parser_ext.go | 76 ++++++++++++++++-------------------------- ast/parser_test.go | 6 ++-- repl/repl.go | 4 +-- runtime/runtime.go | 2 +- runtime/server.go | 2 +- storage/policystore.go | 2 +- 7 files changed, 37 insertions(+), 60 deletions(-) diff --git a/ast/compile_test.go b/ast/compile_test.go index 4e9b8c5c31..a82e522bc6 100644 --- a/ast/compile_test.go +++ b/ast/compile_test.go @@ -75,10 +75,7 @@ func TestCompilerEmpty(t *testing.T) { func TestCompilerExample(t *testing.T) { c := NewCompiler() - m, err := ParseModule(testModule) - if err != nil { - panic(err) - } + m := MustParseModule(testModule) c.Compile(map[string]*Module{"testMod": m}) assertNotFailed(t, c) } diff --git a/ast/parser_ext.go b/ast/parser_ext.go index f050fc887e..d1cc892ed6 100644 --- a/ast/parser_ext.go +++ b/ast/parser_ext.go @@ -12,8 +12,6 @@ package ast import ( "fmt" - "io/ioutil" - "os" "github.com/pkg/errors" ) @@ -31,7 +29,7 @@ func MustParseBody(input string) Body { // MustParseModule returns a parsed module. // If an error occurs during parsing, panic. func MustParseModule(input string) *Module { - parsed, err := ParseModule(input) + parsed, err := ParseModule("", input) if err != nil { panic(err) } @@ -41,7 +39,7 @@ func MustParseModule(input string) *Module { // MustParseStatements returns a slice of parsed statements. // If an error occurs during parsing, panic. func MustParseStatements(input string) []interface{} { - parsed, err := ParseStatements(input) + parsed, err := ParseStatements("", input) if err != nil { panic(err) } @@ -122,26 +120,8 @@ func ParseConstantRule(body Body) *Rule { // ParseModule returns a parsed Module object. // For details on Module objects and their fields, see policy.go. // Empty input will return nil, nil. -func ParseModule(input string) (*Module, error) { - stmts, err := ParseStatements(input) - if err != nil { - return nil, err - } - return parseModule(stmts) -} - -// ParseModuleFile returns a parsed Module object. -func ParseModuleFile(filename string) (*Module, error) { - f, err := os.Open(filename) - if err != nil { - return nil, err - } - defer f.Close() - bs, err := ioutil.ReadAll(f) - if err != nil { - return nil, err - } - stmts, err := ParseStatements(string(bs)) +func ParseModule(filename, input string) (*Module, error) { + stmts, err := ParseStatements(filename, input) if err != nil { return nil, err } @@ -151,7 +131,7 @@ func ParseModuleFile(filename string) (*Module, error) { // ParseBody returns exactly one body. // If multiple bodies are parsed, an error is returned. func ParseBody(input string) (Body, error) { - stmts, err := ParseStatements(input) + stmts, err := ParseStatements("", input) if err != nil { return nil, err } @@ -182,10 +162,23 @@ func ParseTerm(input string) (*Term, error) { return term, nil } +// ParseRef returns exactly one reference. +func ParseRef(input string) (Ref, error) { + term, err := ParseTerm(input) + if err != nil { + return nil, errors.Wrap(err, "failed to parse ref") + } + ref, ok := term.Value.(Ref) + if !ok { + return nil, fmt.Errorf("expected ref but got %v", term) + } + return ref, nil +} + // ParseRule returns exactly one rule. // If multiple rules are parsed, an error is returned. func ParseRule(input string) (*Rule, error) { - stmts, err := ParseStatements(input) + stmts, err := ParseStatements("", input) if err != nil { return nil, err } @@ -199,24 +192,12 @@ func ParseRule(input string) (*Rule, error) { return rule, nil } -// ParseStatements returns a slice of parsed statements. -// This is the default return value from the parser. -func ParseStatements(input string) ([]interface{}, error) { - parsed, err := Parse("", []byte(input)) - if err != nil { - return nil, err - } - stmts := parsed.([]interface{}) - postProcess(stmts) - return stmts, err -} - // ParseStatement returns exactly one statement. // A statement might be a term, expression, rule, etc. Regardless, // this function expects *exactly* one statement. If multiple // statements are parsed, an error is returned. func ParseStatement(input string) (interface{}, error) { - stmts, err := ParseStatements(input) + stmts, err := ParseStatements("", input) if err != nil { return nil, err } @@ -226,17 +207,16 @@ func ParseStatement(input string) (interface{}, error) { return stmts[0], nil } -// ParseRef returns exactly one reference. -func ParseRef(input string) (Ref, error) { - term, err := ParseTerm(input) +// ParseStatements returns a slice of parsed statements. +// This is the default return value from the parser. +func ParseStatements(filename, input string) ([]interface{}, error) { + parsed, err := Parse(filename, []byte(input)) if err != nil { - return nil, errors.Wrap(err, "failed to parse ref") + return nil, err } - ref, ok := term.Value.(Ref) - if !ok { - return nil, fmt.Errorf("expected ref but got %v", term) - } - return ref, nil + stmts := parsed.([]interface{}) + postProcess(stmts) + return stmts, err } func parseModule(stmts []interface{}) (*Module, error) { diff --git a/ast/parser_test.go b/ast/parser_test.go index 6cf3aaf9cc..95fa2e5ca9 100644 --- a/ast/parser_test.go +++ b/ast/parser_test.go @@ -310,7 +310,7 @@ func TestRule(t *testing.T) { } func TestEmptyModule(t *testing.T) { - r, err := ParseModule(" ") + r, err := ParseModule("", " ") if err != nil { t.Errorf("Expected nil for empty module: %s", err) return @@ -491,7 +491,7 @@ func TestWildcards(t *testing.T) { } func assertParse(t *testing.T, msg string, input string, correct func([]interface{})) { - p, err := ParseStatements(input) + p, err := ParseStatements("", input) if err != nil { t.Errorf("Error on test %s: parse error on %s: %s", msg, input, err) return @@ -519,7 +519,7 @@ func assertParseImport(t *testing.T, msg string, input string, correct *Import) func assertParseModule(t *testing.T, msg string, input string, correct *Module) { - m, err := ParseModule(input) + m, err := ParseModule("", input) if err != nil { t.Errorf("Error on test %s: parse error on %s: %s", msg, input, err) return diff --git a/repl/repl.go b/repl/repl.go index 95bbb6f68c..3e48980a58 100644 --- a/repl/repl.go +++ b/repl/repl.go @@ -281,7 +281,7 @@ func (r *REPL) evalBufferOne() bool { // The user may enter lines with comments on the end or // multiple lines with comments interspersed. In these cases // the parser will return multiple statements. - stmts, err := ast.ParseStatements(line) + stmts, err := ast.ParseStatements("", line) if err != nil { return false @@ -305,7 +305,7 @@ func (r *REPL) evalBufferMulti() bool { return false } - stmts, err := ast.ParseStatements(line) + stmts, err := ast.ParseStatements("", line) if err != nil { fmt.Fprintln(r.output, "error:", err) diff --git a/runtime/runtime.go b/runtime/runtime.go index 7ee3348fa4..256c2472c1 100644 --- a/runtime/runtime.go +++ b/runtime/runtime.go @@ -181,7 +181,7 @@ func parseInputs(paths []string) (*parsedInput, error) { return nil, err } - m, astErr := ast.ParseModuleFile(file) + m, astErr := ast.ParseModule(file, string(bs)) if astErr == nil { parsedModules[file] = &parsedModule{ diff --git a/runtime/server.go b/runtime/server.go index c52debca4f..6ca6b176a6 100644 --- a/runtime/server.go +++ b/runtime/server.go @@ -377,7 +377,7 @@ func (s *Server) v1PoliciesPut(w http.ResponseWriter, r *http.Request) { return } - mod, err := ast.ParseModule(string(buf)) + mod, err := ast.ParseModule(id, string(buf)) if err != nil { handleError(w, 400, err) return diff --git a/storage/policystore.go b/storage/policystore.go index e4e6eb7d99..59ef9754ac 100644 --- a/storage/policystore.go +++ b/storage/policystore.go @@ -31,7 +31,7 @@ func LoadPolicies(bufs map[string][]byte) (map[string]*ast.Module, error) { parsed := map[string]*ast.Module{} for id, bs := range bufs { - mod, err := ast.ParseModule(string(bs)) + mod, err := ast.ParseModule(id, string(bs)) if err != nil { return nil, err } From 99928b536aac09bb4bad650d7f97fdae95c87d5d Mon Sep 17 00:00:00 2001 From: Torin Sandall Date: Mon, 27 Jun 2016 14:36:12 -0700 Subject: [PATCH 2/4] Set filename on Location objects --- ast/parser_ext.go | 26 ++++++++++++++++++++++++-- ast/parser_test.go | 18 ++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/ast/parser_ext.go b/ast/parser_ext.go index d1cc892ed6..bda9a4b30b 100644 --- a/ast/parser_ext.go +++ b/ast/parser_ext.go @@ -215,7 +215,7 @@ func ParseStatements(filename, input string) ([]interface{}, error) { return nil, err } stmts := parsed.([]interface{}) - postProcess(stmts) + postProcess(filename, stmts) return stmts, err } @@ -252,7 +252,8 @@ func parseModule(stmts []interface{}) (*Module, error) { return mod, nil } -func postProcess(stmts []interface{}) { +func postProcess(filename string, stmts []interface{}) { + setFilename(filename, stmts) mangleWildcards(stmts) } @@ -302,3 +303,24 @@ func (vis *wildcardMangler) mangleSlice(xs []*Term) { vis.mangle(x) } } + +func setFilename(filename string, stmts []interface{}) { + for _, stmt := range stmts { + vis := &GenericVisitor{func(x interface{}) bool { + switch x := x.(type) { + case *Package: + x.Location.File = filename + case *Import: + x.Location.File = filename + case *Rule: + x.Location.File = filename + case *Expr: + x.Location.File = filename + case *Term: + x.Location.File = filename + } + return false + }} + Walk(vis, stmt) + } +} diff --git a/ast/parser_test.go b/ast/parser_test.go index 95fa2e5ca9..b7b079e45a 100644 --- a/ast/parser_test.go +++ b/ast/parser_test.go @@ -386,6 +386,24 @@ func TestExample(t *testing.T) { }) } +func TestLocation(t *testing.T) { + mod, err := ParseModule("test", testModule) + if err != nil { + t.Errorf("Unexpected error while parsing test module: %v", err) + return + } + expr := mod.Rules[0].Body[0] + if expr.Location.Col != 5 { + t.Errorf("Expected column of %v to be 5 but got: %v", expr, expr.Location.Col) + } + if expr.Location.Row != 9 { + t.Errorf("Expected row of %v to be 9 but got: %v", expr, expr.Location.Row) + } + if expr.Location.File != "test" { + t.Errorf("Expected file of %v to be test but got: %v", expr, expr.Location.File) + } +} + func TestConstantRules(t *testing.T) { testModule := ` package a.b.c From 0aa873ebb40f8aa69175596373d6293d6d1c0ad8 Mon Sep 17 00:00:00 2001 From: Torin Sandall Date: Mon, 27 Jun 2016 14:41:53 -0700 Subject: [PATCH 3/4] Reject non-string import path values --- ast/compile.go | 12 ++---------- ast/parser_test.go | 1 + ast/rego.peg | 6 ++++-- 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/ast/compile.go b/ast/compile.go index 74aadb05d9..21b1b38733 100644 --- a/ast/compile.go +++ b/ast/compile.go @@ -393,22 +393,14 @@ func (c *Compiler) setGlobals() { globals[i.Alias] = p case Var: globals[i.Alias] = p - default: - c.err("unexpected %T: %v", p, i) } } else { switch p := i.Path.Value.(type) { case Ref: - switch v := p[len(p)-1].Value.(type) { - case String: - globals[Var(v)] = p - default: - c.err("unexpected %T: %v", v, i) - } + v := p[len(p)-1].Value.(String) + globals[Var(v)] = p case Var: globals[p] = p - default: - c.err("unexpected %T: %v", i.Path, i.Path) } } } diff --git a/ast/parser_test.go b/ast/parser_test.go index b7b079e45a..57de3a55e4 100644 --- a/ast/parser_test.go +++ b/ast/parser_test.go @@ -256,6 +256,7 @@ func TestImport(t *testing.T) { ref2 := RefTerm(VarTerm("foo"), StringTerm("bar"), StringTerm("white space")) assertParseImport(t, "white space", "import foo.bar[\"white space\"]", &Import{Path: ref2}) assertParseError(t, "non-ground ref", "import foo[x]") + assertParseError(t, "non-string", "import foo[0]") } func TestRule(t *testing.T) { diff --git a/ast/rego.peg b/ast/rego.peg index bd33d8ba0a..0bf0b6854e 100644 --- a/ast/rego.peg +++ b/ast/rego.peg @@ -77,8 +77,10 @@ Import <- "import" ws path:(Ref / Var) alias:(ws "as" ws Var)? { imp.Path = path.(*Term) switch p := imp.Path.Value.(type) { case Ref: - if !p.IsGround() { - return nil, fmt.Errorf("import cannot contain variables in tail: %v", p) + for _, x := range p[1:] { + if _, ok := x.Value.(String); !ok { + return nil, fmt.Errorf("import path cannot contain non-string values: %v", x) + } } } if alias == nil { From 30aa306d496c51002dcaa1c0f659a21a5885ef72 Mon Sep 17 00:00:00 2001 From: Torin Sandall Date: Tue, 28 Jun 2016 14:14:05 -0700 Subject: [PATCH 4/4] Error message improvements These changes add some helpers to obtain contextual information for error mesasages. Specifically, the Location values on all AST nodes can be used to format error messages that include line, column, and filename. Also, add compiler step to check that built-ins are provided with the correct number of arguments. The built-in implementations in topdown assume this is the case and will index into the term slice without checking. --- ast/compile.go | 39 +++++++--- ast/compile_test.go | 162 ++++++++++++++++++++++++++---------------- ast/parser_ext.go | 5 +- ast/policy.go | 26 +++++++ ast/term.go | 23 ++++++ repl/repl.go | 5 +- repl/repl_test.go | 13 ++-- topdown/arithmetic.go | 7 +- 8 files changed, 194 insertions(+), 86 deletions(-) diff --git a/ast/compile.go b/ast/compile.go index 21b1b38733..7667bca216 100644 --- a/ast/compile.go +++ b/ast/compile.go @@ -105,6 +105,7 @@ func NewCompiler() *Compiler { stage{c.setModuleTree, "setModuleTree"}, stage{c.checkSafetyHead, "checkSafetyHead"}, stage{c.checkSafetyBody, "checkSafetyBody"}, + stage{c.checkBuiltinArgs, "checkBuiltinArgs"}, stage{c.resolveAllRefs, "resolveAllRefs"}, stage{c.setRuleGraph, "setRuleGraph"}, stage{c.checkRecursion, "checkRecursion"}, @@ -160,6 +161,26 @@ func (c *Compiler) FlattenErrors() string { return fmt.Sprintf("%d errors occurred:\n%s", len(c.Errors), strings.Join(b, "\n")) } +// checkBuiltinArgs ensures that all built-ins are called with the correct number +// of arguments. +// +// TODO(tsandall): in the future this should be replaced with schema checking. +func (c *Compiler) checkBuiltinArgs() { + for _, m := range c.Modules { + for _, r := range m.Rules { + for _, expr := range r.Body { + if ts, ok := expr.Terms.([]*Term); ok { + if bi, ok := BuiltinMap[ts[0].Value.(Var)]; ok { + if bi.NumArgs != len(ts[1:]) { + c.err(expr.Location.Errorf("%v: wrong number of arguments (expression %s must specify %d arguments to built-in function %v)", r.Name, expr.Location.Text, bi.NumArgs, ts[0])) + } + } + } + } + } + } +} + // checkRecursion ensures that there are no recursive rule definitions, i.e., there are // no cycles in the RuleGraph. func (c *Compiler) checkRecursion() { @@ -173,7 +194,7 @@ func (c *Compiler) checkRecursion() { for _, x := range p { n = append(n, string(x.(*Rule).Name)) } - c.err("recursion found in %v: %v", r.Name, strings.Join(n, ", ")) + c.err(r.Location.Errorf("%v: recursive reference: %v (recursion is not allowed)", r.Name, strings.Join(n, " -> "))) } } } @@ -190,7 +211,9 @@ func (c *Compiler) checkSafetyBody() { for _, r := range m.Rules { reordered, unsafe := reorderBodyForSafety(globals, r.Body) if len(unsafe) != 0 { - c.err("unsafe variables in %v: %v", r.Name, unsafe.Vars()) + for v := range unsafe.Vars() { + c.err(r.Location.Errorf("%v: %v is unsafe (variable %v must appear in the output position of at least one non-negated expression)", r.Name, v, v)) + } } else { r.Body = reordered } @@ -203,19 +226,15 @@ func (c *Compiler) checkSafetyBody() { func (c *Compiler) checkSafetyHead() { for _, m := range c.Modules { for _, r := range m.Rules { - headVars := r.HeadVars() - bodyVars := r.Body.Vars(true) - for headVar := range headVars { - if _, ok := bodyVars[headVar]; !ok { - c.err("unsafe variable from head of %v: %v", r.Name, headVar) - } + unsafe := r.HeadVars().Diff(r.Body.Vars(true)) + for v := range unsafe { + c.err(r.Location.Errorf("%v: %v is unsafe (variable %v must appear in at least one expression within the body of %v)", r.Name, v, v, r.Name)) } } } } -func (c *Compiler) err(f string, a ...interface{}) { - err := fmt.Errorf(f, a...) +func (c *Compiler) err(err error) { c.Errors = append(c.Errors, err) } diff --git a/ast/compile_test.go b/ast/compile_test.go index a82e522bc6..91d92d25cc 100644 --- a/ast/compile_test.go +++ b/ast/compile_test.go @@ -309,40 +309,76 @@ func TestCompilerCheckSafetyBodyErrors(t *testing.T) { `)} compileStages(c, "", "checkSafetyBody") - expected := []error{ - fmt.Errorf("unsafe variables in badBuiltin: [deadbeef]"), - fmt.Errorf("unsafe variables in unboundRef1: [a]"), - fmt.Errorf("unsafe variables in unboundRef2: [a]"), - fmt.Errorf("unsafe variables in unboundNegated1: [i x]"), - fmt.Errorf("unsafe variables in unboundNegated2: [i x]"), - fmt.Errorf("unsafe variables in unboundNegated3: [i j x]"), - fmt.Errorf("unsafe variables in unboundNegated4: [i j]"), - fmt.Errorf("unsafe variables in unsafeBuiltin: [x]"), - fmt.Errorf("unsafe variables in unboundNoTarget: [x]"), - fmt.Errorf("unsafe variables in unboundArrayComprBody1: [y]"), - fmt.Errorf("unsafe variables in unboundArrayComprBody2: [z]"), - fmt.Errorf("unsafe variables in unboundArrayComprBody3: [x]"), - fmt.Errorf("unsafe variables in unboundArrayComprTerm1: [u]"), - fmt.Errorf("unsafe variables in unboundArrayComprTerm2: [w]"), - fmt.Errorf("unsafe variables in unboundArrayComprTerm3: [i]"), - fmt.Errorf("unsafe variables in unboundArrayComprMixed1: [x z]"), - fmt.Errorf("unsafe variables in unsafeClosure1: [x]"), - fmt.Errorf("unsafe variables in unsafeClosure2: [y]"), - fmt.Errorf("unsafe variables in unsafeNestedHead: [dead]"), + makeErrMsg := func(rule string, varName string) string { + return fmt.Sprintf("%v: %v is unsafe (variable %v must appear in the output position of at least one non-negated expression)", rule, varName, varName) } - if !reflect.DeepEqual(expected, c.Errors) { - e := []string{} - for _, x := range expected { - e = append(e, x.Error()) - } - r := []string{} - for _, x := range c.Errors { - r = append(r, x.Error()) - } - t.Errorf("Expected:\n%v\nBut got:\n%v", strings.Join(e, "\n"), strings.Join(r, "\n")) + expected := []string{ + makeErrMsg("badBuiltin", "deadbeef"), + makeErrMsg("unboundRef1", "a"), + makeErrMsg("unboundRef2", "a"), + makeErrMsg("unboundNegated1", "i"), + makeErrMsg("unboundNegated1", "x"), + makeErrMsg("unboundNegated2", "i"), + makeErrMsg("unboundNegated2", "x"), + makeErrMsg("unboundNegated3", "i"), + makeErrMsg("unboundNegated3", "j"), + makeErrMsg("unboundNegated3", "x"), + makeErrMsg("unboundNegated4", "i"), + makeErrMsg("unboundNegated4", "j"), + makeErrMsg("unsafeBuiltin", "x"), + makeErrMsg("unboundNoTarget", "x"), + makeErrMsg("unboundArrayComprBody1", "y"), + makeErrMsg("unboundArrayComprBody2", "z"), + makeErrMsg("unboundArrayComprBody3", "x"), + makeErrMsg("unboundArrayComprTerm1", "u"), + makeErrMsg("unboundArrayComprTerm2", "w"), + makeErrMsg("unboundArrayComprTerm3", "i"), + makeErrMsg("unboundArrayComprMixed1", "x"), + makeErrMsg("unboundArrayComprMixed1", "z"), + makeErrMsg("unsafeClosure1", "x"), + makeErrMsg("unsafeClosure2", "y"), + makeErrMsg("unsafeNestedHead", "dead"), } + result := compilerErrsToStringSlice(c.Errors) + sort.Strings(expected) + + if len(result) != len(expected) { + t.Fatalf("Expected %d:\n%v\nBut got %d:\n%v", len(expected), strings.Join(expected, "\n"), len(result), strings.Join(result, "\n")) + } + + for i := range result { + if expected[i] != result[i] { + t.Errorf("Expected %v but got: %v", expected[i], result[i]) + } + } + +} + +func TestCompilerBuiltinArgs(t *testing.T) { + c := NewCompiler() + c.Modules = map[string]*Module{ + "mod": MustParseModule(` + package badbuiltin + p :- count(1) + q :- count([1,2,3], x, 1) + `), + } + compileStages(c, "", "checkBuiltinArgs") + result := compilerErrsToStringSlice(c.Errors) + expected := []string{ + "p: wrong number of arguments (expression count(1) must specify 2 arguments to built-in function count)", + "q: wrong number of arguments (expression count([1,2,3], x, 1) must specify 2 arguments to built-in function count)", + } + if len(result) != len(expected) { + t.Fatalf("Expected %d:\n%v\nBut got %d:\n%v", len(expected), strings.Join(expected, "\n"), len(result), strings.Join(result, "\n")) + } + for i := range result { + if expected[i] != result[i] { + t.Errorf("Expected %v but got: %v", expected[i], result[i]) + } + } } func TestCompilerResolveAllRefs(t *testing.T) { @@ -487,42 +523,36 @@ func TestCompilerCheckRecursion(t *testing.T) { compileStages(c, "", "checkRecursion") - expected := []error{ - fmt.Errorf("recursion found in s: s, t, s"), - fmt.Errorf("recursion found in t: t, s, t"), - fmt.Errorf("recursion found in a: a, b, c, e, a"), - fmt.Errorf("recursion found in b: b, c, e, a, b"), - fmt.Errorf("recursion found in c: c, e, a, b, c"), - fmt.Errorf("recursion found in e: e, a, b, c, e"), - fmt.Errorf("recursion found in p: p, q, p"), - fmt.Errorf("recursion found in q: q, p, q"), - fmt.Errorf("recursion found in acq: acq, acp, acq"), - fmt.Errorf("recursion found in acp: acp, acq, acp"), - fmt.Errorf("recursion found in np: np, nq, np"), - fmt.Errorf("recursion found in nq: nq, np, nq"), + makeErrMsg := func(rule string, loop ...string) string { + return fmt.Sprintf("%v: recursive reference: %s (recursion is not allowed)", rule, strings.Join(loop, " -> ")) } - if len(c.Errors) != len(expected) { - t.Errorf("Expected exactly %v errors but got %v: %v", len(expected), len(c.Errors), c.Errors) - return + expected := []string{ + makeErrMsg("s", "s", "t", "s"), + makeErrMsg("t", "t", "s", "t"), + makeErrMsg("a", "a", "b", "c", "e", "a"), + makeErrMsg("b", "b", "c", "e", "a", "b"), + makeErrMsg("c", "c", "e", "a", "b", "c"), + makeErrMsg("e", "e", "a", "b", "c", "e"), + makeErrMsg("p", "p", "q", "p"), + makeErrMsg("q", "q", "p", "q"), + makeErrMsg("acq", "acq", "acp", "acq"), + makeErrMsg("acp", "acp", "acq", "acp"), + makeErrMsg("np", "np", "nq", "np"), + makeErrMsg("nq", "nq", "np", "nq"), } - for _, x := range c.Errors { - found := false - for i, y := range expected { - if reflect.DeepEqual(x, y) { - found = true - expected = append(expected[:i], expected[i+1:]...) - break - } + result := compilerErrsToStringSlice(c.Errors) + sort.Strings(expected) + + if len(result) != len(expected) { + t.Fatalf("Expected %d:\n%v\nBut got %d:\n%v", len(expected), strings.Join(expected, "\n"), len(result), strings.Join(result, "\n")) + } + + for i := range result { + if result[i] != expected[i] { + t.Errorf("Expected %v but got: %v", expected[i], result[i]) } - if !found { - t.Errorf("Unexpected error in recursion check: %v", x) - } - } - - if len(expected) > 0 { - t.Errorf("Missing errors in recursion check: %v", expected) } } @@ -715,3 +745,13 @@ func getCompilerTestModules() map[string]*Module { "mod6": mod6, } } + +func compilerErrsToStringSlice(errors []error) []string { + result := []string{} + for _, e := range errors { + msg := strings.SplitN(e.Error(), ":", 3)[2] + result = append(result, strings.TrimSpace(msg)) + } + sort.Strings(result) + return result +} diff --git a/ast/parser_ext.go b/ast/parser_ext.go index bda9a4b30b..2c3e5516f6 100644 --- a/ast/parser_ext.go +++ b/ast/parser_ext.go @@ -227,7 +227,8 @@ func parseModule(stmts []interface{}) (*Module, error) { _package, ok := stmts[0].(*Package) if !ok { - return nil, fmt.Errorf("first statement must be package") + loc := stmts[0].(Statement).Loc() + return nil, loc.Errorf("expected package directive (%s must come after package directive)", stmts[0]) } mod := &Module{ @@ -243,7 +244,7 @@ func parseModule(stmts []interface{}) (*Module, error) { case Body: rule := ParseConstantRule(stmt) if rule == nil { - return nil, fmt.Errorf("body must be contained inside rule: %v", stmt) + return nil, stmt[0].Location.Errorf("expected rule (%s must be declared inside a rule)", stmt[0].Location.Text) } mod.Rules = append(mod.Rules, rule) } diff --git a/ast/policy.go b/ast/policy.go index 9f199886a9..2601ad20bf 100644 --- a/ast/policy.go +++ b/ast/policy.go @@ -32,6 +32,7 @@ var Wildcard = &Term{Value: Var("_")} var WildcardPrefix = "$" type ( + // Module represents a collection of policies (defined by rules) // within a namespace (defined by the package) and optional // dependencies on external documents (defined by imports). @@ -41,6 +42,11 @@ type ( Rules []*Rule } + // Statement represents a single statement within a module. + Statement interface { + Loc() *Location + } + // Package represents the namespace of the documents produced // by rules inside the module. Package struct { @@ -108,6 +114,11 @@ func (pkg *Package) Equal(other *Package) bool { return pkg.Path.Equal(other.Path) } +// Loc returns the location of the Package in the definition. +func (pkg *Package) Loc() *Location { + return pkg.Location +} + func (pkg *Package) String() string { return fmt.Sprintf("package %v", pkg.Path) } @@ -117,6 +128,11 @@ func (imp *Import) Equal(other *Import) bool { return imp.Alias.Equal(other.Alias) && imp.Path.Equal(other.Path) } +// Loc returns the location of the Import in the definition. +func (imp *Import) Loc() *Location { + return imp.Location +} + func (imp *Import) String() string { buf := []string{"import", imp.Path.String()} if len(imp.Alias) > 0 { @@ -177,6 +193,11 @@ func (rule *Rule) HeadVars() VarSet { return vis.vars } +// Loc returns the location of the Rule in the definition. +func (rule *Rule) Loc() *Location { + return rule.Location +} + func (rule *Rule) String() string { var buf []string if rule.Key != nil { @@ -238,6 +259,11 @@ func (body Body) IsGround() bool { return true } +// Loc returns the location of the Body in the definition. +func (body Body) Loc() *Location { + return body[0].Location +} + // OutputVars returns a VarSet containing the variables that would be bound by evaluating // the body. func (body Body) OutputVars(safe VarSet) VarSet { diff --git a/ast/term.go b/ast/term.go index 78379495af..fc938055ba 100644 --- a/ast/term.go +++ b/ast/term.go @@ -11,6 +11,8 @@ import ( "regexp" "strconv" "strings" + + "github.com/pkg/errors" ) // Location records a position in source code @@ -26,6 +28,27 @@ func NewLocation(text []byte, file string, row int, col int) *Location { return &Location{Text: text, File: file, Row: row, Col: col} } +// Errorf returns a new error value with a message formatted to include the location +// info (e.g., line, column, filename, etc.) +func (loc *Location) Errorf(f string, a ...interface{}) error { + return fmt.Errorf(loc.format(f), a...) +} + +// Wrapf returns a new error value that wraps an existing error with a message formatted +// to include the location info (e.g., line, column, filename, etc.) +func (loc *Location) Wrapf(err error, f string, a ...interface{}) error { + return errors.Wrapf(err, loc.format(f), a...) +} + +func (loc *Location) format(f string) string { + if len(loc.File) > 0 { + f = fmt.Sprintf("%v:%v: %v", loc.File, loc.Row, f) + } else { + f = fmt.Sprintf("%v:%v: %v", loc.Row, loc.Col, f) + } + return f +} + // Value declares the common interface for all Term values. Every kind of Term value // in the language is represented as a type that implements this interface: // diff --git a/repl/repl.go b/repl/repl.go index 3e48980a58..9a76761503 100644 --- a/repl/repl.go +++ b/repl/repl.go @@ -235,8 +235,9 @@ func (r *REPL) compileBody(body ast.Body) (ast.Body, error) { r.nextID++ rule := &ast.Rule{ - Name: ast.Var(name), - Body: body, + Location: body[0].Location, + Name: ast.Var(name), + Body: body, } modules := r.policyStore.List() diff --git a/repl/repl_test.go b/repl/repl_test.go index 6b01f9baf6..d8cb52ad35 100644 --- a/repl/repl_test.go +++ b/repl/repl_test.go @@ -39,7 +39,7 @@ func TestUnset(t *testing.T) { repl.OneShot("unset p") repl.OneShot("p") result := buffer.String() - if result != "error: 1 error occurred: unsafe variables in repl2: [p]\n" { + if result != "error: 1 error occurred: 1:1: repl2: p is unsafe (variable p must appear in the output position of at least one non-negated expression)\n" { t.Errorf("Expected p to be unsafe but got: %v", result) return } @@ -50,7 +50,7 @@ func TestUnset(t *testing.T) { repl.OneShot("unset p") repl.OneShot("p") result = buffer.String() - if result != "error: 1 error occurred: unsafe variables in repl4: [p]\n" { + if result != "error: 1 error occurred: 1:1: repl4: p is unsafe (variable p must appear in the output position of at least one non-negated expression)\n" { t.Errorf("Expected p to be unsafe but got: %v", result) return } @@ -344,7 +344,7 @@ func TestEvalRuleCompileError(t *testing.T) { repl := newRepl(store, &buffer) repl.OneShot("p[x] :- true") result := buffer.String() - expected := "error: 1 error occurred: unsafe variable from head of p: x\n" + expected := "error: 1 error occurred: 1:1: p: x is unsafe (variable x must appear in at least one expression within the body of p)\n" if result != expected { t.Errorf("Expected error message in output but got: %v", result) return @@ -354,7 +354,6 @@ func TestEvalRuleCompileError(t *testing.T) { result = buffer.String() if result != "" { t.Errorf("Expected valid rule to compile (because state should have been rolled back) but got: %v", result) - return } } @@ -365,9 +364,9 @@ func TestEvalBodyCompileError(t *testing.T) { repl.outputFormat = "json" repl.OneShot("x = 1, y > x") result1 := buffer.String() - expected1 := "error: 1 error occurred: unsafe variables in repl0: [y]\n" + expected1 := "error: 1 error occurred: 1:1: repl0: y is unsafe (variable y must appear in the output position of at least one non-negated expression)\n" if result1 != expected1 { - t.Errorf("Expected error message in output but got : %v", result1) + t.Errorf("Expected error message in output but got`: %v", result1) return } buffer.Reset() @@ -441,7 +440,7 @@ func TestEvalPackage(t *testing.T) { repl.OneShot("package baz.qux") buffer.Reset() repl.OneShot("p") - if buffer.String() != "error: 1 error occurred: unsafe variables in repl0: [p]\n" { + if buffer.String() != "error: 1 error occurred: 1:1: repl0: p is unsafe (variable p must appear in the output position of at least one non-negated expression)\n" { t.Errorf("Expected unsafe variable error but got: %v", buffer.String()) return } diff --git a/topdown/arithmetic.go b/topdown/arithmetic.go index 45d4b343ba..0609a7e687 100644 --- a/topdown/arithmetic.go +++ b/topdown/arithmetic.go @@ -9,7 +9,6 @@ import ( "math" "github.com/open-policy-agent/opa/ast" - "github.com/pkg/errors" ) type arithArity1 func(a float64) (ast.Number, error) @@ -48,7 +47,7 @@ func evalArithArity1(f arithArity1) BuiltinFunc { ops := expr.Terms.([]*ast.Term) a, err := ValueToFloat64(ops[1].Value, ctx) if err != nil { - return errors.Wrapf(err, "arithmetic") + return expr.Location.Wrapf(err, "expected number (operand %s is not a number)", ops[0].Location.Text) } r, err := f(a) @@ -77,12 +76,12 @@ func evalArithArity2(f arithArity2) BuiltinFunc { a, err := ValueToFloat64(ops[1].Value, ctx) if err != nil { - return errors.Wrapf(err, "arithemtic") + return expr.Location.Wrapf(err, "expected number (first operand %s is not a number)", ops[0].Location.Text) } b, err := ValueToFloat64(ops[2].Value, ctx) if err != nil { - return errors.Wrapf(err, "arithemtic") + return expr.Location.Wrapf(err, "expected number (second operand %s is not a number)", ops[2].Location.Text) } c, err := f(a, b)