cue: fix Allows for new evaluator

The new evaluator computes closedness quite
differently from the old one. This CL adapts the API to
use the new data.

Note that the data the new evaluator generates is
considerably more convenient. That is why the logic
is quite a bit simpler. The logic could probably be simplified
more.

Also note that HasEllipsis seems to be not computed
correclty entirely. Although it seems that many tests
improve if we add the check to isClosed or
IsClosedStruct, there are some cases where this causes
errors. This seems to be the case with one of the tests
in TestAllows. As this only manifests itself when structure
sharing is disabled, we just mark this test as todo and
leave the investigation for later.

Issue #3060
Issue #543
Issue #2884

Signed-off-by: Marcel van Lohuizen <[email protected]>
Change-Id: I98e8c25e0680674e2aaa9f743a49b39920d7b266
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1194080
Reviewed-by: Daniel Martí <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
Unity-Result: CUE porcuepine <[email protected]>

Author: mpvl

cue/matrix_test.go

@@ -69,3 +69,9 @@ func TODO_Sharing(t *testing.T, c *evalConfig) {
 		t.Skip("Skipping v3 with sharing")
 	}
 }
+
+func TODO_NoSharing(t *testing.T, c *evalConfig) {
+	if c.version == internal.DevVersion && !c.flags.Sharing {
+		t.Skip("Skipping v3 without sharing")
+	}
+}

cue/types_test.go

@@ -1430,13 +1430,13 @@ func TestFillPathError(t *testing.T) {
 }
 
 func TestAllows(t *testing.T) {
-	r := &Runtime{}
-
 	testCases := []struct {
 		desc  string
 		in    string
 		sel   Selector
 		allow bool
+
+		todo_nosharing bool
 	}{{
 		desc: "allow new field in open struct",
 		in: `
@@ -1463,6 +1463,14 @@ func TestAllows(t *testing.T) {
 		})
 		`,
 		sel: Str("b"),
+	}, {
+		desc: "allow field in pattern",
+		in: `
+				x: #X
+				#X: [>"a"]: 1
+				`,
+		sel:   Str("b"),
+		allow: true,
 	}, {
 		desc: "allow index in open list",
 		in: `
@@ -1615,6 +1623,8 @@ func TestAllows(t *testing.T) {
 		`,
 		sel:   AnyString,
 		allow: true,
+
+		todo_nosharing: true,
 	}, {
 		desc: "disallow label in disjunction",
 		in: `
@@ -1657,7 +1667,10 @@ func TestAllows(t *testing.T) {
 
 	for _, tc := range testCases {
 		runMatrix(t, tc.desc, func(t *testing.T, cfg *evalConfig) {
-			v := compileT(t, r, tc.in).Value()
+			if tc.todo_nosharing {
+				TODO_NoSharing(t, cfg)
+			}
+			v := compileT(t, cfg.runtime(), tc.in).Value()
 			v = v.LookupPath(path)
 
 			got := v.Allows(tc.sel)
@@ -2211,8 +2224,6 @@ func TestUnify(t *testing.T) {
 	}}
 	// TODO(tdtest): use cuetest.Run when supported.
 	doMatrix(t, func(t *testing.T, cfg *evalConfig) {
-		TODO_V3(t, cfg)
-
 		tdtest.Run(t, testCases, func(t *cuetest.T, tc *testCase) {
 			v := cfg.getValue(t.T, tc.value)
 			x := v.LookupPath(ParsePath(tc.pathA))
@@ -2267,8 +2278,6 @@ func TestUnifyAccept(t *testing.T) {
 	}}
 	// TODO(tdtest): use cuetest.Run when supported.
 	doMatrix(t, func(t *testing.T, cfg *evalConfig) {
-		TODO_V3(t, cfg)
-
 		tdtest.Run(t, testCases, func(t *cuetest.T, tc *testCase) {
 			v := cfg.getValue(t.T, tc.value)
 			x := v.LookupPath(ParsePath("#v"))

internal/core/adt/closed.go

@@ -344,6 +344,9 @@ func isClosed(v *Vertex) bool {
 // Accept determines whether f is allowed in n. It uses the OpContext for
 // caching administrative fields.
 func Accept(ctx *OpContext, n *Vertex, f Feature) (found, required bool) {
+	if ctx.isDevVersion() {
+		return n.accept(ctx, f), true
+	}
 	ctx.generation++
 	ctx.todo = nil
 

internal/core/adt/composite.go

@@ -964,10 +964,16 @@ func (v *Vertex) IsClosedList() bool {
 
 // TODO: return error instead of boolean? (or at least have version that does.)
 func (v *Vertex) Accept(ctx *OpContext, f Feature) bool {
+	// TODO(#543): remove this check.
+	if f.IsDef() {
+		return true
+	}
+
 	if f.IsHidden() || f.IsLet() {
 		return true
 	}
 
+	v = v.Indirect()
 	if x, ok := v.BaseValue.(*Disjunction); ok {
 		for _, v := range x.Values {
 			if x, ok := v.(*Vertex); ok && x.Accept(ctx, f) {
@@ -999,6 +1005,14 @@ func (v *Vertex) Accept(ctx *OpContext, f Feature) bool {
 		}
 	}
 
+	// TODO: move this check to IsClosedStruct. Right now this causes too many
+	// changes in the debug output, and it also appears to be not entirely
+	// correct.
+	if v.HasEllipsis {
+		return true
+
+	}
+
 	if !v.IsClosedStruct() || v.Lookup(f) != nil {
 		return true
 	}

internal/core/adt/unify.go

@@ -721,3 +721,22 @@ func (v *Vertex) lookup(c *OpContext, pos token.Pos, f Feature, flags combinedFl
 	v.reportFieldIndexError(c, pos, f)
 	return nil
 }
+
+// accept reports whether the given feature is allowed by the pattern
+// constraints.
+func (v *Vertex) accept(ctx *OpContext, f Feature) bool {
+	// TODO: this is already handled by callers at the moment, but it may be
+	// better design to move this here.
+	// if v.LookupRaw(f) != nil {
+	// 	return true, true
+	// }
+
+	v = v.Indirect()
+
+	pc := v.PatternConstraints
+	if pc == nil {
+		return false
+	}
+
+	return matchPattern(ctx, pc.Allowed, f)
+}