cue: fix Value.Unify closedness issues for evalv3

When using Unify to unify cue.Values, the conjuncts of each value need to be grouped into their own closeContext. This prevents conjuncts that are interpreted as embeddings (e.g. because they are wrapped in a StructLit associated with a file) from erasing closedness. Basically, when unifying {close({})} and {a: 5}, the former should not be treated as an embedding that is at the same level as the latter. The fix generally improves closedness check behavior for V3, and thus we could also disable the use of "allowed" for V3. This is good, as this function does not work well for V3. This means that all test cases of Issue 3562 could be fixed. This fix also fixes discrepancies in TestAPI, which were resulting from not replicating the V2 logic in V3. We include this change in the same CL as it uses the same mechanism and it will be useful to consider these together when studying this change down the line. Fixes #3562 Signed-off-by: Marcel van Lohuizen <[email protected]> Change-Id: If6acd2a1ec7f10928180688782d1fae4a3a8ca00 Dispatch-Trailer: {"type":"trybot","CL":1204365,"patchset":3,"ref":"refs/changes/65/1204365/3","targetBranch":"master"}
cue-lang · Nov 20, 2024 · 43abe74 · 43abe74
1 parent 0f35b93
commit 43abe74
Show file tree

Hide file tree

Showing 15 changed files with 99 additions and 118 deletions.
diff --git a/cue/types.go b/cue/types.go
@@ -1693,6 +1693,8 @@ func (v Value) Subsume(w Value, opts ...Option) error {
 	return p.Value(ctx, v.v, w.v)
 }
 
+// TODO: this is likely not correct for V3. There are some cases where this is
+// still used for V3. Transition away from those.
 func allowed(ctx *adt.OpContext, parent, n *adt.Vertex) *adt.Bottom {
 	if !parent.IsClosedList() && !parent.IsClosedStruct() {
 		return nil
@@ -1709,11 +1711,17 @@ func allowed(ctx *adt.OpContext, parent, n *adt.Vertex) *adt.Bottom {
 	return nil
 }
 
-func addConjuncts(dst, src *adt.Vertex) {
+func addConjuncts(ctx *adt.OpContext, dst, src *adt.Vertex) {
 	c := adt.MakeRootConjunct(nil, src)
+	c.CloseInfo.GroupUnify = true
+
 	if src.ClosedRecursive {
-		var root adt.CloseInfo
-		c.CloseInfo = root.SpawnRef(src, src.ClosedRecursive, nil)
+		if ctx.Version == internal.EvalV2 {
+			var root adt.CloseInfo
+			c.CloseInfo = root.SpawnRef(src, src.ClosedRecursive, nil)
+		} else {
+			c.CloseInfo.FromDef = true
+		}
 	}
 	dst.AddConjunct(c)
 }
@@ -1730,11 +1738,11 @@ func (v Value) Unify(w Value) Value {
 		return v
 	}
 
+	ctx := newContext(v.idx)
 	n := &adt.Vertex{}
-	addConjuncts(n, v.v)
-	addConjuncts(n, w.v)
+	addConjuncts(ctx, n, v.v)
+	addConjuncts(ctx, n, w.v)
 
-	ctx := newContext(v.idx)
 	n.Finalize(ctx)
 
 	n.Parent = v.v.Parent
@@ -1744,11 +1752,13 @@ func (v Value) Unify(w Value) Value {
 	if err := n.Err(ctx); err != nil {
 		return makeValue(v.idx, n, v.parent_)
 	}
-	if err := allowed(ctx, v.v, n); err != nil {
-		return newErrValue(w, err)
-	}
-	if err := allowed(ctx, w.v, n); err != nil {
-		return newErrValue(v, err)
+	if ctx.Version == internal.EvalV2 {
+		if err := allowed(ctx, v.v, n); err != nil {
+			return newErrValue(w, err)
+		}
+		if err := allowed(ctx, w.v, n); err != nil {
+			return newErrValue(v, err)
+		}
 	}
 
 	return makeValue(v.idx, n, v.parent_)

diff --git a/cue/types_test.go b/cue/types_test.go
@@ -33,6 +33,8 @@ import (
 	"cuelang.org/go/internal/astinternal"
 	"cuelang.org/go/internal/core/adt"
 	"cuelang.org/go/internal/core/debug"
+	"cuelang.org/go/internal/core/runtime"
+	"cuelang.org/go/internal/cuedebug"
 	"cuelang.org/go/internal/cuetdtest"
 	"cuelang.org/go/internal/cuetest"
 	"cuelang.org/go/internal/tdtest"
@@ -139,8 +141,6 @@ func TestAPI(t *testing.T) {
 			continue
 		}
 		cuetdtest.FullMatrix.Run(t, "", func(t *testing.T, m *cuetdtest.M) {
-			m.TODO_V3(t) // P1: faulty closedness
-
 			ctx := m.CueContext()
 
 			valIn := mustCompile(t, ctx, tc.input)
@@ -2175,6 +2175,47 @@ func TestUnify(t *testing.T) {
 	})
 }
 
+// TestUnify2 is similar to TestUnify, but uses CompileString and Validate.
+func TestUnify2(t *testing.T) {
+	type testCase struct {
+		name   string
+		a      string
+		b      string
+		output string
+		err    bool
+	}
+	testCases := []testCase{{
+		a:   `null | close({})`,
+		b:   `quux: "boom"`,
+		err: true,
+	}, {
+		a: `close({[=~"a"]: _})`,
+		b: `a: 1`,
+	}, {
+		a: `close({{[=~"a"]: _}})`,
+		b: `a: 1`,
+	}}
+
+	adt.DebugDeps = true
+
+	cuetdtest.Run(t, testCases, func(t *cuetdtest.T, tc *testCase) {
+		ctx := t.M.CueContext()
+
+		a := ctx.CompileString(tc.a)
+		b := ctx.CompileString(tc.b)
+
+		r := (*runtime.Runtime)(ctx)
+		r.SetDebugOptions(&cuedebug.Config{LogEval: 1})
+
+		c := a.Unify(b)
+
+		err := c.Validate()
+		t.Equal(err != nil, tc.err, "hasError")
+		t.Log("error:", err)
+	})
+
+}
+
 func TestUnifyAccept(t *testing.T) {
 	type testCase struct {
 		value string

diff --git a/encoding/jsonschema/external_teststats.txt b/encoding/jsonschema/external_teststats.txt
@@ -6,8 +6,8 @@ v2:
 
 v3:
 	schema extract (pass / total): 1013 / 1363 = 74.3%
-	tests (pass / total): 3611 / 4803 = 75.2%
-	tests on extracted schemas (pass / total): 3611 / 3865 = 93.4%
+	tests (pass / total): 3621 / 4803 = 75.4%
+	tests on extracted schemas (pass / total): 3621 / 3865 = 93.7%
 
 Optional tests
 
@@ -18,5 +18,5 @@ v2:
 
 v3:
 	schema extract (pass / total): 220 / 274 = 80.3%
-	tests (pass / total): 1581 / 2372 = 66.7%
-	tests on extracted schemas (pass / total): 1581 / 2223 = 71.1%
+	tests (pass / total): 1596 / 2372 = 67.3%
+	tests on extracted schemas (pass / total): 1596 / 2223 = 71.8%
diff --git a/encoding/jsonschema/testdata/external/tests/draft2019-09/additionalProperties.json b/encoding/jsonschema/testdata/external/tests/draft2019-09/additionalProperties.json
@@ -27,10 +27,7 @@
 					"bar": 2,
 					"quux": "boom"
 				},
-				"valid": false,
-				"skip": {
-					"v3": "unexpected success"
-				}
+				"valid": false
 			},
 			{
 				"description": "ignores arrays",
@@ -83,10 +80,7 @@
 				"data": {
 					"élmény": 2
 				},
-				"valid": false,
-				"skip": {
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},

diff --git a/encoding/jsonschema/testdata/external/tests/draft2019-09/optional/ecmascript-regex.json b/encoding/jsonschema/testdata/external/tests/draft2019-09/optional/ecmascript-regex.json
@@ -606,10 +606,7 @@
 				"data": {
 					"l'ecole": "pas de vraie vie"
 				},
-				"valid": true,
-				"skip": {
-					"v3": "field not allowed: \"l'ecole\"\n"
-				}
+				"valid": true
 			},
 			{
 				"description": "literal unicode character in json string",
@@ -664,10 +661,7 @@
 				"data": {
 					"l'ecole": "pas de vraie vie"
 				},
-				"valid": true,
-				"skip": {
-					"v3": "field not allowed: \"l'ecole\"\n"
-				}
+				"valid": true
 			}
 		]
 	},
@@ -687,10 +681,7 @@
 				"data": {
 					"42": "life, the universe, and everything"
 				},
-				"valid": true,
-				"skip": {
-					"v3": "field not allowed: \"42\"\n"
-				}
+				"valid": true
 			},
 			{
 				"description": "ascii non-digits",

diff --git a/encoding/jsonschema/testdata/external/tests/draft2020-12/additionalProperties.json b/encoding/jsonschema/testdata/external/tests/draft2020-12/additionalProperties.json
@@ -27,10 +27,7 @@
 					"bar": 2,
 					"quux": "boom"
 				},
-				"valid": false,
-				"skip": {
-					"v3": "unexpected success"
-				}
+				"valid": false
 			},
 			{
 				"description": "ignores arrays",
@@ -83,10 +80,7 @@
 				"data": {
 					"élmény": 2
 				},
-				"valid": false,
-				"skip": {
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},

diff --git a/encoding/jsonschema/testdata/external/tests/draft2020-12/optional/ecmascript-regex.json b/encoding/jsonschema/testdata/external/tests/draft2020-12/optional/ecmascript-regex.json
@@ -624,10 +624,7 @@
 				"data": {
 					"l'ecole": "pas de vraie vie"
 				},
-				"valid": true,
-				"skip": {
-					"v3": "field not allowed: \"l'ecole\"\n"
-				}
+				"valid": true
 			},
 			{
 				"description": "literal unicode character in json string",
@@ -682,10 +679,7 @@
 				"data": {
 					"l'ecole": "pas de vraie vie"
 				},
-				"valid": true,
-				"skip": {
-					"v3": "field not allowed: \"l'ecole\"\n"
-				}
+				"valid": true
 			}
 		]
 	},
@@ -705,10 +699,7 @@
 				"data": {
 					"42": "life, the universe, and everything"
 				},
-				"valid": true,
-				"skip": {
-					"v3": "field not allowed: \"42\"\n"
-				}
+				"valid": true
 			},
 			{
 				"description": "ascii non-digits",

diff --git a/encoding/jsonschema/testdata/external/tests/draft4/additionalProperties.json b/encoding/jsonschema/testdata/external/tests/draft4/additionalProperties.json
@@ -26,10 +26,7 @@
 					"bar": 2,
 					"quux": "boom"
 				},
-				"valid": false,
-				"skip": {
-					"v3": "unexpected success"
-				}
+				"valid": false
 			},
 			{
 				"description": "ignores arrays",
@@ -81,10 +78,7 @@
 				"data": {
 					"élmény": 2
 				},
-				"valid": false,
-				"skip": {
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},

diff --git a/encoding/jsonschema/testdata/external/tests/draft4/optional/ecmascript-regex.json b/encoding/jsonschema/testdata/external/tests/draft4/optional/ecmascript-regex.json
@@ -589,10 +589,7 @@
 				"data": {
 					"l'ecole": "pas de vraie vie"
 				},
-				"valid": true,
-				"skip": {
-					"v3": "field not allowed: \"l'ecole\"\n"
-				}
+				"valid": true
 			},
 			{
 				"description": "literal unicode character in json string",
@@ -646,10 +643,7 @@
 				"data": {
 					"l'ecole": "pas de vraie vie"
 				},
-				"valid": true,
-				"skip": {
-					"v3": "field not allowed: \"l'ecole\"\n"
-				}
+				"valid": true
 			}
 		]
 	},
@@ -668,10 +662,7 @@
 				"data": {
 					"42": "life, the universe, and everything"
 				},
-				"valid": true,
-				"skip": {
-					"v3": "field not allowed: \"42\"\n"
-				}
+				"valid": true
 			},
 			{
 				"description": "ascii non-digits",

diff --git a/encoding/jsonschema/testdata/external/tests/draft6/additionalProperties.json b/encoding/jsonschema/testdata/external/tests/draft6/additionalProperties.json
@@ -26,10 +26,7 @@
 					"bar": 2,
 					"quux": "boom"
 				},
-				"valid": false,
-				"skip": {
-					"v3": "unexpected success"
-				}
+				"valid": false
 			},
 			{
 				"description": "ignores arrays",
@@ -81,10 +78,7 @@
 				"data": {
 					"élmény": 2
 				},
-				"valid": false,
-				"skip": {
-					"v3": "unexpected success"
-				}
+				"valid": false
 			}
 		]
 	},

diff --git a/encoding/jsonschema/testdata/external/tests/draft6/optional/ecmascript-regex.json b/encoding/jsonschema/testdata/external/tests/draft6/optional/ecmascript-regex.json
@@ -589,10 +589,7 @@
 				"data": {
 					"l'ecole": "pas de vraie vie"
 				},
-				"valid": true,
-				"skip": {
-					"v3": "field not allowed: \"l'ecole\"\n"
-				}
+				"valid": true
 			},
 			{
 				"description": "literal unicode character in json string",
@@ -646,10 +643,7 @@
 				"data": {
 					"l'ecole": "pas de vraie vie"
 				},
-				"valid": true,
-				"skip": {
-					"v3": "field not allowed: \"l'ecole\"\n"
-				}
+				"valid": true
 			}
 		]
 	},
@@ -668,10 +662,7 @@
 				"data": {
 					"42": "life, the universe, and everything"
 				},
-				"valid": true,
-				"skip": {
-					"v3": "field not allowed: \"42\"\n"
-				}
+				"valid": true
 			},
 			{
 				"description": "ascii non-digits",