core/adt: identify duplicate errors when pairwise-combining

Revert cue/errors/errors.go to before
https://review.gerrithub.io/c/cue-lang/cue/+/1199401

The previous attempt to fix this issue was extremely fragile: it only
worked if there was an attempt to append the exact same error
contiguously. In practice we have seen that this may not happen,
rendering that fix ineffective. So it is reverted here.

By contrast, in core/adt *OpContext.node(), it can be the case that the
call to unifyNode returns the opContext's own errors. The subsequent
call to *OpContext.AddBottom() and CombineErrors() can therefore result
in the duplication of the opContext's errors. Identifying and solving
this in CombineErrors() seems to be a robust solution.

Fixes #3307

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

cue/errors/errors.go

@@ -274,22 +274,14 @@ func (e *posError) Position() token.Pos         { return e.pos }
 
 // Append combines two errors, flattening Lists as necessary.
 func Append(a, b Error) Error {
-	switch a := a.(type) {
+	switch x := a.(type) {
 	case nil:
 		return b
 	case list:
-		return appendToList(a, b)
+		return appendToList(x, b)
 	}
-	switch b := b.(type) {
-	case nil:
-		return a
-	case list:
-		return appendToList(list{a}, b)
-	}
-	if a == b {
-		return a
-	}
-	return list{a, b}
+	// Preserve order of errors.
+	return appendToList(list{a}, b)
 }
 
 // Errors reports the individual errors associated with an error, which is

internal/core/adt/errors.go

@@ -176,17 +176,22 @@ func CombineErrors(src ast.Node, x, y Value) *Bottom {
 	a, _ := Unwrap(x).(*Bottom)
 	b, _ := Unwrap(y).(*Bottom)
 
-	if a == b && isCyclePlaceholder(a) {
-		return a
-	}
 	switch {
-	case a != nil && b != nil:
-	case a != nil:
-		return a
-	case b != nil:
-		return b
-	default:
+	case a == nil && b == nil:
 		return nil
+	case a == nil:
+		return b
+	case b == nil:
+		return a
+	case a == b && isCyclePlaceholder(a):
+		return a
+	case a == b:
+		// Don't return a (or b) because they may have other non-nil fields.
+		return &Bottom{
+			Src:  src,
+			Err:  a.Err,
+			Code: a.Code,
+		}
 	}
 
 	if a.Code != b.Code {

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

@@ -366,7 +366,7 @@ was known to compile and is known to be correct.
 		}
 		selfRefValue: {
 			struct: {
-				a: _|_ // valueAlias.selfRefValue.struct.a: incomplete list: _ (and 3 more errors)
+				a: _|_ // valueAlias.selfRefValue.struct.a: incomplete list: _ (and 1 more errors)
 			}
 			pattern: {
 				a: {}
@@ -516,7 +516,7 @@ diff old new
  		selfRefValue: {
  			struct: {
 -				a: _|_ // cycle error
-+				a: _|_ // valueAlias.selfRefValue.struct.a: incomplete list: _ (and 3 more errors)
++				a: _|_ // valueAlias.selfRefValue.struct.a: incomplete list: _ (and 1 more errors)
  			}
  			pattern: {
  				a: {}

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

@@ -41,7 +41,7 @@ additional: {
 [additional confs]
 -- diff/value/explanation --
 Benign change in error message.
--- out/value --
+-- out/value-v3 --
 == Simplified
 {
 	x: {
@@ -105,3 +105,80 @@ Benign change in error message.
 		}
 	}
 }
+-- diff/-out/value-v3<==>+out/value --
+diff old new
+--- old
++++ new
+@@ -32,7 +32,7 @@
+ 	s: strings.MinRunes(4) & strings.MaxRunes(7)
+ 	additional: {
+ 		env:   _
+-		confs: _|_ // additional.confs: incomplete bool: _ (and 1 more errors)
++		confs: _|_ // additional.confs: incomplete bool: _
+ 	}
+ }
+ == All
+-- out/value --
+== Simplified
+{
+	x: {
+		y: int64
+	}
+	s: strings.MinRunes(4) & strings.MaxRunes(7)
+	additional: {
+		env: _
+		confs: {
+			if env {}
+		}
+	}
+}
+== Raw
+{
+	x: {
+		y: >=-9223372036854775808 & <=9223372036854775807 & int
+	}
+	s: strings.MinRunes(4) & strings.MaxRunes(7)
+	additional: {
+		env: _
+		confs: {
+			if env {}
+		}
+	}
+}
+== Final
+{
+	x: {
+		y: int64
+	}
+	s: strings.MinRunes(4) & strings.MaxRunes(7)
+	additional: {
+		env:   _
+		confs: _|_ // additional.confs: incomplete bool: _ (and 1 more errors)
+	}
+}
+== All
+{
+	x: {
+		y: int64
+	}
+	s: strings.MinRunes(4) & strings.MaxRunes(7)
+	additional: {
+		env: _
+		confs: {
+			if env {}
+		}
+	}
+}
+== Eval
+{
+	x: {
+		y: >=-9223372036854775808 & <=9223372036854775807 & int
+	}
+	s: strings.MinRunes(4) & strings.MaxRunes(7)
+	additional: {
+		env: _
+		confs: {
+			if env {}
+		}
+	}
+}