internal/core/eval: tweak to disjunction

Except for a small bug, that is also fixed in this CL, the
new evaluator implemented disjunctions according to
spec before this CL. The old evaluator, however, didn't...

It turns out the semantics of the spec is somewhat awkward,
though theoretically nicer. It seems like we do need to change
the definition somewhat to make it less awkward, or at least
come up with a good workaround before adopting the spec.

This CL introduces a hack that mimic the old behavior for scalar
values, where this is the most relevant.

Change-Id: If58d27b4f4e9aa1a5ae8316cb774cf6b26c70796
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/6655
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>

cue/testdata/choosedefault/002_associativity_of_defaults.txtar

@@ -8,6 +8,13 @@ b: (*"a" | "b") | "c"
 c: *"a" | (*"b" | "c")
 x: a & b
 y: b & c
+
+s1: *1 | ((*2|3) & (2|*3))
+s2: *1 | ((*2|3) & (*2|3))
+s3: *1 | ((*2|3) & 3)
+s4: *1 | ((*2|3) & 2)
+s5: *1 | *(*2 | 3)
+
 -- out/def --
 x: a & b
 y: b & c
@@ -24,6 +31,11 @@ c: *"a" | *"b" | "c"
   c: (*"a"|(*"b"|"c"))
   x: (〈0;a〉 & 〈0;b〉)
   y: (〈0;b〉 & 〈0;c〉)
+  s1: (*1|((*2|3) & (2|*3)))
+  s2: (*1|((*2|3) & (*2|3)))
+  s3: (*1|((*2|3) & 3))
+  s4: (*1|((*2|3) & 2))
+  s5: (*1|*(*2|3))
 }
 -- out/eval --
 (struct){
@@ -32,4 +44,9 @@ c: *"a" | *"b" | "c"
   c: (string){ |(*(string){ "a" }, *(string){ "b" }, (string){ "c" }) }
   x: (string){ |(*(string){ "a" }, (string){ "b" }, (string){ "c" }) }
   y: (string){ |(*(string){ "a" }, (string){ "b" }, (string){ "c" }) }
+  s1: (int){ |(*(int){ 1 }, *(int){ 2 }, (int){ 3 }) }
+  s2: (int){ |(*(int){ 1 }, *(int){ 2 }, (int){ 3 }) }
+  s3: (int){ |(*(int){ 1 }, (int){ 3 }) }
+  s4: (int){ |(*(int){ 1 }, (int){ 2 }) }
+  s5: (int){ |(*(int){ 1 }, *(int){ 2 }, (int){ 3 }) }
 }

cue/testdata/cycle/issue429.txtar

@@ -73,7 +73,7 @@ Result:
   s1: (#struct){
     res: (int){ |(*(int){ 0 }, (int){ &(>=0, int) }) }
     min: (int){ 5 }
-    max: (number){ |((int){ 5 }, (number){ >5 }) }
+    max: (number){ |(*(int){ 5 }, (number){ >5 }) }
   }
   s2: (#struct){
     res: (int){ |(*(int){ 0 }, (int){ &(>=0, int) }) }

cue/testdata/disjunctions/specdeviation.txtar

@@ -0,0 +1,68 @@
+It turns out the semantics of the spec is somewhat awkward,
+though theoretically nicer. It seems like we do need to change
+the definition somewhat to make it less awkward, or at least
+come up with a good workaround before adopting the spec.
+
+We have introduce a small hack to mimic the old behavior for scalar
+values.
+
+Note that the value of p below is now 2 (default), but should
+be the non-concrete 2 | int.
+
+Proof:
+    p: *((*1 | int) & 2)  | int   // substitution of both P conjuncts in p
+    p: *(*_|_ | 2)  | int         // U1: distribute conjuncts
+    p: *_|_ | 2  | int            // M2: remove mark
+    p: 2  | int                   // value after removing default.
+
+-- in.cue --
+
+Q: *1 | int
+q: *Q | int // 1 as expected
+
+P: *1 | int
+P: 2
+p: *P | int // now 2, but should be (2 | int), according to the spec:
+
+
+s1: #Size & { min: 5 }
+
+#Size : {
+    max: >min | *min
+    res: uint | * 0
+    min: >res | *(1 + res)
+}
+-- out/eval --
+(struct){
+  Q: (int){ |(*(int){ 1 }, (int){ int }) }
+  q: (int){ |(*(int){ 1 }, (int){ int }) }
+  P: (int){ 2 }
+  p: (int){ |(*(int){ 2 }, (int){ int }) }
+  s1: (#struct){
+    max: (number){ |(*(int){ 5 }, (number){ >5 }) }
+    res: (int){ 0 }
+    min: (int){ 5 }
+  }
+  #Size: (#struct){
+    max: (number){ |(*(int){ 1 }, (number){ >0 }, (number){ >1 }) }
+    res: (int){ 0 }
+    min: (number){ |(*(int){ 1 }, (number){ >0 }) }
+  }
+}
+-- out/compile --
+--- in.cue
+{
+  Q: (*1|int)
+  q: (*〈0;Q〉|int)
+  P: (*1|int)
+  P: 2
+  p: (*〈0;P〉|int)
+  s1: (〈0;#Size〉 & {
+    min: 5
+  })
+  #Size: {
+    max: (>〈0;min〉|*〈0;min〉)
+    res: (&(int, >=0)|*0)
+    min: (>〈0;res〉|*(1 + 〈0;res〉))
+  }
+}

internal/core/eval/disjunct.go

@@ -240,23 +240,40 @@ func (n *nodeContext) insertDisjuncts() (inserted bool) {
 			break
 		}
 
-		subMode := []defaultMode{}
+		subMode := maybeDefault
 		for ; sub < len(n.disjunctions); sub++ {
 			d := n.disjunctions[sub]
+
+			// TODO: HACK ALERT: we ignore the default tags of the subexpression
+			// if we already have a scalar value and can no longer change the
+			// outcome.
+			// This is not conform the spec, but mimics the old implementation.
+			// It also results in nicer default semantics. Changing this will
+			// break existing CUE code in awkward ways.
+			// We probably should address this when we figure out how to change
+			// the spec to accommodate for this. For instance, we could say
+			// that if a disjunction only contributes a single disjunct to an
+			// end result, default information is ignored. Not the greatest
+			// definition, though.
+			// Another alternative might be to have a special builtin that
+			// mimics the good behavior.
+			// Note that the same result can be obtained in CUE by adding
+			// 0 to a referenced number (forces the default to be discarded).
+			wasScalar := n.scalar != nil // Hack line 1
+
 			disjunctions = append(disjunctions, d)
 			mode, ok := n.insertSingleDisjunct(p, d, true)
 			p++
 			if !ok {
 				inserted = false
 				break
 			}
-			subMode = append(subMode, mode)
-		}
-		for i := len(subMode) - 1; i >= 0; i-- {
-			defMode = combineSubDefault(defMode, subMode[i])
-		}
 
-		// fmt.Println("RESMODE", defMode, combineDefault(n.defaultMode, defMode))
+			if !wasScalar { // Hack line 2.
+				subMode = combineDefault(subMode, mode)
+			}
+		}
+		defMode = combineSubDefault(defMode, subMode)
 
 		n.defaultMode = combineDefault(n.defaultMode, defMode)
 	}
@@ -308,11 +325,14 @@ func (n *nodeContext) insertSingleDisjunct(p int, d envDisjunct, isSub bool) (mo
 //
 // M1: *v        => (v, v)
 // M2: *(v1, d1) => (v1, d1)
-// or
-// M2: *(v1, d1) => (v1, v1)
-// or
-// M2: *(v1, d1) => v1 if d1 == _|_
-// M2:              d1 otherwise
+//
+// NOTE: M2 cannot be *(v1, d1) => (v1, v1), as this has the weird property
+// of making a value less specific. This causes issues, for instance, when
+// trimming.
+//
+// The old implementation does something similar though. It will discard
+// default information after first determining if more than one conjunct
+// has survived.
 //
 // def + maybe -> def
 // not + maybe -> def
@@ -326,31 +346,49 @@ const (
 	isDefault
 )
 
+// combineSubDefault combines default modes where b is a subexpression in
+// a disjunctions.
+//
+// Default rules from spec:
+//
+// D1: (v1, d1) | v2       => (v1|v2, d1)
+// D2: (v1, d1) | (v2, d2) => (v1|v2, d1|d2)
+//
+// Spec:
+// M1: *v        => (v, v)
+// M2: *(v1, d1) => (v1, d1)
+//
 func combineSubDefault(a, b defaultMode) defaultMode {
 	switch {
-	case a == maybeDefault && b == maybeDefault:
+	case a == maybeDefault && b == maybeDefault: // D1
 		return maybeDefault
-	case a == maybeDefault && b == notDefault:
+	case a == maybeDefault && b == notDefault: // D1
 		return notDefault
-	case a == maybeDefault && b == isDefault:
+	case a == maybeDefault && b == isDefault: // D1
 		return isDefault
-	case a == notDefault && b == maybeDefault:
+	case a == notDefault && b == maybeDefault: // D1
 		return notDefault
-	case a == notDefault && b == notDefault:
+	case a == notDefault && b == notDefault: // D2
 		return notDefault
-	case a == notDefault && b == isDefault:
+	case a == notDefault && b == isDefault: // D2
 		return isDefault
-	case a == isDefault && b == maybeDefault:
+	case a == isDefault && b == maybeDefault: // D1
 		return isDefault
-	case a == isDefault && b == notDefault:
+	case a == isDefault && b == notDefault: // M2
 		return notDefault
-	case a == isDefault && b == isDefault:
+	case a == isDefault && b == isDefault: // D2
 		return isDefault
 	default:
 		panic("unreachable")
 	}
 }
 
+// combineDefaults combines default modes for unifying conjuncts.
+//
+// Default rules from spec:
+//
+// U1: (v1, d1) & v2       => (v1&v2, d1&v2)
+// U2: (v1, d1) & (v2, d2) => (v1&v2, d1&d2)
 func combineDefault(a, b defaultMode) defaultMode {
 	if a > b {
 		a, b = b, a