Fix reference evaluation bug

References were not being evaluated correctly. The top-level evalRefRec
function was computing the ground prefix from the reference's binding
and then performing the rest of the evaluation with the original
reference and the prefix. Instead, the rest of the evaluation should
proceed with the plugged value and the prefix. This fix was not
compatible with how reference bindings were stored. Before, reference
bindings were keyed with the original (potentially non-ground)
reference. Now, they are keyed with the evaluated version of the
reference. This version is guaranteed to be ground. As a result, the
PlugValue handling of references must be updated to handle cases such
as:

orig ref:   p[x][y]
locals:     {x: 1, y: 2}
refs:       {p[1][2]: "foo"}

The solution is to recursively plug the ref. E.g., p[x][y] => p[1][y] =>
p[1][2] => "foo".

Fixes #238

topdown/topdown.go

@@ -451,7 +451,9 @@ func PlugExpr(expr *ast.Expr, binding Binding) *ast.Expr {
 func PlugTerm(term *ast.Term, binding Binding) *ast.Term {
 	switch v := term.Value.(type) {
 	case ast.Var:
-		return &ast.Term{Value: PlugValue(v, binding)}
+		plugged := *term
+		plugged.Value = PlugValue(v, binding)
+		return &plugged
 
 	case ast.Ref:
 		plugged := *term
@@ -505,13 +507,16 @@ func PlugValue(v ast.Value, binding func(ast.Value) ast.Value) ast.Value {
 
 	case ast.Ref:
 		if b := binding(v); b != nil {
-			return b
+			return PlugValue(b, binding)
 		}
 		buf := make(ast.Ref, len(v))
 		buf[0] = v[0]
 		for i, p := range v[1:] {
 			buf[i+1] = PlugTerm(p, binding)
 		}
+		if b := binding(buf); b != nil {
+			return PlugValue(b, binding)
+		}
 		return buf
 
 	case ast.Array:
@@ -1000,6 +1005,10 @@ func evalRefRec(t *Topdown, ref ast.Ref, iter Iterator) error {
 
 	switch v := PlugValue(ref, t.Binding).(type) {
 	case ast.Ref:
+		// https://github.com/open-policy-agent/opa/issues/238
+		// We must set ref to the plugged value here otherwise the ref
+		// evaluation doesn't have consistent values for prefix and ref.
+		ref = v
 		prefix = v.GroundPrefix()
 	default:
 		// Fast-path? TODO test case.
@@ -1650,7 +1659,10 @@ func evalRefRuleResult(t *Topdown, ref ast.Ref, suffix ast.Ref, result ast.Value
 	}
 
 	return evalRefRuleResultRec(t, result, s, ast.Ref{}, func(t *Topdown, v ast.Value) error {
-		return Continue(t, ref, v, iter)
+		// Must add binding with plugged value of ref in case ref contains
+		// suffix with one or more vars.
+		// Test case: "input: object dereference ground 2" exercises this.
+		return Continue(t, PlugValue(ref, t.Binding), v, iter)
 	})
 }
 

topdown/topdown_test.go

@@ -481,7 +481,7 @@ func TestTopDownVirtualDocs(t *testing.T) {
 		{"input: set embedded", []string{`p[x] :- x = {"b": [q[2]]}`, `q[x] :- a[i] = x`}, `[{"b": [true]}]`},
 		{"input: set undefined", []string{"p = true :- q[1000]", "q[x] :- a[x] = y"}, ""},
 		{"input: set dereference error", []string{"p :- x = [1], q[x][0]", "q[[x]] :- a[_] = x"}, setDereferenceTypeErr(nil)},
-		{"input: set ground var", []string{"p[x] :- x = 1, q[x]", "q[y] :- a = [1,2,3,4], a[y] = i"}, "[1]"},
+		{"input: set ground var", []string{"p[x] :- x = 1, q[x]", "q[y] :- a[y] = i"}, "[1]"},
 		{"input: set ground composite (1)", []string{
 			"p :- z = [[1,2], 2], q[z]",
 			"q[[x,y]] :- x = [1,y], y = 2",
@@ -613,6 +613,10 @@ func TestTopDownVirtualDocs(t *testing.T) {
 			`p[x] :- q = x`,
 			`q[k] = {"v": v} :- v = [i,j], k = i, i = "a", j = 1`},
 			`[{"a": {"v": ["a", 1]}}]`},
+		// data.c[0].z.p is longer than data.q
+		{"no suffix: bound ref with long prefix (#238)", []string{
+			"p :- q, q",
+			"q = x :- x = data.c[0].z.p"}, "true"},
 		{"no suffix: object conflict (error)", []string{
 			`p[x] = y :- xs = ["a","b","c","a"], x = xs[i], y = a[i]`},
 			objectDocKeyConflictErr(nil)},
@@ -1859,7 +1863,7 @@ func runTopDownTracingTestCase(t *testing.T, module string, n int, cases map[int
 
 	_, err := Query(params)
 	if err != nil {
-		panic(err)
+		t.Fatalf("Unexpected error: %v", err)
 	}
 
 	if len(*buf) != n {