internal/core/adt: fix hang on cyclic pattern constraints

This issue triggered a cyclic evaluation in pattern constraints.

It seems like there is still an issue as the evaluation
of `x`  in`and` may trigger a cyclic evaluation of `and`.
However, `and` only does a shallow evaluation, and
inserting `x`'s conjuncts into itself is identified as
idempotent and thus further evaluation is skipped.

The same mechanism does not work for pattern
constraints (...T in this case), so we disable evaluation
there. Note that v2 does not evaluate patterns either.
It is not strictly necessary, but increases precision with
subsumption and equality, which in turn improves
efficiency of disambiguating disjunctions.

Fixes #3326

Signed-off-by: Marcel van Lohuizen <[email protected]>
Change-Id: I5ac7f5a19c72de64d95bbdb186e2393e6bc824a1
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1202604
Unity-Result: CUE porcuepine <[email protected]>
Reviewed-by: Matthew Sackman <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>

Author: mpvl

cue/testdata/cycle/evaluate.txtar

@@ -133,7 +133,8 @@ letCycleOK.t2.a.X: structural cycle:
     ./in.cue:23:11
 letCycleFail.t1.a.X: structural cycle:
     ./in.cue:33:11
-disjunctionCycle.b: cannot use 1 (type int) as list in argument 1 to and:
+disjunctionCycle.b: cannot use 1 (type int) as type list:
+    ./in.cue:56:5
     ./in.cue:56:9
 b: structural cycle:
     ./in.cue:62:6
@@ -211,15 +212,18 @@ Result:
   disjunctionCycle: (_|_){
     // [eval]
     a: (_|_){
-      // [eval] disjunctionCycle.b: cannot use 1 (type int) as list in argument 1 to and:
+      // [eval] disjunctionCycle.b: cannot use 1 (type int) as type list:
+      //     ./in.cue:56:5
       //     ./in.cue:56:9
     }
     b: (_|_){
-      // [eval] disjunctionCycle.b: cannot use 1 (type int) as list in argument 1 to and:
+      // [eval] disjunctionCycle.b: cannot use 1 (type int) as type list:
+      //     ./in.cue:56:5
       //     ./in.cue:56:9
     }
     c: (_|_){
-      // [eval] disjunctionCycle.b: cannot use 1 (type int) as list in argument 1 to and:
+      // [eval] disjunctionCycle.b: cannot use 1 (type int) as type list:
+      //     ./in.cue:56:5
       //     ./in.cue:56:9
     }
   }
@@ -349,7 +353,7 @@ Result:
 diff old new
 --- old
 +++ new
-@@ -1,47 +1,49 @@
+@@ -1,50 +1,50 @@
  Errors:
 -closeCycle.a: structural cycle
 -closeCycle.b.d: structural cycle
@@ -361,17 +365,20 @@ diff old new
 -letCycleFail.t1.a.c: structural cycle
 -structCycle.a: structural cycle
 -structCycle.b.d: structural cycle
--disjunctionCycle.a: cannot use 1 (type int) as list in argument 1 to and:
+-disjunctionCycle.a: cannot use 1 (type int) as type list:
+-    ./in.cue:56:5
 -    ./in.cue:56:9
 +printCycle.a.X: structural cycle
 +structCycle.c: structural cycle
 +letCycleOK.t2.a.X: structural cycle:
 +    ./in.cue:23:11
 +letCycleFail.t1.a.X: structural cycle:
 +    ./in.cue:33:11
- disjunctionCycle.b: cannot use 1 (type int) as list in argument 1 to and:
+ disjunctionCycle.b: cannot use 1 (type int) as type list:
+     ./in.cue:56:5
      ./in.cue:56:9
--disjunctionCycle.c: cannot use 1 (type int) as list in argument 1 to and:
+-disjunctionCycle.c: cannot use 1 (type int) as type list:
+-    ./in.cue:56:5
 -    ./in.cue:56:9
  b: structural cycle:
      ./in.cue:62:6
@@ -424,7 +431,7 @@ diff old new
        }
      }
    }
-@@ -56,20 +58,16 @@
+@@ -59,20 +59,16 @@
            // [structural cycle] letCycleFail.t1.a.X: structural cycle
          }
          c: (_|_){
@@ -455,32 +462,36 @@ diff old new
          }
          x: (struct){
            y: (string){ "" }
-@@ -85,15 +83,15 @@
+@@ -88,17 +84,17 @@
    disjunctionCycle: (_|_){
      // [eval]
      a: (_|_){
--      // [eval] disjunctionCycle.a: cannot use 1 (type int) as list in argument 1 to and:
+-      // [eval] disjunctionCycle.a: cannot use 1 (type int) as type list:
+-      //     ./in.cue:56:5
 -      //     ./in.cue:56:9
 -    }
 -    b: (_|_){
--      // [eval] disjunctionCycle.b: cannot use 1 (type int) as list in argument 1 to and:
+-      // [eval] disjunctionCycle.b: cannot use 1 (type int) as type list:
+-      //     ./in.cue:56:5
 -      //     ./in.cue:56:9
 -    }
 -    c: (_|_){
--      // [eval] disjunctionCycle.c: cannot use 1 (type int) as list in argument 1 to and:
-+      // [eval] disjunctionCycle.b: cannot use 1 (type int) as list in argument 1 to and:
+-      // [eval] disjunctionCycle.c: cannot use 1 (type int) as type list:
++      // [eval] disjunctionCycle.b: cannot use 1 (type int) as type list:
++      //     ./in.cue:56:5
 +      //     ./in.cue:56:9
 +    }
 +    b: (_|_){
-+      // [eval] disjunctionCycle.b: cannot use 1 (type int) as list in argument 1 to and:
++      // [eval] disjunctionCycle.b: cannot use 1 (type int) as type list:
++      //     ./in.cue:56:5
 +      //     ./in.cue:56:9
 +    }
 +    c: (_|_){
-+      // [eval] disjunctionCycle.b: cannot use 1 (type int) as list in argument 1 to and:
++      // [eval] disjunctionCycle.b: cannot use 1 (type int) as type list:
+       //     ./in.cue:56:5
        //     ./in.cue:56:9
      }
-   }
-@@ -118,80 +116,79 @@
+@@ -124,80 +120,79 @@
      }
      b: (struct){
      }
@@ -611,7 +622,7 @@ diff old new
      }
    }
    closeFail: (_|_){
-@@ -201,21 +198,22 @@
+@@ -207,21 +202,22 @@
      }
      x: (_|_){
        // [eval]
@@ -653,11 +664,14 @@ closeFail.x.b: field not allowed:
 letCycleFail.t1.a.c: structural cycle
 structCycle.a: structural cycle
 structCycle.b.d: structural cycle
-disjunctionCycle.a: cannot use 1 (type int) as list in argument 1 to and:
+disjunctionCycle.a: cannot use 1 (type int) as type list:
+    ./in.cue:56:5
     ./in.cue:56:9
-disjunctionCycle.b: cannot use 1 (type int) as list in argument 1 to and:
+disjunctionCycle.b: cannot use 1 (type int) as type list:
+    ./in.cue:56:5
     ./in.cue:56:9
-disjunctionCycle.c: cannot use 1 (type int) as list in argument 1 to and:
+disjunctionCycle.c: cannot use 1 (type int) as type list:
+    ./in.cue:56:5
     ./in.cue:56:9
 b: structural cycle:
     ./in.cue:62:6
@@ -729,15 +743,18 @@ Result:
   disjunctionCycle: (_|_){
     // [eval]
     a: (_|_){
-      // [eval] disjunctionCycle.a: cannot use 1 (type int) as list in argument 1 to and:
+      // [eval] disjunctionCycle.a: cannot use 1 (type int) as type list:
+      //     ./in.cue:56:5
       //     ./in.cue:56:9
     }
     b: (_|_){
-      // [eval] disjunctionCycle.b: cannot use 1 (type int) as list in argument 1 to and:
+      // [eval] disjunctionCycle.b: cannot use 1 (type int) as type list:
+      //     ./in.cue:56:5
       //     ./in.cue:56:9
     }
     c: (_|_){
-      // [eval] disjunctionCycle.c: cannot use 1 (type int) as list in argument 1 to and:
+      // [eval] disjunctionCycle.c: cannot use 1 (type int) as type list:
+      //     ./in.cue:56:5
       //     ./in.cue:56:9
     }
   }

cue/testdata/cycle/patterns.txtar

@@ -1,3 +1,4 @@
+// Excluded from V2, which no longer passes.
 Lots of cycle-reference goodness.
 
 -- in.cue --
@@ -9,6 +10,19 @@ c: a:        int
 a:        b
 b: [string]: int
 c: a:        int
+
+-- issue3326.cue --
+issue3326: {
+	// This issue triggered a cyclic evaluation in pattern constraints.
+	//
+	// It seems like there is still an issue as the evaluation of `x` in
+	// `and` may trigger a cyclic evaluation of `and`. However, `and` only
+	// does a shallow evaluation, and inserting `x`'s conjuncts into itself
+	// is identified as idempotent and thus further evaluation is skipped.
+	_self: x: [...and(x)]
+	_self
+	x: [1]
+}
 -- out/eval/stats --
 Leaks:  0
 Freed:  7
@@ -19,6 +33,47 @@ Retain: 2
 Unifications: 7
 Conjuncts:    59
 Disjuncts:    9
+-- out/evalalpha --
+(struct){
+  a: (struct){
+    a: (int){ int }
+  }
+  b: (struct){
+    a: (int){ int }
+  }
+  c: (struct){
+    a: (int){ int }
+  }
+  issue3326: (struct){
+    _self: (struct){
+      x: (list){
+      }
+    }
+    x: (#list){
+      0: (int){ 1 }
+    }
+    a: (int){ int }
+  }
+}
+-- diff/-out/evalalpha<==>+out/eval --
+diff old new
+--- old
++++ new
+@@ -8,4 +8,14 @@
+   c: (struct){
+     a: (int){ int }
+   }
++  issue3326: (struct){
++    _self: (struct){
++      x: (list){
++      }
++    }
++    x: (#list){
++      0: (int){ 1 }
++    }
++    a: (int){ int }
++  }
+ }
 -- out/eval --
 (struct){
   a: (struct){
@@ -51,3 +106,17 @@ Disjuncts:    9
     a: int
   }
 }
+--- issue3326.cue
+{
+  issue3326: {
+    _self: {
+      x: [
+        ...and(〈1;x〉),
+      ]
+    }
+    〈0;_self〉
+    x: [
+      1,
+    ]
+  }
+}

internal/core/adt/composite.go

@@ -206,6 +206,10 @@ type Vertex struct {
 	// Used for cycle detection.
 	IsDynamic bool
 
+	// IsPatternConstraint indicates that this Vertex is an entry in
+	// Vertex.PatternConstraints.
+	IsPatternConstraint bool
+
 	// nonRooted indicates that this Vertex originates within the context of
 	// a dynamic, or inlined, Vertex (e.g. `{out: ...}.out``). Note that,
 	// through reappropriation, this Vertex may become rooted down the line.

internal/core/adt/constraints.go

@@ -93,7 +93,10 @@ func (n *nodeContext) insertConstraint(pattern Value, c Conjunct) bool {
 	}
 
 	if constraint == nil {
-		constraint = &Vertex{}
+		constraint = &Vertex{
+			// See "Self-referencing patterns" in cycle.go
+			IsPatternConstraint: true,
+		}
 		pcs.Pairs = append(pcs.Pairs, PatternConstraint{
 			Pattern:    pattern,
 			Constraint: constraint,

internal/core/adt/context.go

@@ -656,6 +656,20 @@ func (c *OpContext) Evaluate(env *Environment, x Expr) (result Value, complete b
 	return val, true
 }
 
+// EvaluateKeepState does an evaluate, but leaves any errors an cycle info
+// within the context.
+func (c *OpContext) EvaluateKeepState(x Expr) (result Value) {
+	src := c.src
+	c.src = x.Source()
+
+	result, ci := c.evalStateCI(x, final(partial, concreteKnown))
+
+	c.src = src
+	c.ci = ci
+
+	return result
+}
+
 func (c *OpContext) evaluateRec(v Conjunct, state combinedFlags) Value {
 	x := v.Expr()
 	s := c.PushConjunct(v)
@@ -687,10 +701,18 @@ func (c *OpContext) value(x Expr, state combinedFlags) (result Value) {
 }
 
 func (c *OpContext) evalState(v Expr, state combinedFlags) (result Value) {
+	result, _ = c.evalStateCI(v, state)
+	return result
+}
+
+func (c *OpContext) evalStateCI(v Expr, state combinedFlags) (result Value, ci CloseInfo) {
 	savedSrc := c.src
 	c.src = v.Source()
 	err := c.errs
 	c.errs = nil
+	// Save the old CloseInfo and restore after evaluate to avoid detecting
+	// spurious cycles.
+	saved := c.ci
 
 	defer func() {
 		c.errs = CombineErrors(c.src, c.errs, err)
@@ -721,23 +743,29 @@ func (c *OpContext) evalState(v Expr, state combinedFlags) (result Value) {
 			result = c.errs
 		}
 		c.src = savedSrc
+
+		// TODO(evalv3): this c.ci should be passed to the caller who may need
+		// it to continue cycle detection for partially evaluated values.
+		// Either this or we must prove that this is covered by structural cycle
+		// detection.
+		c.ci = saved
 	}()
 
 	switch x := v.(type) {
 	case Value:
-		return x
+		return x, c.ci
 
 	case Evaluator:
 		v := x.evaluate(c, state)
-		return v
+		return v, c.ci
 
 	case Resolver:
 		arc := x.resolve(c, state)
 		if c.HasErr() {
-			return nil
+			return nil, c.ci
 		}
 		if arc == nil {
-			return nil
+			return nil, c.ci
 		}
 		// TODO(deref): what is the right level of dereferencing here?
 		// DerefValue seems to work too.
@@ -748,9 +776,6 @@ func (c *OpContext) evalState(v Expr, state combinedFlags) (result Value) {
 		// TODO: is this indirect necessary?
 		// arc = arc.Indirect()
 
-		// Save the old CloseInfo and restore after evaluate to avoid detecting
-		// spurious cycles.
-		saved := c.ci
 		n := arc.state
 		if c.isDevVersion() {
 			n = arc.getState(c)
@@ -787,7 +812,7 @@ func (c *OpContext) evalState(v Expr, state combinedFlags) (result Value) {
 						err := c.Newf("cycle with field %v", x)
 						b := &Bottom{Code: CycleError, Err: err}
 						s.setBaseValue(b)
-						return b
+						return b, c.ci
 						// TODO: use this instead, as is usual for incomplete errors,
 						// and also move this block one scope up to also apply to
 						// defined arcs. In both cases, though, doing so results in
@@ -796,13 +821,13 @@ func (c *OpContext) evalState(v Expr, state combinedFlags) (result Value) {
 						// return nil
 					}
 					c.undefinedFieldError(v, IncompleteError)
-					return nil
+					return nil, c.ci
 				}
 			}
 		}
 		v := c.evaluate(arc, x, state)
-		c.ci = saved
-		return v
+
+		return v, c.ci
 
 	default:
 		// This can only happen, really, if v == nil, which is not allowed.