Merge pull request #57 from tsandall/error-improvements

Improve error messages

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)
 				}
 			}
 		}

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
+}

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)
+	}
+}

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

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 {

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 {

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:
 //

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)

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
 	}

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{

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

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
 		}

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)