cue: make *bottom not implement error

This gives us compile-time warnings when bottom
is not properly wrapped with a path.

- allows removing path logic for marshal error

Issue #52.

Change-Id: Ice05ef650dbec404a37ef1c887019f3be28a83c9
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/2208
Reviewed-by: Marcel van Lohuizen <mpvl@google.com>
diff --git a/cue/ast.go b/cue/ast.go
index 2a3d93c..15e6212 100644
--- a/cue/ast.go
+++ b/cue/ast.go
@@ -16,10 +16,12 @@
 
 import (
 	"fmt"
+	"strconv"
 	"strings"
 
 	"cuelang.org/go/cue/ast"
 	"cuelang.org/go/cue/build"
+	"cuelang.org/go/cue/errors"
 	"cuelang.org/go/cue/literal"
 	"cuelang.org/go/cue/token"
 	"cuelang.org/go/internal"
@@ -39,9 +41,14 @@
 	// First process single file.
 	v := newVisitor(inst.index, inst.inst, inst.rootStruct, inst.scope)
 	v.astState.astMap[f] = inst.rootStruct
+	// TODO: fix cmd/import to resolve references in the AST before
+	// inserting. For now, we accept errors that did not make it up to the tree.
 	result := v.walk(f)
 	if isBottom(result) {
-		return result.(*bottom)
+		if v.errors.Len() > 0 {
+			return v.errors
+		}
+		return &callError{result.(*bottom)}
 	}
 
 	return nil
@@ -50,6 +57,9 @@
 type astVisitor struct {
 	*astState
 	object *structLit
+
+	parent *astVisitor
+	sel    string // label or index; may be '*'
 	// For single line fields, the doc comment is applied to the inner-most
 	// field value.
 	//
@@ -75,6 +85,8 @@
 
 	// make unique per level to avoid reuse of structs being an issue.
 	astMap map[ast.Node]scope
+
+	errors errors.List
 }
 
 func (s *astState) mapScope(n ast.Node) (m scope) {
@@ -112,8 +124,27 @@
 	return v
 }
 
-func (v *astVisitor) error(n ast.Node, args ...interface{}) value {
-	return v.mkErr(newNode(n), args...)
+func (v *astVisitor) errf(n ast.Node, format string, args ...interface{}) evaluated {
+	v.astState.errors.Add(&nodeError{
+		path: v.appendPath(nil),
+		n:    n,
+		Message: errors.Message{
+			Format: format,
+			Args:   args,
+		},
+	})
+	arguments := append([]interface{}{format}, args...)
+	return v.mkErr(newNode(n), arguments...)
+}
+
+func (v *astVisitor) appendPath(a []string) []string {
+	if v.parent != nil {
+		a = v.parent.appendPath(a)
+	}
+	if v.sel != "" {
+		a = append(a, v.sel)
+	}
+	return a
 }
 
 func (v *astVisitor) resolve(n *ast.Ident) value {
@@ -139,7 +170,7 @@
 	ctx := v.ctx()
 	path, err := literal.Unquote(imp.Path.Value)
 	if err != nil {
-		return ctx.mkErr(newNode(imp), "illformed import spec")
+		return v.errf(imp, "illformed import spec")
 	}
 	// TODO: allow builtin *and* imported package. The result is a unified
 	// struct.
@@ -148,7 +179,7 @@
 	}
 	bimp := v.inst.LookupImport(path)
 	if bimp == nil {
-		return ctx.mkErr(newNode(imp), "package %q not found", path)
+		return v.errf(imp, "package %q not found", path)
 	}
 	impInst := v.index.loadInstance(bimp)
 	return impInst.rootValue.evalPartial(ctx)
@@ -193,6 +224,7 @@
 		v1 := &astVisitor{
 			astState: v.astState,
 			object:   obj,
+			parent:   v,
 		}
 		passDoc := len(n.Elts) == 1 && !n.Lbrace.IsValid() && v.doc != nil
 		if passDoc {
@@ -202,7 +234,7 @@
 			switch x := e.(type) {
 			case *ast.EmitDecl:
 				// Only allowed at top-level.
-				return v1.error(x, "emitting values is only allowed at top level")
+				return v1.errf(x, "emitting values is only allowed at top level")
 			case *ast.Field, *ast.Alias:
 				v1.walk(e)
 			case *ast.ComprehensionDecl:
@@ -214,6 +246,28 @@
 		}
 		value = obj
 
+	case *ast.ListLit:
+		v1 := &astVisitor{
+			astState: v.astState,
+			object:   v.object,
+			parent:   v,
+		}
+		arcs := []arc{}
+		for i, e := range n.Elts {
+			v1.sel = strconv.Itoa(i)
+			arcs = append(arcs, arc{feature: label(i), v: v1.walk(e)})
+		}
+		s := &structLit{baseValue: newExpr(n), arcs: arcs}
+		list := &list{baseValue: newExpr(n), elem: s}
+		list.initLit()
+		if n.Ellipsis != token.NoPos || n.Type != nil {
+			list.len = newBound(list.baseValue, opGeq, intKind, list.len)
+			if n.Type != nil {
+				list.typ = v1.walk(n.Type)
+			}
+		}
+		value = list
+
 	case *ast.ComprehensionDecl:
 		yielder := &yield{baseValue: newExpr(n.Field.Value)}
 		fc := &fieldComprehension{
@@ -223,10 +277,12 @@
 		field := n.Field
 		switch x := field.Label.(type) {
 		case *ast.Interpolation:
+			v.sel = "?"
 			yielder.key = v.walk(x)
 			yielder.value = v.walk(field.Value)
 
 		case *ast.TemplateLabel:
+			v.sel = "*"
 			f := v.label(x.Ident.Name, true)
 
 			sig := &params{}
@@ -241,8 +297,10 @@
 		case *ast.BasicLit, *ast.Ident:
 			name, ok := ast.LabelName(x)
 			if !ok {
-				return v.error(x, "invalid field name: %v", x)
+				return v.errf(x, "invalid field name: %v", x)
 			}
+			// TODO: is this correct? Just for info, so not very important.
+			v.sel = name
 
 			// TODO: if the clauses do not contain a guard, we know that this
 			// field will always be added and we can move the comprehension one
@@ -273,6 +331,7 @@
 		opt := n.Optional != token.NoPos
 		switch x := n.Label.(type) {
 		case *ast.Interpolation:
+			v.sel = "?"
 			yielder := &yield{baseValue: newNode(x)}
 			fc := &fieldComprehension{
 				baseValue: newDecl(n),
@@ -284,6 +343,7 @@
 			v.object.comprehensions = append(v.object.comprehensions, fc)
 
 		case *ast.TemplateLabel:
+			v.sel = "*"
 			f := v.label(x.Ident.Name, true)
 
 			sig := &params{}
@@ -303,13 +363,14 @@
 			if internal.DropOptional && opt {
 				break
 			}
+			v.sel, _ = ast.LabelName(x)
 			attrs, err := createAttrs(v.ctx(), newNode(n), n.Attrs)
 			if err != nil {
 				return err
 			}
 			f, ok := v.nodeLabel(x)
 			if !ok {
-				return v.error(n.Label, "invalid field name: %v", n.Label)
+				return v.errf(n.Label, "invalid field name: %v", n.Label)
 			}
 			if f != 0 {
 				var leftOverDoc *docNode
@@ -378,7 +439,7 @@
 				return r
 			}
 
-			value = v.error(n, "reference %q not found", n.Name)
+			value = v.errf(n, "reference %q not found", n.Name)
 			break
 		}
 
@@ -398,25 +459,26 @@
 		}
 
 	case *ast.BottomLit:
-		value = v.error(n, "from source")
+		// TODO: record inline comment.
+		value = &bottom{baseValue: newExpr(n), msg: "from source"}
 
 	case *ast.BadDecl:
 		// nothing to do
 
 	case *ast.BadExpr:
-		value = v.error(n, "invalid expression")
+		value = v.errf(n, "invalid expression")
 
 	case *ast.BasicLit:
 		value = v.litParser.parse(n)
 
 	case *ast.Interpolation:
 		if len(n.Elts) == 0 {
-			return v.error(n, "invalid interpolation")
+			return v.errf(n, "invalid interpolation")
 		}
 		first, ok1 := n.Elts[0].(*ast.BasicLit)
 		last, ok2 := n.Elts[len(n.Elts)-1].(*ast.BasicLit)
 		if !ok1 || !ok2 {
-			return v.error(n, "invalid interpolation")
+			return v.errf(n, "invalid interpolation")
 		}
 		if len(n.Elts) == 1 {
 			value = v.walk(n.Elts[0])
@@ -426,17 +488,17 @@
 		value = lit
 		info, prefixLen, _, err := literal.ParseQuotes(first.Value, last.Value)
 		if err != nil {
-			return v.error(n, "invalid interpolation: %v", err)
+			return v.errf(n, "invalid interpolation: %v", err)
 		}
 		prefix := ""
 		for i := 0; i < len(n.Elts); i += 2 {
 			l, ok := n.Elts[i].(*ast.BasicLit)
 			if !ok {
-				return v.error(n, "invalid interpolation")
+				return v.errf(n, "invalid interpolation")
 			}
 			s := l.Value
 			if !strings.HasPrefix(s, prefix) {
-				return v.error(l, "invalid interpolation: unmatched ')'")
+				return v.errf(l, "invalid interpolation: unmatched ')'")
 			}
 			s = l.Value[prefixLen:]
 			x := parseString(v.ctx(), l, info, s)
@@ -448,22 +510,6 @@
 			prefixLen = 1
 		}
 
-	case *ast.ListLit:
-		arcs := []arc{}
-		for i, e := range n.Elts {
-			arcs = append(arcs, arc{feature: label(i), v: v.walk(e)})
-		}
-		s := &structLit{baseValue: newExpr(n), arcs: arcs}
-		list := &list{baseValue: newExpr(n), elem: s}
-		list.initLit()
-		if n.Ellipsis != token.NoPos || n.Type != nil {
-			list.len = newBound(list.baseValue, opGeq, intKind, list.len)
-			if n.Type != nil {
-				list.typ = v.walk(n.Type)
-			}
-		}
-		value = list
-
 	case *ast.ParenExpr:
 		value = v.walk(n.X)
 
@@ -514,9 +560,9 @@
 			)
 
 		case token.MUL:
-			return v.error(n, "preference mark not allowed at this position")
+			return v.errf(n, "preference mark not allowed at this position")
 		default:
-			return v.error(n, "unsupported unary operator %q", n.Op)
+			return v.errf(n, "unsupported unary operator %q", n.Op)
 		}
 
 	case *ast.BinaryExpr:
diff --git a/cue/build.go b/cue/build.go
index 67c2b52..aac42eb 100644
--- a/cue/build.go
+++ b/cue/build.go
@@ -289,10 +289,10 @@
 			}
 		} else if _, ok := builtins[id]; !ok {
 			// continue
-			return idx.mkErr(newNode(spec), "package %q not found", id)
+			return nodeErrorf(spec, "package %q not found", id)
 		}
 		if n, ok := fields[name]; ok {
-			return idx.mkErr(newNode(spec),
+			return nodeErrorf(spec,
 				"%s redeclared as imported package name\n"+
 					"\tprevious declaration at %v", name, lineStr(idx, n))
 		}
@@ -303,10 +303,10 @@
 		}
 		if !used {
 			if spec.Name == nil {
-				errUnused = idx.mkErr(newNode(spec),
+				errUnused = nodeErrorf(spec,
 					"imported and not used: %s", spec.Path.Value)
 			} else {
-				errUnused = idx.mkErr(newNode(spec),
+				errUnused = nodeErrorf(spec,
 					"imported and not used: %s as %s", spec.Path.Value, spec.Name)
 			}
 		}
diff --git a/cue/builtin.go b/cue/builtin.go
index fc22d07..8a251cc 100644
--- a/cue/builtin.go
+++ b/cue/builtin.go
@@ -214,9 +214,10 @@
 		}
 		switch err := errVal.(type) {
 		case nil:
-		case *bottom:
-			ret = err
+		case *callError:
+			ret = err.b
 		default:
+			// TODO: store the underlying error explicitly
 			ret = ctx.mkErr(src, x, "call error: %v", err)
 		}
 	}()
@@ -290,6 +291,25 @@
 	return c.err == nil
 }
 
+type callError struct {
+	b *bottom
+}
+
+func (e *callError) Error() string {
+	return fmt.Sprint(e.b)
+}
+
+func (c *callCtxt) errf(src source, underlying error, format string, args ...interface{}) {
+	a := make([]interface{}, 0, 2+len(args))
+	if underlying != nil {
+		a = append(a, underlying)
+	}
+	a = append(a, format)
+	a = append(a, args...)
+	err := c.ctx.mkErr(src, a...)
+	c.err = &callError{err}
+}
+
 func (c *callCtxt) value(i int) Value {
 	return newValueRoot(c.ctx, c.args[i])
 }
@@ -305,11 +325,11 @@
 	x := newValueRoot(c.ctx, c.args[i])
 	n, err := x.Int(nil)
 	if err != nil {
-		c.err = c.ctx.mkErr(c.src, "argument %d must be in int, found number", i)
+		c.errf(c.src, err, "argument %d must be in int, found number", i)
 		return 0
 	}
 	if n.BitLen() > bits {
-		c.err = c.ctx.mkErr(c.src, err, "argument %d out of range: has %d > %d bits", n.BitLen(), bits)
+		c.errf(c.src, err, "argument %d out of range: has %d > %d bits", n.BitLen(), bits)
 	}
 	res, _ := x.Int64()
 	return res
@@ -326,15 +346,15 @@
 	x := newValueRoot(c.ctx, c.args[i])
 	n, err := x.Int(nil)
 	if err != nil {
-		c.err = c.ctx.mkErr(c.src, "argument %d must be an integer", i)
+		c.errf(c.src, err, "argument %d must be an integer", i)
 		return 0
 	}
 	if n.Sign() < 0 {
-		c.err = c.ctx.mkErr(c.src, "argument %d must be a positive integer", i)
+		c.errf(c.src, nil, "argument %d must be a positive integer", i)
 		return 0
 	}
 	if n.BitLen() > bits {
-		c.err = c.ctx.mkErr(c.src, err, "argument %d out of range: has %d > %d bits", i, n.BitLen(), bits)
+		c.errf(c.src, nil, "argument %d out of range: has %d > %d bits", i, n.BitLen(), bits)
 	}
 	res, _ := x.Uint64()
 	return res
@@ -344,7 +364,7 @@
 	x := newValueRoot(c.ctx, c.args[i])
 	res, err := x.Float64()
 	if err != nil {
-		c.err = c.ctx.mkErr(c.src, err, "invalid argument %d: %v", i, err)
+		c.errf(c.src, err, "invalid argument %d: %v", i, err)
 		return 0
 	}
 	return res
@@ -354,7 +374,7 @@
 	x := newValueRoot(c.ctx, c.args[i])
 	n, err := x.Int(nil)
 	if err != nil {
-		c.err = c.ctx.mkErr(c.src, "argument %d must be in int, found number", i)
+		c.errf(c.src, err, "argument %d must be in int, found number", i)
 		return nil
 	}
 	return n
@@ -365,7 +385,7 @@
 	var mant big.Int
 	exp, err := x.MantExp(&mant)
 	if err != nil {
-		c.err = c.ctx.mkErr(c.src, err, "invalid argument %d: %v", i, err)
+		c.errf(c.src, err, "invalid argument %d: %v", i, err)
 		return nil
 	}
 	f := &big.Float{}
@@ -382,7 +402,7 @@
 	x := newValueRoot(c.ctx, c.args[i])
 	v, err := x.String()
 	if err != nil {
-		c.err = c.ctx.mkErr(c.src, err, "invalid argument %d: %v", i, err)
+		c.errf(c.src, err, "invalid argument %d: %v", i, err)
 		return ""
 	}
 	return v
@@ -392,7 +412,7 @@
 	x := newValueRoot(c.ctx, c.args[i])
 	v, err := x.Bytes()
 	if err != nil {
-		c.err = c.ctx.mkErr(c.src, err, "invalid argument %d: %v", i, err)
+		c.errf(c.src, err, "invalid argument %d: %v", i, err)
 		return nil
 	}
 	return v
@@ -403,7 +423,7 @@
 	// TODO: optimize for string and bytes cases
 	r, err := x.Reader()
 	if err != nil {
-		c.err = c.ctx.mkErr(c.src, err, "invalid argument %d: %v", i, err)
+		c.errf(c.src, err, "invalid argument %d: %v", i, err)
 		return nil
 	}
 	return r
@@ -413,22 +433,17 @@
 	x := newValueRoot(c.ctx, c.args[i])
 	b, err := x.Bool()
 	if err != nil {
-		c.err = c.ctx.mkErr(c.src, err, "invalid argument %d: %v", i, err)
+		c.errf(c.src, err, "invalid argument %d: %v", i, err)
 		return false
 	}
 	return b
 }
 
-func (c *callCtxt) error(i int) error {
-	x := newValueRoot(c.ctx, c.args[i])
-	return x.err()
-}
-
 func (c *callCtxt) list(i int) (a Iterator) {
 	x := newValueRoot(c.ctx, c.args[i])
 	v, err := x.List()
 	if err != nil {
-		c.err = c.ctx.mkErr(c.src, err, "invalid argument %d: %v", i, err)
+		c.errf(c.src, err, "invalid argument %d: %v", i, err)
 		return Iterator{ctx: c.ctx}
 	}
 	return v
@@ -438,13 +453,13 @@
 	x := newValueRoot(c.ctx, c.args[i])
 	v, err := x.List()
 	if err != nil {
-		c.err = c.ctx.mkErr(c.src, err, "invalid argument %d: %v", i, err)
+		c.errf(c.src, err, "invalid argument %d: %v", i, err)
 		return nil
 	}
 	for i := 0; v.Next(); i++ {
 		str, err := v.Value().String()
 		if err != nil {
-			c.err = c.ctx.mkErr(c.src, err, "list element %d: %v", i, err)
+			c.errf(c.src, err, "list element %d: %v", i, err)
 		}
 		a = append(a, str)
 	}
diff --git a/cue/errors.go b/cue/errors.go
index 3bf3dbb..0a8d845 100644
--- a/cue/errors.go
+++ b/cue/errors.go
@@ -16,12 +16,10 @@
 
 import (
 	"fmt"
-	"sort"
 
 	"cuelang.org/go/cue/ast"
 	"cuelang.org/go/cue/errors"
 	"cuelang.org/go/cue/token"
-	"golang.org/x/xerrors"
 )
 
 var _ errors.Error = &nodeError{}
@@ -166,38 +164,11 @@
 	return pos
 }
 
-func (x *bottom) Error() string { return fmt.Sprint(x) }
-
 func (x *bottom) Format(s fmt.State, verb rune) {
-	xerrors.FormatError(x, s, verb)
-}
-
-func (x *bottom) FormatError(p xerrors.Printer) error {
-	p.Print(x.msg)
-	if p.Detail() && x.index != nil {
-		locs := appendLocations(nil, x.pos)
-		sort.Strings(locs)
-		for _, l := range locs {
-			p.Printf("%s\n", l)
-		}
-	}
+	fmt.Fprint(s, x.msg)
 	if x.wrapped != nil {
-		return x.wrapped // nil interface
+		fmt.Fprint(s, ": ", x.wrapped)
 	}
-	return nil
-}
-
-func appendLocations(locs []string, src source) []string {
-	if src != nil {
-		if p := src.Pos(); p != token.NoPos {
-			return append(locs, src.Pos().String())
-		}
-		if c := src.computed(); c != nil {
-			locs = appendLocations(locs, c.x)
-			locs = appendLocations(locs, c.y)
-		}
-	}
-	return locs
 }
 
 func cycleError(v evaluated) *bottom {
diff --git a/cue/instance.go b/cue/instance.go
index 219c548..b797327 100644
--- a/cue/instance.go
+++ b/cue/instance.go
@@ -210,9 +210,10 @@
 	}
 
 	ctx := inst.newContext()
-	v, err := newValueRoot(ctx, inst.rootValue).structVal(ctx)
+	val := newValueRoot(ctx, inst.rootValue)
+	v, err := val.structVal(ctx)
 	if err != nil {
-		i.setError(err)
+		i.setError(val.toErr(err))
 		return i
 	}
 	i.scope = v.n
diff --git a/cue/types.go b/cue/types.go
index 4425930..e199984 100644
--- a/cue/types.go
+++ b/cue/types.go
@@ -117,20 +117,20 @@
 
 // MarshalJSON returns a valid JSON encoding or reports an error if any of the
 // fields is invalid.
-func (o *structValue) marshalJSON() (b []byte, err error) {
+func (o *structValue) marshalJSON() (b []byte, err errors.Error) {
 	b = append(b, '{')
 	n := o.Len()
 	for i := 0; i < n; i++ {
 		k, v := o.At(i)
 		s, err := json.Marshal(k)
 		if err != nil {
-			return nil, unwrapJSONError(k, err)
+			return nil, unwrapJSONError(err)
 		}
 		b = append(b, s...)
 		b = append(b, ':')
 		bb, err := json.Marshal(v)
 		if err != nil {
-			return nil, unwrapJSONError(k, err)
+			return nil, unwrapJSONError(err)
 		}
 		b = append(b, bb...)
 		if i < n-1 {
@@ -141,32 +141,45 @@
 	return b, nil
 }
 
+var _ errors.Error = &marshalError{}
+
 type marshalError struct {
-	path string
-	err  error
-	// TODO: maybe also collect the file locations contributing to the
-	// failing values?
+	err errors.Error
+}
+
+func toMarshalErr(v Value, b *bottom) error {
+	return &marshalError{v.toErr(b)}
+}
+
+func marshalErrf(v Value, src source, msg string, args ...interface{}) error {
+	arguments := append([]interface{}{msg}, args...)
+	b := v.idx.mkErr(src, arguments...)
+	return toMarshalErr(v, b)
 }
 
 func (e *marshalError) Error() string {
-	return fmt.Sprintf("cue: marshal error at path %s: %v", e.path, e.err)
+	path := e.Path()
+	if len(path) == 0 {
+		return fmt.Sprintf("cue: marshal error: %v", e.err)
+	}
+	p := strings.Join(path, ".")
+	return fmt.Sprintf("cue: marshal error at path %s: %v", p, e.err)
 }
 
-func unwrapJSONError(key interface{}, err error) error {
-	if je, ok := err.(*json.MarshalerError); ok {
-		err = je.Err
+func (e *marshalError) Path() []string      { return e.err.Path() }
+func (e *marshalError) Position() token.Pos { return e.err.Position() }
+
+func unwrapJSONError(err error) errors.Error {
+	switch x := err.(type) {
+	case *json.MarshalerError:
+		return unwrapJSONError(x.Err)
+	case *marshalError:
+		return x
+	case errors.Error:
+		return &marshalError{x}
+	default:
+		return &marshalError{errors.Wrapf(err, token.NoPos, err.Error())}
 	}
-	path := make([]string, 0, 2)
-	if key != nil {
-		path = append(path, fmt.Sprintf("%v", key))
-	}
-	if me, ok := err.(*marshalError); ok {
-		if me.path != "" {
-			path = append(path, me.path)
-		}
-		err = me.err
-	}
-	return &marshalError{strings.Join(path, "."), err}
 }
 
 // An Iterator iterates over values.
@@ -223,13 +236,13 @@
 
 // marshalJSON iterates over the list and generates JSON output. HasNext
 // will return false after this operation.
-func marshalList(l *Iterator) (b []byte, err error) {
+func marshalList(l *Iterator) (b []byte, err errors.Error) {
 	b = append(b, '[')
 	if l.Next() {
 		for i := 0; ; i++ {
 			x, err := json.Marshal(l.Value())
 			if err != nil {
-				return nil, unwrapJSONError(i, err)
+				return nil, unwrapJSONError(err)
 			}
 			b = append(b, x...)
 			if !l.Next() {
@@ -242,10 +255,10 @@
 	return b, nil
 }
 
-func (v Value) getNum(k kind) (*numLit, error) {
+func (v Value) getNum(k kind) (*numLit, errors.Error) {
 	v, _ = v.Default()
 	if err := v.checkKind(v.ctx(), k); err != nil {
-		return nil, err
+		return nil, v.toErr(err)
 	}
 	n, _ := v.path.cache.(*numLit)
 	return n, nil
@@ -660,7 +673,7 @@
 func (v Value) MarshalJSON() (b []byte, err error) {
 	b, err = v.marshalJSON()
 	if err != nil {
-		return nil, unwrapJSONError(nil, err)
+		return nil, unwrapJSONError(err)
 	}
 	return b, nil
 }
@@ -692,15 +705,15 @@
 		obj, _ := v.structVal(ctx)
 		return obj.marshalJSON()
 	case bottomKind:
-		return nil, x.(*bottom)
+		return nil, toMarshalErr(v, x.(*bottom))
 	default:
 		if k.hasReferences() {
-			return nil, v.idx.mkErr(x, "value %q contains unresolved references", debugStr(ctx, x))
+			return nil, marshalErrf(v, x, "value %q contains unresolved references", debugStr(ctx, x))
 		}
 		if !k.isGround() {
-			return nil, v.idx.mkErr(x, "cannot convert incomplete value %q to JSON", debugStr(ctx, x))
+			return nil, marshalErrf(v, x, "cannot convert incomplete value %q to JSON", debugStr(ctx, x))
 		}
-		return nil, v.idx.mkErr(x, "cannot convert value %q of type %T to JSON", debugStr(ctx, x), x)
+		return nil, marshalErrf(v, x, "cannot convert value %q of type %T to JSON", debugStr(ctx, x), x)
 	}
 }
 
@@ -794,8 +807,7 @@
 	return nil
 }
 
-// TODO: make bottom not an error and then make this return *bottom.
-func (v Value) err() error {
+func (v Value) err() *bottom {
 	// TODO(incomplete): change to not return an error for incomplete.
 	if err := v.checkKind(v.ctx(), bottomKind); err != nil {
 		return err
@@ -1406,7 +1418,7 @@
 func (v Value) Attribute(key string) Attribute {
 	// look up the attributes
 	if v.path == nil || v.path.attrs == nil {
-		return Attribute{err: errNotExists}
+		return Attribute{err: v.toErr(errNotExists)}
 	}
 	for _, a := range v.path.attrs.attr {
 		if a.key() != key {
@@ -1418,7 +1430,7 @@
 		}
 		return at
 	}
-	return Attribute{err: errNotExists}
+	return Attribute{err: v.toErr(errNotExists)}
 }
 
 var (
diff --git a/cue/types_test.go b/cue/types_test.go
index e8a59c3..a4e60c4 100644
--- a/cue/types_test.go
+++ b/cue/types_test.go
@@ -463,7 +463,7 @@
 	}}
 	for _, tc := range testCases {
 		t.Run(tc.value, func(t *testing.T) {
-			err := getInstance(t, tc.value).Value().err()
+			err := getInstance(t, tc.value).Value().Err()
 			checkErr(t, err, tc.err, "init")
 		})
 	}
@@ -638,7 +638,7 @@
 				}
 			}
 			v := obj.Lookup("non-existing")
-			checkErr(t, v.err(), "not found", "non-existing")
+			checkErr(t, v.Err(), "not found", "non-existing")
 		})
 	}
 }
@@ -1610,7 +1610,6 @@
 		json:  `false`,
 	}, {
 		value: `bool`,
-		json:  `bool`,
 		err:   "cannot convert incomplete value",
 	}, {
 		value: `"str"`,
@@ -1635,7 +1634,7 @@
 		json:  `[1,2,3]`,
 	}, {
 		value: `[int]`,
-		err:   `cannot convert incomplete value`,
+		err:   `0: cannot convert incomplete value`,
 	}, {
 		value: `(>=3 * [1, 2])`,
 		err:   "incomplete error", // TODO: improve error
@@ -1646,6 +1645,12 @@
 		value: `{a: 2, b: 3, c: ["A", "B"]}`,
 		json:  `{"a":2,"b":3,"c":["A","B"]}`,
 	}, {
+		value: `{a: 2, b: 3, c: [string, "B"]}`,
+		err:   `c.0: cannot convert incomplete value`,
+	}, {
+		value: `{a: [{b: [0, {c: string}] }] }`,
+		err:   `path a.0.b.1.c: cannot convert incomplete value`,
+	}, {
 		value: `{foo?: 1, bar?: 2, baz: 3}`,
 		json:  `{"baz":3}`,
 	}, {