internal/core/adt: fix regressions in disjunct elimination
Fix finalization of disjunctions. This bug was introduced
with the latest (critical) performance optimization for
disjunctions and was miraculously not caught by tests.
This also fixes a regression in v0.3 from v0.2 where
cyclic disjunctons were sometimes not fully resolved.
TODO: it probably makes sense to resolve defaults and
disambiguate not considering optionals during the
finalization step of disjunctions. A more concrete
proposal (no pun intended) is in the works.
Fixes #702
Fixes #700
Change-Id: Iae2a1c2a609dfb3a7e9f5ed3d7c6ee5ee17b3e55
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/8462
Reviewed-by: CUE cueckoo <cueckoo@gmail.com>
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
diff --git a/cue/testdata/cycle/051_resolved_self-reference_cycles_with_disjunction.txtar b/cue/testdata/cycle/051_resolved_self-reference_cycles_with_disjunction.txtar
index 2d2d32d..e2685bc 100644
--- a/cue/testdata/cycle/051_resolved_self-reference_cycles_with_disjunction.txtar
+++ b/cue/testdata/cycle/051_resolved_self-reference_cycles_with_disjunction.txtar
@@ -17,7 +17,7 @@
xa3: 6 & xa1-2
xa4: xa2 + 2
-// The second disjunct in xb4 can be eliminated as both disjuncts
+// The second disjunct of xb1 can be eliminated as both disjuncts
// of xb3 result in an incompatible sum when substituted.
xb1: (xb2 & 8) | (xb4 & 9)
xb2: xb3 + 2
@@ -174,16 +174,10 @@
xa2: (int){ 8 }
xa3: (int){ 6 }
xa4: (int){ 10 }
- xb1: (int){ |((int){ 8 }, (int){ 9 }) }
- xb2: (_|_){
- // [incomplete] xb2: unresolved disjunction 6 | 9 (type int):
- // ./in.cue:18:6
- }
- xb3: (int){ |((int){ 6 }, (int){ 9 }) }
- xb4: (_|_){
- // [incomplete] xb2: unresolved disjunction 6 | 9 (type int):
- // ./in.cue:18:6
- }
+ xb1: (int){ 8 }
+ xb2: (int){ 8 }
+ xb3: (int){ 6 }
+ xb4: (int){ 10 }
xc1: (int){ |((int){ 8 }, (int){ 9 }) }
xc2: (int){ 8 }
xc3: (_|_){
@@ -226,16 +220,10 @@
// ./in.cue:45:6
// ./in.cue:45:10
}
- xf1: (int){ |((int){ 8 }, (int){ 9 }) }
- xf2: (_|_){
- // [incomplete] xf2: unresolved disjunction 6 | 9 (type int):
- // ./in.cue:52:6
- }
- xf3: (int){ |((int){ 6 }, (int){ 9 }) }
- xf4: (_|_){
- // [incomplete] xf2: unresolved disjunction 6 | 9 (type int):
- // ./in.cue:52:6
- }
+ xf1: (int){ 8 }
+ xf2: (int){ 8 }
+ xf3: (int){ 6 }
+ xf4: (int){ 10 }
z1: (int){ |((int){ 11 }, (int){ 13 }) }
z2: (int){ 10 }
z3: (_|_){
diff --git a/cue/testdata/cycle/052_resolved_self-reference_cycles_with_disjunction_with_defaults.txtar b/cue/testdata/cycle/052_resolved_self-reference_cycles_with_disjunction_with_defaults.txtar
index b807253..60e48f6 100644
--- a/cue/testdata/cycle/052_resolved_self-reference_cycles_with_disjunction_with_defaults.txtar
+++ b/cue/testdata/cycle/052_resolved_self-reference_cycles_with_disjunction_with_defaults.txtar
@@ -10,9 +10,6 @@
xa3: 6 & xa1-2
xa4: xa2 + 2
-// As xb3 is a disjunction, xb2 cannot be resolved and evaluating
-// the cycle completely is broken. However, it is not an error
-// as the user might still resolve the disjunction.
xb1: *(xb2 & 8) | (xb4 & 9)
xb2: xb3 + 2
xb3: *(6 & (xb1 - 2)) | (xb4 & 9)
@@ -131,11 +128,11 @@
Errors:
xe1: 2 errors in empty disjunction:
xe1: conflicting values 8 and 9:
- ./in.cue:36:14
- ./in.cue:41:6
-xe3: conflicting values 7 and 6:
+ ./in.cue:33:14
./in.cue:38:6
- ./in.cue:38:10
+xe3: conflicting values 7 and 6:
+ ./in.cue:35:6
+ ./in.cue:35:10
Result:
(_|_){
@@ -146,7 +143,7 @@
xa4: (int){ 10 }
xb1: (int){ 8 }
xb2: (int){ 8 }
- xb3: (int){ |(*(int){ 6 }, (int){ 9 }) }
+ xb3: (int){ 6 }
xb4: (int){ 10 }
xc1: (int){ |(*(int){ 8 }, (int){ 9 }) }
xc2: (int){ 8 }
@@ -161,31 +158,31 @@
xe1: (_|_){
// [eval] xe1: 2 errors in empty disjunction:
// xe1: conflicting values 8 and 9:
- // ./in.cue:36:14
- // ./in.cue:41:6
- // xe3: conflicting values 7 and 6:
+ // ./in.cue:33:14
// ./in.cue:38:6
- // ./in.cue:38:10
+ // xe3: conflicting values 7 and 6:
+ // ./in.cue:35:6
+ // ./in.cue:35:10
}
xe2: (_|_){
// [eval] xe3: conflicting values 7 and 6:
- // ./in.cue:38:6
- // ./in.cue:38:10
+ // ./in.cue:35:6
+ // ./in.cue:35:10
}
xe3: (_|_){
// [eval] xe3: conflicting values 7 and 6:
- // ./in.cue:38:6
- // ./in.cue:38:10
+ // ./in.cue:35:6
+ // ./in.cue:35:10
}
xe4: (_|_){
// [eval] xe3: conflicting values 7 and 6:
- // ./in.cue:38:6
- // ./in.cue:38:10
+ // ./in.cue:35:6
+ // ./in.cue:35:10
}
xe5: (_|_){
// [eval] xe3: conflicting values 7 and 6:
- // ./in.cue:38:6
- // ./in.cue:38:10
+ // ./in.cue:35:6
+ // ./in.cue:35:10
}
z1: (int){ |(*(int){ 11 }, (int){ 13 }) }
z2: (int){ 10 }
diff --git a/cue/testdata/disjunctions/incomplete.txtar b/cue/testdata/disjunctions/incomplete.txtar
new file mode 100644
index 0000000..b910e45
--- /dev/null
+++ b/cue/testdata/disjunctions/incomplete.txtar
@@ -0,0 +1,110 @@
+-- in.cue --
+import "encoding/yaml"
+
+issue700: {
+ a:y: "y"
+ test1: *a.x | 1
+ test2: *a.y | 1
+}
+
+lookup: {
+ x: {
+ name: "Hello"
+ }
+ y: x.a
+ ok1: *x.a | x
+ ok2: x | *x.a
+ ok3: x.a | *x
+ ok4: *x | x.a
+ ok5: x | x.a
+ ok5: x.a | x
+ allFail1: x.a | x.b
+ allFail2: x.a | x.b
+}
+
+func: {
+ s: string
+ ok1: yaml.MarshalStream(s) | yaml.Marshal(s)
+}
+
+-- out/eval --
+(struct){
+ issue700: (struct){
+ a: (struct){
+ y: (string){ "y" }
+ }
+ test1: (int){ 1 }
+ test2: ((int|string)){ |(*(string){ "y" }, (int){ 1 }) }
+ }
+ lookup: (struct){
+ x: (struct){
+ name: (string){ "Hello" }
+ }
+ y: (_|_){
+ // [incomplete] lookup.y: undefined field a:
+ // ./in.cue:13:10
+ }
+ ok1: (struct){
+ name: (string){ "Hello" }
+ }
+ ok2: (struct){
+ name: (string){ "Hello" }
+ }
+ ok3: (struct){
+ name: (string){ "Hello" }
+ }
+ ok4: (struct){
+ name: (string){ "Hello" }
+ }
+ ok5: (struct){
+ name: (string){ "Hello" }
+ }
+ allFail1: (_|_){
+ // [incomplete] lookup.allFail1: 1 errors in empty disjunction:
+ // lookup.allFail1: undefined field a:
+ // ./in.cue:20:18
+ }
+ allFail2: (_|_){
+ // [incomplete] lookup.allFail2: 1 errors in empty disjunction:
+ // lookup.allFail2: undefined field a:
+ // ./in.cue:21:18
+ }
+ }
+ func: (struct){
+ s: (string){ string }
+ ok1: (_|_){
+ // [incomplete] func.ok1: 1 errors in empty disjunction:
+ // func.ok1: non-concrete argument 0:
+ // ./in.cue:26:10
+ }
+ }
+}
+-- out/compile --
+--- in.cue
+{
+ issue700: {
+ a: {
+ y: "y"
+ }
+ test1: (*〈0;a〉.x|1)
+ test2: (*〈0;a〉.y|1)
+ }
+ lookup: {
+ x: {
+ name: "Hello"
+ }
+ y: 〈0;x〉.a
+ ok1: (*〈0;x〉.a|〈0;x〉)
+ ok2: (〈0;x〉|*〈0;x〉.a)
+ ok3: (〈0;x〉.a|*〈0;x〉)
+ ok4: (*〈0;x〉|〈0;x〉.a)
+ ok5: (〈0;x〉|〈0;x〉.a)
+ ok5: (〈0;x〉.a|〈0;x〉)
+ allFail1: (〈0;x〉.a|〈0;x〉.b)
+ allFail2: (〈0;x〉.a|〈0;x〉.b)
+ }
+ func: {
+ s: string
+ ok1: (〈import;"encoding/yaml"〉.MarshalStream(〈0;s〉)|〈import;"encoding/yaml"〉.Marshal(〈0;s〉))
+ }
+}
diff --git a/internal/core/adt/eval.go b/internal/core/adt/eval.go
index 253b02f..3e03ae0 100644
--- a/internal/core/adt/eval.go
+++ b/internal/core/adt/eval.go
@@ -307,10 +307,7 @@
}
n.expandDisjuncts(disState, n, maybeDefault, false)
- for _, d := range n.disjuncts {
- d.finalizeIncomplete()
- d.free()
- }
+ n.finalizeDisjuncts()
switch len(n.disjuncts) {
case 0:
@@ -354,7 +351,11 @@
// We don't do this in postDisjuncts, as it should only be done after
// completing all disjunctions.
- n.finalizeIncomplete()
+ if !n.done() {
+ if err := n.incompleteErrors(); err != nil {
+ n.node.BaseValue = err
+ }
+ }
if state != Finalized {
return
@@ -385,15 +386,34 @@
}
}
-// finalizeIncomplete collects all uncompleted expressions and adds them as
-// errors. As disjuncts are always evaluated with Finalized, care should be
-// taken to only call this after all disjunctions in a path have been completed.
-func (n *nodeContext) finalizeIncomplete() {
- if !n.done() {
- if err := n.incompleteErrors(); err != nil {
- n.node.BaseValue = err
- }
+// finalizeDisjuncts: incomplete errors are kept around and not removed early.
+// This call filters the incomplete errors and removes them
+//
+// This also collects all errors of empty disjunctions. These cannot be
+// collected during the finalization state of individual disjuncts. Care should
+// be taken to only call this after all disjuncts have been finalized.
+func (n *nodeContext) finalizeDisjuncts() {
+ a := n.disjuncts
+ if len(a) == 0 {
+ return
}
+ k := 0
+ for i, d := range a {
+ switch d.finalDone() {
+ case true:
+ a[k], a[i] = d, a[k]
+ k++
+ default:
+ if err := d.incompleteErrors(); err != nil {
+ n.disjunctErrs = append(n.disjunctErrs, err)
+ }
+ }
+ d.free()
+ }
+ if k == 0 {
+ n.makeError()
+ }
+ n.disjuncts = a[:k]
}
func (n *nodeContext) doNotify() {
@@ -668,6 +688,11 @@
a[i] = v
}
}
+ // TODO: disambiguate based on concrete values.
+ // TODO: consider not storing defaults.
+ // if p > 0 {
+ // a = a[:p]
+ // }
return &Disjunction{
Values: a,
NumDefaults: p,
@@ -952,6 +977,19 @@
len(n.exprs) == 0
}
+// finalDone is like done, but allows for cycle errors, which can be ignored
+// as they essentially indicate a = a & _.
+func (n *nodeContext) finalDone() bool {
+ for _, x := range n.exprs {
+ if x.err.Code != CycleError {
+ return false
+ }
+ }
+ return len(n.dynamicFields) == 0 &&
+ len(n.ifClauses) == 0 &&
+ len(n.forClauses) == 0
+}
+
// hasErr is used to determine if an evaluation path, for instance a single
// path after expanding all disjunctions, has an error.
func (n *nodeContext) hasErr() bool {
