internal/core/adt: delay dereferencing in lookup

This fixes a bug in the API, where dereferencing caused Value.Dereference to be too eager. This changes test in elimination.txtar. The results are a bit better, but still wrong, so it seems. So no changes in the todos. The main change is that Vertex.lookup no longer fully dereferences. Dereferencing is now done in a bespoke manner at the call sites of lookup. Issue #3060 Issue #2884 Signed-off-by: Marcel van Lohuizen <mpvl@gmail.com> Change-Id: I59f791e78213c3cdacdb8ad3b6ba829a73d2d6ca Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1194104 Reviewed-by: Daniel Martí <mvdan@mvdan.cc> TryBot-Result: CUEcueckoo <cueckoo@cuelang.org> Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
cue-lang · May 1, 2024 · 7dd29f1 · 7dd29f1
1 parent 8935df9
commit 7dd29f1
Show file tree

Hide file tree

Showing 9 changed files with 135 additions and 82 deletions.
diff --git a/cue/testdata/cycle/chain.txtar b/cue/testdata/cycle/chain.txtar
@@ -208,21 +208,21 @@ issue2052: full: {
 	d: #Depth & {#in: tree}
 }
 -- out/evalalpha/stats --
-Leaks:  22127
-Freed:  1994
-Reused: 1993
-Allocs: 22128
+Leaks:  19985
+Freed:  1462
+Reused: 1461
+Allocs: 19986
 Retain: 0
 
-Unifications: 8358
-Conjuncts:    121910
-Disjuncts:    15463
+Unifications: 7496
+Conjuncts:    107561
+Disjuncts:    13651
 -- out/evalalpha --
 Errors:
 issue2052.t1.#Depth: adding field #basic not allowed as field set was already referenced:
     ./issue2052.cue:8:20
 issue2052.t2.#Depth: adding field #basic not allowed as field set was already referenced:
-    ./issue2052.cue:46:14
+    ./issue2052.cue:41:20
 
 Result:
 (_|_){
@@ -350,7 +350,7 @@ Result:
       // [eval]
       #Depth: (_|_){
         // [eval] issue2052.t2.#Depth: adding field #basic not allowed as field set was already referenced:
-        //     ./issue2052.cue:46:14
+        //     ./issue2052.cue:41:20
         #maxiter: (int){ 4 }
         #funcs: (#struct){
           "4": (null){ null }
@@ -725,18 +725,18 @@ diff old new
 -Reused: 1801
 -Allocs: 84
 -Retain: 185
-+Leaks:  22127
-+Freed:  1994
-+Reused: 1993
-+Allocs: 22128
++Leaks:  19985
++Freed:  1462
++Reused: 1461
++Allocs: 19986
 +Retain: 0
 
 -Unifications: 800
 -Conjuncts:    3175
 -Disjuncts:    1974
-+Unifications: 8358
-+Conjuncts:    121910
-+Disjuncts:    15463
++Unifications: 7496
++Conjuncts:    107561
++Disjuncts:    13651
 -- diff/-out/evalalpha<==>+out/eval --
 diff old new
 --- old
@@ -747,7 +747,7 @@ diff old new
 +issue2052.t1.#Depth: adding field #basic not allowed as field set was already referenced:
 +    ./issue2052.cue:8:20
 +issue2052.t2.#Depth: adding field #basic not allowed as field set was already referenced:
-+    ./issue2052.cue:46:14
++    ./issue2052.cue:41:20
 +
 +Result:
 +(_|_){
@@ -969,7 +969,7 @@ diff old new
 +      // [eval]
 +      #Depth: (_|_){
 +        // [eval] issue2052.t2.#Depth: adding field #basic not allowed as field set was already referenced:
-+        //     ./issue2052.cue:46:14
++        //     ./issue2052.cue:41:20
 +        #maxiter: (int){ 4 }
 +        #funcs: (#struct){
 +          "4": (null){ null }

diff --git a/cue/testdata/cycle/comprehension.txtar b/cue/testdata/cycle/comprehension.txtar
@@ -310,15 +310,15 @@ issue2367: {
 }
 
 -- out/evalalpha/stats --
-Leaks:  451
+Leaks:  431
 Freed:  12
 Reused: 12
-Allocs: 451
+Allocs: 431
 Retain: 0
 
-Unifications: 375
-Conjuncts:    2094
-Disjuncts:    58
+Unifications: 397
+Conjuncts:    2202
+Disjuncts:    36
 -- out/evalalpha --
 Errors:
 issue1881.p1.o.retry: field not allowed:
@@ -1482,18 +1482,18 @@ diff old new
 -Reused: 1260
 -Allocs: 60
 -Retain: 145
-+Leaks:  451
++Leaks:  431
 +Freed:  12
 +Reused: 12
-+Allocs: 451
++Allocs: 431
 +Retain: 0
 
 -Unifications: 832
 -Conjuncts:    2525
 -Disjuncts:    1404
-+Unifications: 375
-+Conjuncts:    2094
-+Disjuncts:    58
++Unifications: 397
++Conjuncts:    2202
++Disjuncts:    36
 -- out/eval/stats --
 Leaks:  50
 Freed:  1270

diff --git a/cue/testdata/disjunctions/elimination.txtar b/cue/testdata/disjunctions/elimination.txtar
@@ -979,22 +979,37 @@ issue2263: full: {
         }
         BAZ: (_){ _ }
       }
-      t3: (struct){
-        #A: (#struct){
-          v: (int){ 1 }
-        }
-        BAZ: (_){ _ }
-        S: (#struct){
-          x: (int){ 1 }
-          y: (int){ 1 }
-        }
-        #B: (#struct){
-          x: (int){ 1 }
-          y: (int){ 1 }
-        }
-        f1: (int){ int }
-        f2: (int){ int }
-      }
+      t3: (struct){ |((struct){
+          #A: (#struct){
+            v: (int){ 1 }
+          }
+          BAZ: (_){ _ }
+          S: (#struct){
+            x: (int){ 1 }
+            y: (int){ 1 }
+          }
+          #B: (#struct){
+            x: (int){ 1 }
+            y: (int){ 1 }
+          }
+          f1: (int){ int }
+          f2: (int){ int }
+        }, (struct){
+          #A: (#struct){
+            v: (int){ 1 }
+          }
+          BAZ: (_){ _ }
+          S: (#struct){
+            x: (int){ 1 }
+            y: (int){ 1 }
+          }
+          #B: (#struct){
+            x: (int){ 1 }
+            y: (int){ 1 }
+          }
+          f1: (int){ int }
+          b2: (int){ int }
+        }) }
     }
     full: (struct){
       Foo: (#struct){
@@ -1646,7 +1661,7 @@ diff old new
        t1: (struct){
          #SpecFoo: (#struct){
            foo: (#struct){
-@@ -508,185 +484,103 @@
+@@ -508,185 +484,118 @@
              x: (int){ 1 }
            }
          }
@@ -1669,16 +1684,9 @@ diff old new
 -      t3: (_|_){
 -        // [eval] issue2209.simplified.t3.BAZ: undefined field: y:
 -        //     ./issue2209full.cue:35:9
-+        spec: (struct){
-+          bar: (struct){
-+          }
-+        }
-+        BAZ: (_){ _ }
-+      }
-+      t3: (struct){
-         #A: (#struct){
-           v: (int){ 1 }
-         }
+-        #A: (#struct){
+-          v: (int){ 1 }
+-        }
 -        BAZ: (_|_){
 -          // [eval] issue2209.simplified.t3.BAZ: undefined field: y:
 -          //     ./issue2209full.cue:35:9
@@ -1690,23 +1698,52 @@ diff old new
 -            x: (int){ 1 }
 -            y: (int){ 1 }
 -          }) }
-+        BAZ: (_){ _ }
-+        S: (#struct){
-+          x: (int){ 1 }
-+          y: (int){ 1 }
-+        }
-         #B: (#struct){
-           x: (int){ 1 }
-           y: (int){ 1 }
-         }
+-        #B: (#struct){
+-          x: (int){ 1 }
+-          y: (int){ 1 }
+-        }
 -        b2: (int){ int }
 -      }
 -    }
 -    full: (_|_){
 -      // [eval]
-+        f1: (int){ int }
-+        f2: (int){ int }
++        spec: (struct){
++          bar: (struct){
++          }
++        }
++        BAZ: (_){ _ }
 +      }
++      t3: (struct){ |((struct){
++          #A: (#struct){
++            v: (int){ 1 }
++          }
++          BAZ: (_){ _ }
++          S: (#struct){
++            x: (int){ 1 }
++            y: (int){ 1 }
++          }
++          #B: (#struct){
++            x: (int){ 1 }
++            y: (int){ 1 }
++          }
++          f1: (int){ int }
++          f2: (int){ int }
++        }, (struct){
++          #A: (#struct){
++            v: (int){ 1 }
++          }
++          BAZ: (_){ _ }
++          S: (#struct){
++            x: (int){ 1 }
++            y: (int){ 1 }
++          }
++          #B: (#struct){
++            x: (int){ 1 }
++            y: (int){ 1 }
++          }
++          f1: (int){ int }
++          b2: (int){ int }
++        }) }
 +    }
 +    full: (struct){
        Foo: (#struct){
@@ -1921,7 +1958,7 @@ diff old new
          }
        }
        #Abstract: (#struct){
-@@ -702,34 +596,34 @@
+@@ -702,34 +611,34 @@
              }
            }) }
          resource: (#struct){
@@ -1984,7 +2021,7 @@ diff old new
          }
        }
        _#Spec: (#struct){ |(*(#struct){
-@@ -756,36 +650,36 @@
+@@ -756,36 +665,36 @@
          }
        }
        _Thing: (#struct){
@@ -2050,7 +2087,7 @@ diff old new
        }
        #Constrained: (#struct){
          spec: (#struct){ |(*(#struct){
-@@ -869,19 +763,19 @@
+@@ -869,19 +778,19 @@
            common: (int){ 3 }
          }
          #FormFoo: (#struct){
@@ -2077,7 +2114,7 @@ diff old new
            }) }
        }
        #Input: (#struct){
-@@ -930,10 +824,23 @@
+@@ -930,10 +839,23 @@
      }
      full: (struct){
        metrics: (#list){

diff --git a/cue/types.go b/cue/types.go
@@ -235,11 +235,6 @@ func (i *Iterator) Next() bool {
 	p := linkParent(i.val.parent_, i.val.v, arc)
 	i.f = arc.Label
 	i.arcType = arc.ArcType
-	// TODO(deref): this should not indirect disjunctions, as this will cause losing
-	// information, which is now available in the new evaluator. Find a safe
-	// way to preserve disjunction information while keeping backward
-	// compatibility in the API.
-	arc = arc.DerefValue()
 	i.cur = makeValue(i.val.idx, arc, p)
 	i.p++
 	return true

diff --git a/cue/types_test.go b/cue/types_test.go
@@ -3559,8 +3559,6 @@ func TestPathCorrection(t *testing.T) {
 			continue
 		}
 		runMatrix(t, "", func(t *testing.T, cfg *evalConfig) {
-			TODO_Sharing(t, cfg)
-
 			r := cfg.runtime()
 
 			inst, err := r.Compile("in", tc.input)

diff --git a/internal/core/adt/context.go b/internal/core/adt/context.go
@@ -710,6 +710,10 @@ func (c *OpContext) evalState(v Expr, state combinedFlags) (result Value) {
 		if arc == nil {
 			return nil
 		}
+		// TODO(deref): what is the right level of dereferencing here?
+		// DerefValue seems to work too.
+		arc = arc.DerefNonShared()
+
 		// TODO: consider moving this after markCycle, depending on how we
 		// implement markCycle, or whether we need it at all.
 		// TODO: is this indirect necessary?
@@ -838,6 +842,8 @@ func (c *OpContext) unifyNode(v Expr, state combinedFlags) (result Value) {
 		if v == nil {
 			return nil
 		}
+		v = v.DerefValue()
+
 		// TODO: consider moving this after markCycle, depending on how we
 		// implement markCycle, or whether we need it at all.
 		// TODO: is this indirect necessary?

diff --git a/internal/core/adt/share.go b/internal/core/adt/share.go
@@ -153,6 +153,17 @@ func (v *Vertex) DerefNonDisjunct() *Vertex {
 	}
 }
 
+// DerefNonRooted indirects a node that points to a value that is not rooted.
+func (v *Vertex) DerefNonRooted() *Vertex {
+	for {
+		arc, ok := v.BaseValue.(*Vertex)
+		if !ok || arc.IsDisjunct || v.IsShared {
+			return v
+		}
+		v = arc
+	}
+}
+
 // DerefNonShared finds the indirection of an arc that is not the result of
 // structure sharing. This is especially relevant when indirecting disjunction
 // values.

diff --git a/internal/core/adt/tasks.go b/internal/core/adt/tasks.go
@@ -101,6 +101,8 @@ func processResolver(ctx *OpContext, t *task, mode runMode) {
 		// TODO: yield instead?
 		return
 	}
+	arc = arc.DerefNonDisjunct()
+
 	ctx.Logf(t.node.node, "RESOLVED %v to %v %v", r, arc.Label, fmt.Sprintf("%p", arc))
 	// TODO: consider moving after markCycle or removing.
 	d := arc.DerefDisjunct()