internal/core/adt: fix disjunct dedupping regression

Equal was called on unfinalized values, which could lead to premuture elimination of values. Fixes #641 Change-Id: I56343ecee2c8bb6332903b5deed7906fd00925b3 Reviewed-on: https://cue-review.googlesource.com/c/cue/+/8224 Reviewed-by: CUE cueckoo <[email protected]> Reviewed-by: Marcel van Lohuizen <[email protected]>
cuelang · Jan 16, 2021 · a086f74 · a086f74
1 parent ed7d254
commit a086f74
Show file tree

Hide file tree

Showing 2 changed files with 91 additions and 12 deletions.
diff --git a/cue/testdata/eval/disjunctions.txtar b/cue/testdata/eval/disjunctions.txtar
@@ -108,6 +108,31 @@ d100: {
   j: i.bar
 }
 
+issue641: {
+    #A: {
+      type: "a"
+      x:    ""
+    }
+
+    #B: {
+      type: "b"
+      x:    string
+    }
+
+    #C: {
+      b: #A | #B
+    }
+
+    e: [string]: #C & {
+      b: #A | #B
+    }
+
+    e: foobar: #C & {
+      b: #B & {
+        x: "foobar"
+      }
+    }
+}
 -- out/eval --
 Errors:
 f: 2 errors in empty disjunction:
@@ -251,6 +276,33 @@ Result:
       //     ./in.cue:107:6
     }
   }
+  issue641: (struct){
+    #A: (#struct){
+      type: (string){ "a" }
+      x: (string){ "" }
+    }
+    #B: (#struct){
+      type: (string){ "b" }
+      x: (string){ string }
+    }
+    #C: (#struct){
+      b: (struct){ |((#struct){
+          type: (string){ "a" }
+          x: (string){ "" }
+        }, (#struct){
+          type: (string){ "b" }
+          x: (string){ string }
+        }) }
+    }
+    e: (struct){
+      foobar: (#struct){
+        b: (#struct){
+          type: (string){ "b" }
+          x: (string){ "foobar" }
+        }
+      }
+    }
+  }
 }
 -- out/compile --
 --- in.cue
@@ -411,4 +463,29 @@ Result:
     })
     j: 〈0;i〉.bar
   }
+  issue641: {
+    #A: {
+      type: "a"
+      x: ""
+    }
+    #B: {
+      type: "b"
+      x: string
+    }
+    #C: {
+      b: (〈1;#A〉|〈1;#B〉)
+    }
+    e: {
+      [string]: (〈1;#C〉 & {
+        b: (〈2;#A〉|〈2;#B〉)
+      })
+    }
+    e: {
+      foobar: (〈1;#C〉 & {
+        b: (〈2;#B〉 & {
+          x: "foobar"
+        })
+      })
+    }
+  }
 }
diff --git a/internal/core/adt/disjunct.go b/internal/core/adt/disjunct.go
@@ -165,22 +165,16 @@ func (n *nodeContext) expandDisjuncts(
 			}
 			return
 		}
-		if n.node.BaseValue == nil {
-			n.node.BaseValue = n.getValidators()
-		}
-
-		// TODO: clean up this mess:
-		result := *n.node // XXX: n.result = snapshotVertex(n.node)?
-
-		if recursive && state < Finalized {
-			*n = m
-		}
-		n.result = result
-		n.node = &n.result
 
 		if recursive {
+			*n = m
+			n.result = *n.node // XXX: n.result = snapshotVertex(n.node)?
+			n.node = &n.result
 			n.disjuncts = append(n.disjuncts, n)
 		}
+		if n.node.BaseValue == nil {
+			n.node.BaseValue = n.getValidators()
+		}
 
 	case len(n.disjunctions) > 0:
 		// Process full disjuncts to ensure that erroneous disjuncts are
@@ -201,6 +195,8 @@ func (n *nodeContext) expandDisjuncts(
 				// not be done unless there is complete information.
 				state = Partial
 			}
+			//  TODO(perf): ideally always finalize. See comment below
+			// state = Finalized
 
 			for _, dn := range a {
 				switch {
@@ -272,6 +268,12 @@ func (n *nodeContext) expandDisjuncts(
 	outer:
 		for _, d := range n.disjuncts {
 			for _, v := range p.disjuncts {
+				// TODO(perf): not checking equality here may lead to polynomial
+				// blowup. Doesn't seem to affect large configurations, though,
+				// where this could matter at the moment.
+				if state != Finalized {
+					break
+				}
 				if Equal(n.ctx, &v.result, &d.result) {
 					if d.defaultMode == isDefault {
 						v.defaultMode = isDefault