ast: Fix ordering of rule type checking errors

Previously the type checker would not sort errors before returning
them. As a result, the ordering of errors would depend on the order of
rules passed to the type checker (which is non-deterministic due to Go
map iteration order.)

These changes update the type checker to sort errors by location
before before returning them. This way the errors reported by opa test
and opa check and everything else are consistent across runs.

Fixes open-policy-agent#1620

Signed-off-by: Torin Sandall <torinsandall@gmail.com>

ast/check.go

@@ -92,6 +92,7 @@ func (tc *typeChecker) CheckTypes(env *TypeEnv, sorted []util.T) (*TypeEnv, Erro
 	for _, s := range sorted {
 		tc.checkRule(env, s.(*Rule))
 	}
+	tc.errs.Sort()
 	return env, tc.errs
 }
 

ast/check_test.go

@@ -1029,6 +1029,37 @@ func TestCheckErrorDetails(t *testing.T) {
 	}
 }
 
+func TestCheckErrorOrdering(t *testing.T) {
+
+	mod := MustParseModule(`
+		package test
+
+		q = true
+
+		p { data.test.q = 1 }  # type error: bool = number
+		p { data.test.q = 2 }  # type error: bool = number
+	`)
+
+	input := make([]util.T, len(mod.Rules))
+	inputReversed := make([]util.T, len(mod.Rules))
+
+	for i := range input {
+		input[i] = mod.Rules[i]
+		inputReversed[i] = mod.Rules[i]
+	}
+
+	tmp := inputReversed[1]
+	inputReversed[1] = inputReversed[2]
+	inputReversed[2] = tmp
+
+	_, errs1 := newTypeChecker().CheckTypes(nil, input)
+	_, errs2 := newTypeChecker().CheckTypes(nil, inputReversed)
+
+	if errs1.Error() != errs2.Error() {
+		t.Fatalf("Expected error slices to be equal. errs1:\n\n%v\n\nerrs2:\n\n%v\n\n", errs1, errs2)
+	}
+}
+
 func newTestEnv(rs []string) *TypeEnv {
 	module := MustParseModule(`
 		package test

ast/errors.go

@@ -6,6 +6,7 @@ package ast
 
 import (
 	"fmt"
+	"sort"
 	"strings"
 )
 
@@ -31,6 +32,21 @@ func (e Errors) Error() string {
 	return fmt.Sprintf("%d errors occurred:\n%s", len(e), strings.Join(s, "\n"))
 }
 
+// Sort sorts the error slice by location. If the locations are equal then the
+// error message is compared.
+func (e Errors) Sort() {
+	sort.Slice(e, func(i, j int) bool {
+		a := e[i]
+		b := e[j]
+
+		if cmp := a.Location.Compare(b.Location); cmp != 0 {
+			return cmp < 0
+		}
+
+		return a.Error() < b.Error()
+	})
+}
+
 const (
 	// ParseErr indicates an unclassified parse error occurred.
 	ParseErr = "rego_parse_error"