internal/core: fix incorrect error handling

1: don't double-wrap errors in BinOp. This could happen
when a value is passed as a Vertex in API usage.
2: subsumption incorrectly used And op for BinOp.

Together these Fixes #461

This also improves reporting of structural errors,
although this is not complete and not the goal of
this CL.

Change-Id: Id021a0eb4fa5628f56167d2fccea7d99dbddf390
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/7065
Reviewed-by: CUE cueckoo <[email protected]>
Reviewed-by: Marcel van Lohuizen <[email protected]>

Author: mpvl

cmd/cue/cmd/testdata/script/issue461.txt

@@ -0,0 +1,26 @@
+cue cmd writefile
+
+-- cue.mod/module.cue --
+module: "mod.com"
+-- expect-hello --
+Hello, world!
+-- x_tool.cue --
+package x
+
+import (
+	"strings"
+	"tool/exec"
+	"tool/file"
+)
+
+command: writefile: {
+	echo: exec.Run & {
+		cmd:    "echo hello.txt"
+		stdout: string
+	}
+
+	write: file.Create & {
+		filename:  strings.TrimSpace(echo.stdout)
+		contents: "Hello, world!"
+	}
+}

cue/testdata/cycle/structural.txtar

@@ -84,7 +84,7 @@ d2: {
 }
 
 d3: {
-    // TODO(errors): position reporting in structural cycle.
+    // TODO(errors): correct position reporting in structural cycle.
     config: {
         a: b: c: indirect
         indirect: [a.b, null][i]
@@ -253,6 +253,8 @@ d2.a.b.c.d.t: structural cycle:
     ./in.cue:79:8
 d2.r: structural cycle:
     ./in.cue:79:8
+0: structural cycle:
+    ./in.cue:89:19
 
 Result:
 (_|_){
@@ -507,12 +509,14 @@ Result:
         b: (_|_){
           // [structural cycle]
           c: (_|_){
-            // [structural cycle]
+            // [structural cycle] 0: structural cycle:
+            //     ./in.cue:89:19
           }
         }
       }
       indirect: (_|_){
-        // [structural cycle]
+        // [structural cycle] 0: structural cycle:
+        //     ./in.cue:89:19
       }
       i: (int){ 1 }
     }
@@ -523,12 +527,14 @@ Result:
         b: (_|_){
           // [structural cycle]
           c: (_|_){
-            // [structural cycle]
+            // [structural cycle] 0: structural cycle:
+            //     ./in.cue:89:19
           }
         }
       }
       indirect: (_|_){
-        // [structural cycle]
+        // [structural cycle] 0: structural cycle:
+        //     ./in.cue:89:19
       }
       i: (int){ 0 }
     }

internal/core/adt/binop.go

@@ -51,11 +51,8 @@ func BinOp(c *OpContext, op Op, left, right Value) Value {
 		}
 	}
 
-	if a, ok := left.(*Bottom); ok {
-		return CombineErrors(nil, a, right)
-	}
-	if b, ok := left.(*Bottom); ok {
-		return b
+	if err := CombineErrors(c.src, left, right); err != nil {
+		return err
 	}
 
 	switch op {

internal/core/adt/errors.go

@@ -171,8 +171,8 @@ func (v *Vertex) AddChildError(recursive *Bottom) {
 
 // CombineErrors combines two errors that originate at the same Vertex.
 func CombineErrors(src ast.Node, x, y Value) *Bottom {
-	a, _ := x.(*Bottom)
-	b, _ := y.(*Bottom)
+	a, _ := Unwrap(x).(*Bottom)
+	b, _ := Unwrap(y).(*Bottom)
 
 	switch {
 	case a != nil && b != nil:

internal/core/subsume/subsume.go

@@ -103,14 +103,21 @@ func (s *subsumer) errf(msg string, args ...interface{}) {
 	s.errs = errors.Append(s.errs, b.Err)
 }
 
+func unifyValue(c *adt.OpContext, a, b adt.Value) adt.Value {
+	v := &adt.Vertex{}
+	v.AddConjunct(adt.MakeRootConjunct(c.Env(0), a))
+	v.AddConjunct(adt.MakeRootConjunct(c.Env(0), b))
+	return c.Unifier.Evaluate(c, v)
+}
+
 func (s *subsumer) getError() (err errors.Error) {
 	c := s.ctx
 	// src := binSrc(token.NoPos, opUnify, gt, lt)
 	if s.gt != nil && s.lt != nil {
 		// src := binSrc(token.NoPos, opUnify, s.gt, s.lt)
 		if s.missing != 0 {
 			s.errf("missing field %q", s.missing.SelectorString(c))
-		} else if b, ok := adt.BinOp(c, adt.AndOp, s.gt, s.lt).(*adt.Bottom); !ok {
+		} else if b, ok := unifyValue(c, s.gt, s.lt).(*adt.Bottom); !ok {
 			s.errf("value not an instance")
 		} else {
 			s.errs = errors.Append(s.errs, b.Err)