internal/core/adt: fix self reference issue

Evaluating comprehension sources was done using
"yield" mode. This caused sets to be evaluated out
of band, causing structs to be closed before fields
can be added.

We now, instead, rely on the recursive call-by-need
behavior in combination with the freezing mechanism.
In the future we plan to move to a scheduler that
relies fully on either call by need or scheduling, but
for now this will have to do.

comprehension.txtar: fixes a P0 bug

freeze.txtar: fixes the target issue.

It would also fix the comprehension.t1.ok tests if
we use "attemptOnly" instead of "finalize". But this
breaks issue 3616. It is either way unclear what the
correct behavior should be and V2 also errors here,
so it is not the worst.

This change would also fix a possible issue with
propagating up an incomplete error. Also here the
behavior is the same as V2, so no problem for now.

adt.txtar: the changes are benign and due to reordering.

Fixes #3178

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

Author: mpvl

cue/testdata/cycle/comprehension.txtar

@@ -310,23 +310,19 @@ issue2367: {
 }
 
 -- out/evalalpha/stats --
-Leaks:  778
+Leaks:  782
 Freed:  63
 Reused: 63
-Allocs: 778
+Allocs: 782
 Retain: 0
 
-Unifications: 498
-Conjuncts:    2977
+Unifications: 502
+Conjuncts:    3031
 Disjuncts:    196
 -- out/evalalpha --
 Errors:
 selfReferential.insertionError.A: adding field foo3 not allowed as field set was already referenced:
     ./in.cue:117:14
-siblingInsertion.t2.p1.deployment: adding field logging not allowed as field set was already referenced:
-    ./in.cue:277:21
-siblingInsertion.t2.p2.deployment: adding field logging not allowed as field set was already referenced:
-    ./in.cue:292:21
 
 Result:
 (_|_){
@@ -651,8 +647,7 @@ Result:
       }
     }
   }
-  siblingInsertion: (_|_){
-    // [eval]
+  siblingInsertion: (struct){
     t1: (struct){
       p1: (struct){
         D: (struct){
@@ -685,28 +680,35 @@ Result:
         }
       }
     }
-    t2: (_|_){
-      // [eval]
-      p1: (_|_){
-        // [eval] siblingInsertion.t2.p1.deployment: adding field logging not allowed as field set was already referenced:
-        //     ./in.cue:277:21
+    t2: (struct){
+      p1: (struct){
         D: (struct){
           logging: (_){ _ }
         }
-        deployment: (_|_){
-          // [eval] siblingInsertion.t2.p1.deployment: adding field logging not allowed as field set was already referenced:
-          //     ./in.cue:277:21
+        deployment: (struct){
+          logging: (struct){
+            env2: (struct){
+              ENV: (string){ "True" }
+            }
+            env: (struct){
+              ENV: (string){ "True" }
+            }
+          }
         }
       }
-      p2: (_|_){
-        // [eval] siblingInsertion.t2.p2.deployment: adding field logging not allowed as field set was already referenced:
-        //     ./in.cue:292:21
+      p2: (struct){
         D: (struct){
           logging: (_){ _ }
         }
-        deployment: (_|_){
-          // [eval] siblingInsertion.t2.p2.deployment: adding field logging not allowed as field set was already referenced:
-          //     ./in.cue:292:21
+        deployment: (struct){
+          logging: (struct){
+            env2: (struct){
+              ENV: (string){ "True" }
+            }
+            env: (struct){
+              ENV: (string){ "True" }
+            }
+          }
         }
       }
     }
@@ -719,19 +721,15 @@ Result:
 diff old new
 --- old
 +++ new
-@@ -1,5 +1,10 @@
+@@ -1,5 +1,6 @@
  Errors:
 -selfReferential.insertionError.A: field foo3 not allowed by earlier comprehension or reference cycle
 +selfReferential.insertionError.A: adding field foo3 not allowed as field set was already referenced:
 +    ./in.cue:117:14
-+siblingInsertion.t2.p1.deployment: adding field logging not allowed as field set was already referenced:
-+    ./in.cue:277:21
-+siblingInsertion.t2.p2.deployment: adding field logging not allowed as field set was already referenced:
-+    ./in.cue:292:21
 
  Result:
  (_|_){
-@@ -19,7 +24,9 @@
+@@ -19,7 +20,9 @@
    B: (struct){
      a: (struct){
        parent: (string){ "" }
@@ -742,7 +740,7 @@ diff old new
        }
      }
    }
-@@ -52,17 +59,12 @@
+@@ -52,17 +55,12 @@
        }
      }
      _e: (#struct){
@@ -766,7 +764,7 @@ diff old new
        }
      }
      e: (#struct){
-@@ -76,12 +78,7 @@
+@@ -76,12 +74,7 @@
        }
        f2: (#struct){
          a: (#struct){
@@ -780,7 +778,7 @@ diff old new
          }
        }
      }
-@@ -127,9 +124,9 @@
+@@ -127,9 +120,9 @@
      insertionError: (_|_){
        // [eval]
        A: (_|_){
@@ -792,7 +790,7 @@ diff old new
        }
      }
      acrossOr: (struct){
-@@ -254,27 +251,31 @@
+@@ -254,27 +247,31 @@
    issue1881: (struct){
      p1: (struct){
        o: (#struct){
@@ -845,7 +843,7 @@ diff old new
          }) }
      }
      p2: (struct){
-@@ -290,7 +291,7 @@
+@@ -290,7 +287,7 @@
          }
        }
        o: (#struct){
@@ -854,90 +852,15 @@ diff old new
            output: (#struct){
              reject: (string){ "ok" }
            }
-@@ -320,15 +321,16 @@
+@@ -320,7 +317,7 @@
            resource: (string){ string }
          }) }
        o: (#struct){
 -        retry: (#struct){
--          output: (#struct){
--            reject: (string){ "ok" }
--          }
--        }
--      }
--    }
--  }
--  siblingInsertion: (struct){
 +        retry: (struct){
-+          output: (#struct){
-+            reject: (string){ "ok" }
-+          }
-+        }
-+      }
-+    }
-+  }
-+  siblingInsertion: (_|_){
-+    // [eval]
-     t1: (struct){
-       p1: (struct){
-         D: (struct){
-@@ -361,35 +363,28 @@
-         }
-       }
-     }
--    t2: (struct){
--      p1: (struct){
--        D: (struct){
--          logging: (_){ _ }
--        }
--        deployment: (struct){
--          logging: (struct){
--            env2: (struct){
--              ENV: (string){ "True" }
--            }
--            env: (struct){
--              ENV: (string){ "True" }
--            }
--          }
--        }
--      }
--      p2: (struct){
--        D: (struct){
--          logging: (_){ _ }
--        }
--        deployment: (struct){
--          logging: (struct){
--            env2: (struct){
--              ENV: (string){ "True" }
--            }
--            env: (struct){
--              ENV: (string){ "True" }
--            }
--          }
-+    t2: (_|_){
-+      // [eval]
-+      p1: (_|_){
-+        // [eval] siblingInsertion.t2.p1.deployment: adding field logging not allowed as field set was already referenced:
-+        //     ./in.cue:277:21
-+        D: (struct){
-+          logging: (_){ _ }
-+        }
-+        deployment: (_|_){
-+          // [eval] siblingInsertion.t2.p1.deployment: adding field logging not allowed as field set was already referenced:
-+          //     ./in.cue:277:21
-+        }
-+      }
-+      p2: (_|_){
-+        // [eval] siblingInsertion.t2.p2.deployment: adding field logging not allowed as field set was already referenced:
-+        //     ./in.cue:292:21
-+        D: (struct){
-+          logging: (_){ _ }
-+        }
-+        deployment: (_|_){
-+          // [eval] siblingInsertion.t2.p2.deployment: adding field logging not allowed as field set was already referenced:
-+          //     ./in.cue:292:21
-         }
-       }
-     }
+           output: (#struct){
+             reject: (string){ "ok" }
+           }
 -- diff/-out/evalalpha/stats<==>+out/eval/stats --
 diff old new
 --- old
@@ -948,17 +871,17 @@ diff old new
 -Reused: 1260
 -Allocs: 60
 -Retain: 145
-+Leaks:  778
++Leaks:  782
 +Freed:  63
 +Reused: 63
-+Allocs: 778
++Allocs: 782
 +Retain: 0
 
 -Unifications: 832
 -Conjuncts:    2525
 -Disjuncts:    1404
-+Unifications: 498
-+Conjuncts:    2977
++Unifications: 502
++Conjuncts:    3031
 +Disjuncts:    196
 -- out/eval/stats --
 Leaks:  50
@@ -972,8 +895,6 @@ Conjuncts:    2525
 Disjuncts:    1404
 -- diff/explanation --
 B.a.children: now correctly marked as incomplete
--- diff/todo/p0 --
-siblingInsertion.t2: errors
 -- diff/todo/p2 --
 issue1881.#AllOutputs.retry: additional entry.
 Might be error in practice and okay.

cue/testdata/cycle/freeze.txtar

@@ -158,8 +158,6 @@ comprehension.t3.err.a: adding field xq not allowed as field set was already ref
     ./comprehension.cue:82:13
 comprehension.moreSpecific.err.a.x: adding field z not allowed as field set was already referenced:
     ./comprehension.cue:114:9
-issue3178.output: adding field Value not allowed as field set was already referenced:
-    ./issue3178.cue:9:23
 
 Result:
 (_|_){
@@ -295,23 +293,20 @@ Result:
       }
     }
   }
-  issue3178: (_|_){
-    // [eval] issue3178.output: adding field Value not allowed as field set was already referenced:
-    //     ./issue3178.cue:9:23
+  issue3178: (struct){
     input: (#list){
       0: (string){ "Value" }
     }
-    output: (_|_){
-      // [eval] issue3178.output: adding field Value not allowed as field set was already referenced:
-      //     ./issue3178.cue:9:23
+    output: (struct){
+      Value: (bool){ true }
     }
   }
 }
 -- diff/-out/evalalpha<==>+out/eval --
 diff old new
 --- old
 +++ new
-@@ -1,11 +1,12 @@
+@@ -1,11 +1,10 @@
  Errors:
 -comprehension.moreSpecific.err.a: field z not allowed by earlier comprehension or reference cycle
 -comprehension.t1.ok.p0.x: field z not allowed by earlier comprehension or reference cycle
@@ -326,12 +321,10 @@ diff old new
 +    ./comprehension.cue:82:13
 +comprehension.moreSpecific.err.a.x: adding field z not allowed as field set was already referenced:
 +    ./comprehension.cue:114:9
-+issue3178.output: adding field Value not allowed as field set was already referenced:
-+    ./issue3178.cue:9:23
 
  Result:
  (_|_){
-@@ -12,73 +13,65 @@
+@@ -12,73 +11,65 @@
    // [eval]
    comprehension: (_|_){
      // [eval]
@@ -455,7 +448,7 @@ diff old new
              }
            }
          }
-@@ -89,9 +82,9 @@
+@@ -89,9 +80,9 @@
        err: (_|_){
          // [eval]
          a: (_|_){
@@ -468,7 +461,7 @@ diff old new
          }
        }
      }
-@@ -99,16 +92,14 @@
+@@ -99,16 +90,14 @@
        // [eval]
        err: (_|_){
          // [eval]
@@ -491,7 +484,7 @@ diff old new
          }
        }
      }
-@@ -132,10 +123,10 @@
+@@ -132,10 +121,10 @@
        err: (_|_){
          // [eval]
          a: (_|_){
@@ -504,27 +497,6 @@ diff old new
            }
          }
        }
-@@ -151,12 +142,15 @@
-       }
-     }
-   }
--  issue3178: (struct){
-+  issue3178: (_|_){
-+    // [eval] issue3178.output: adding field Value not allowed as field set was already referenced:
-+    //     ./issue3178.cue:9:23
-     input: (#list){
-       0: (string){ "Value" }
-     }
--    output: (struct){
--      Value: (bool){ true }
-+    output: (_|_){
-+      // [eval] issue3178.output: adding field Value not allowed as field set was already referenced:
-+      //     ./issue3178.cue:9:23
-     }
-   }
- }
--- diff/todo/p3 --
-Error path location could improve.
 -- diff/explanation --
 v0.7 fixes bugs in v0.6:
 - t1 should be incomplete error, as it is fixable.

internal/core/adt/expr.go

@@ -2026,7 +2026,13 @@ func (c *OpContext) forSource(x Expr) *Vertex {
 
 	node, ok := v.(*Vertex)
 	if ok && c.isDevVersion() {
-		node.unify(c, state.conditions(), yield)
+		// We do not request to "yield" here, but rather rely on the
+		// call-by-need behavior in combination with the freezing mechanism.
+		// TODO: this seems a bit fragile. At some point we need to make this
+		// more robust by moving to a pure call-by-need mechanism, for instance.
+		// TODO: using attemptOnly here will remove the cyclic reference error
+		// of comprehension.t1.ok (which also errors in V2),
+		node.unify(c, state.conditions(), finalize)
 	}
 
 	v, ok = c.getDefault(v)
@@ -2064,7 +2070,7 @@ func (c *OpContext) forSource(x Expr) *Vertex {
 		}
 
 	default:
-		if kind := v.Kind(); kind&StructKind != 0 {
+		if kind := v.Kind(); kind&(StructKind|ListKind) != 0 {
 			c.addErrf(IncompleteError, pos(x),
 				"cannot range over %s (incomplete type %s)", x, kind)
 			return emptyNode
@@ -2076,6 +2082,17 @@ func (c *OpContext) forSource(x Expr) *Vertex {
 			return emptyNode
 		}
 	}
+	if c.isDevVersion() {
+		kind := v.Kind()
+		// At this point it is possible that the Vertex represents an incomplete
+		// struct or list, which is the case if it may be struct or list, but
+		// is also at least some other type, such as is the case with top.
+		if kind&(StructKind|ListKind) != 0 && kind != StructKind && kind != ListKind {
+			c.addErrf(IncompleteError, pos(x),
+				"cannot range over %s (incomplete type %s)", x, kind)
+			return emptyNode
+		}
+	}
 
 	return node
 }

internal/core/export/testdata/main/adt.txtar

@@ -253,6 +253,107 @@ errorListDef: {
 }
 -- diff/doc/explanation --
 Reordering.
+-- out/doc-v3 --
+[]
+[p1]
+[d1]
+[d1 foobar]
+[bar]
+[d2]
+[d2 foobar]
+[d2 foobar name]
+[d2 foobar foo]
+[a]
+- Issue #1910
+
+[comp]
+[comp bar]
+[bytes]
+[c1]
+[s1]
+[l1]
+[l1 0]
+[l2]
+[l3]
+[l4]
+[l4 0]
+[l4 1]
+[l5]
+[l5 #foo]
+[l5 0]
+[l5 1]
+[#foo]
+[l6]
+[l6 #foo]
+[l6 0]
+[l6 1]
+[n1]
+[n10]
+[t]
+- t is true
+
+[e1]
+[e2]
+[e3]
+[e4]
+[e5]
+[e6]
+[e7]
+[e8]
+[m1]
+[m1 foo]
+- foo is an optional field
+
+[m1 bar]
+- bar is a field
+
+[m1 baz]
+- baz is a required field.
+
+[x]
+[y1]
+[y1 src]
+[y1 src 0]
+[y1 src 1]
+[y1 src 2]
+[y1 x]
+[y1 x 0]
+[y1 x 1]
+[y1 x 2]
+[y1 foo0]
+[y1 foo1]
+[y1 foo2]
+[y1 bar1]
+[y1 bar2]
+[preserveKeyFieldInComprehension]
+[errorStructDef]
+[errorStructDef a]
+[errorStructDef b]
+[errorStructDef #Def]
+[errorList]
+[errorList 0]
+[errorList 1]
+[errorListDef]
+[errorListDef #Def]
+[errorListDef 0]
+[errorListDef 1]
+-- diff/-out/doc-v3<==>+out/doc --
+diff old new
+--- old
++++ new
+@@ -65,10 +65,10 @@
+ [y1 x 1]
+ [y1 x 2]
+ [y1 foo0]
+-[y1 bar1]
+-[y1 bar2]
+ [y1 foo1]
+ [y1 foo2]
++[y1 bar1]
++[y1 bar2]
+ [preserveKeyFieldInComprehension]
+ [errorStructDef]
+ [errorStructDef a]
 -- out/doc --
 []
 [p1]
@@ -339,6 +440,123 @@ Reordering.
 [errorListDef 1]
 -- diff/value/todo/p3 --
 Error message change.
+-- out/value-v3 --
+== Simplified
+_|_ // e3: index out of range [2] with length 2
+== Raw
+_|_ // e3: index out of range [2] with length 2
+== Final
+_|_ // e3: index out of range [2] with length 2
+== All
+{
+	@foo(bar)
+	p1: {}
+	d1: {
+		foobar: int
+	}
+	bar: "bar"
+	d2: {
+		foobar: {
+			name: "xx"
+			foo:  "xx"
+		}
+	}
+
+	// Issue #1910
+	a: _
+	comp: {
+		for k, v in [0]
+		let w = v {
+			"\(a)": w
+			"bar":  w
+		}
+	}
+	bytes: '\xeb \x1a\xf5\xaa\xf0\xd6\x06)'
+	c1:    true
+	s1: """
+		multi
+		bar
+		line
+		"""
+	l1: [3, ...int]
+	l2: [...int]
+	l3: []
+	l4: [1, 2]
+	l5: {
+		#foo: int
+		[1, 3]
+	}
+	#foo: int
+	l6: {
+		#foo: int
+		[1, 3]
+	}
+	n1:  1.0
+	n10: 10
+
+	// t is true
+	t:   true
+	e1:  <1.0
+	e2:  >1.0 & <10
+	e3:  _|_ // e3: index out of range [2] with length 2
+	e4:  _|_ // e4: index 3 out of range
+	e5:  _|_ // e3: index out of range [2] with length 2
+	e6:  false
+	e7:  true
+	e8?: true
+	m1: {
+		// foo is an optional field
+		foo?: 3
+
+		// bar is a field
+		bar: 4
+
+		// baz is a required field.
+		baz!: 5
+	}
+	y1: {
+		src: [1, 2, 3]
+		foo0: 1
+		foo1: 2
+		foo2: 3
+		bar1: 2
+		x: [1, 2, 3]
+		bar2: 3
+	}
+	preserveKeyFieldInComprehension: 1
+	errorStructDef: {
+		a:    1
+		b:    _|_ // errorStructDef.b: conflicting values 2 and 1
+		#Def: 1
+	}
+	errorList: [1, _|_]
+	x: int
+	errorListDef: {
+		#Def: 1
+		[1, _|_]
+	}
+}
+== Eval
+_|_ // e3: index out of range [2] with length 2
+-- diff/-out/value-v3<==>+out/value --
+diff old new
+--- old
++++ new
+@@ -74,11 +74,11 @@
+ 	y1: {
+ 		src: [1, 2, 3]
+ 		foo0: 1
+-		bar1: 2
+-		bar2: 3
+ 		foo1: 2
+-		x: [1, 2, 3]
+ 		foo2: 3
++		bar1: 2
++		x: [1, 2, 3]
++		bar2: 3
+ 	}
+ 	preserveKeyFieldInComprehension: 1
+ 	errorStructDef: {
 -- out/value --
 == Simplified
 _|_ // e3: index out of range [2] with length 2