diff --git a/ast/compile.go b/ast/compile.go index 74aadb05d9..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) } @@ -393,22 +412,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/compile_test.go b/ast/compile_test.go index 4e9b8c5c31..91d92d25cc 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) } @@ -312,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) { @@ -490,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) } } @@ -718,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 f050fc887e..2c3e5516f6 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,50 +162,6 @@ func ParseTerm(input string) (*Term, error) { return term, 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) - if err != nil { - return nil, err - } - if len(stmts) != 1 { - return nil, fmt.Errorf("expected exactly one statement (rule)") - } - rule, ok := stmts[0].(*Rule) - if !ok { - return nil, fmt.Errorf("expected rule but got %T", stmts[0]) - } - 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) - if err != nil { - return nil, err - } - if len(stmts) != 1 { - return nil, fmt.Errorf("expected exactly one statement") - } - return stmts[0], nil -} - // ParseRef returns exactly one reference. func ParseRef(input string) (Ref, error) { term, err := ParseTerm(input) @@ -239,6 +175,50 @@ func ParseRef(input string) (Ref, error) { 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) + if err != nil { + return nil, err + } + if len(stmts) != 1 { + return nil, fmt.Errorf("expected exactly one statement (rule)") + } + rule, ok := stmts[0].(*Rule) + if !ok { + return nil, fmt.Errorf("expected rule but got %T", stmts[0]) + } + return rule, nil +} + +// 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) + if err != nil { + return nil, err + } + if len(stmts) != 1 { + return nil, fmt.Errorf("expected exactly one statement") + } + return stmts[0], nil +} + +// 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, err + } + stmts := parsed.([]interface{}) + postProcess(filename, stmts) + return stmts, err +} + func parseModule(stmts []interface{}) (*Module, error) { if len(stmts) == 0 { @@ -247,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{ @@ -263,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) } @@ -272,7 +253,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) } @@ -322,3 +304,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 6cf3aaf9cc..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) { @@ -310,7 +311,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 @@ -386,6 +387,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 @@ -491,7 +510,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 +538,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/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/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 { 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 95bbb6f68c..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() @@ -281,7 +282,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 +306,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/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/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 } 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)