internal/core/eval: add more positions to conflict errors
This should bring conflict errors mostly up to par with v0.2.2.
It also adds some context errors for definitions. More refined
tracing of references (in acceptor.Canopy) will be able to
address Issue #129.
Change-Id: Ic359a324c2d3034b4895b06cb7f91c3c546a8a0a
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/7302
Reviewed-by: CUE cueckoo <cueckoo@gmail.com>
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
diff --git a/internal/core/adt/context.go b/internal/core/adt/context.go
index bdf1af9..7511b51 100644
--- a/internal/core/adt/context.go
+++ b/internal/core/adt/context.go
@@ -332,6 +332,9 @@
}
// Validate calls validates value for the given validator.
+//
+// TODO(errors): return boolean instead: only the caller has enough information
+// to generate a proper error message.
func (c *OpContext) Validate(check Validator, value Value) *Bottom {
// TODO: use a position stack to push both values.
saved := c.src
diff --git a/internal/core/adt/errors.go b/internal/core/adt/errors.go
index 1f54c86..5085e28 100644
--- a/internal/core/adt/errors.go
+++ b/internal/core/adt/errors.go
@@ -201,8 +201,8 @@
}
}
-// A valueError is returned as a result of evaluating a value.
-type valueError struct {
+// A ValueError is returned as a result of evaluating a value.
+type ValueError struct {
r Runtime
v *Vertex
pos token.Pos
@@ -210,6 +210,20 @@
errors.Message
}
+func (v *ValueError) AddPosition(n Node) {
+ if n == nil {
+ return
+ }
+ if p := pos(n); p != token.NoPos {
+ for _, q := range v.auxpos {
+ if p == q {
+ return
+ }
+ }
+ v.auxpos = append(v.auxpos, p)
+ }
+}
+
func (c *OpContext) errNode() *Vertex {
return c.vertex
}
@@ -225,14 +239,16 @@
}
func (c *OpContext) AddPosition(n Node) {
- c.positions = append(c.positions, n)
+ if n != nil {
+ c.positions = append(c.positions, n)
+ }
}
-func (c *OpContext) Newf(format string, args ...interface{}) *valueError {
+func (c *OpContext) Newf(format string, args ...interface{}) *ValueError {
return c.NewPosf(c.pos(), format, args...)
}
-func (c *OpContext) NewPosf(p token.Pos, format string, args ...interface{}) *valueError {
+func (c *OpContext) NewPosf(p token.Pos, format string, args ...interface{}) *ValueError {
var a []token.Pos
if len(c.positions) > 0 {
a = make([]token.Pos, 0, len(c.positions))
@@ -248,7 +264,7 @@
}
}
}
- return &valueError{
+ return &ValueError{
r: c.Runtime,
v: c.errNode(),
pos: p,
@@ -257,19 +273,19 @@
}
}
-func (e *valueError) Error() string {
+func (e *ValueError) Error() string {
return errors.String(e)
}
-func (e *valueError) Position() token.Pos {
+func (e *ValueError) Position() token.Pos {
return e.pos
}
-func (e *valueError) InputPositions() (a []token.Pos) {
+func (e *ValueError) InputPositions() (a []token.Pos) {
return e.auxpos
}
-func (e *valueError) Path() (a []string) {
+func (e *ValueError) Path() (a []string) {
for _, f := range appendPath(nil, e.v) {
a = append(a, f.SelectorString(e.r))
}
diff --git a/internal/core/adt/expr.go b/internal/core/adt/expr.go
index ea55ec3..ff1b2de 100644
--- a/internal/core/adt/expr.go
+++ b/internal/core/adt/expr.go
@@ -394,6 +394,8 @@
if v.B {
return nil
}
+ // TODO(errors): use "invalid value %v (not an %s)" if x is a
+ // predeclared identifier such as `int`.
return c.NewErrf("invalid value %v (out of bound %s)",
c.Str(y), c.Str(x))
diff --git a/internal/core/eval/eval.go b/internal/core/eval/eval.go
index 556d1a3..654fdc0 100644
--- a/internal/core/eval/eval.go
+++ b/internal/core/eval/eval.go
@@ -454,15 +454,15 @@
for n.maybeSetCache(); n.expandOne(); n.maybeSetCache() {
}
- if aList := n.addLists(ctx); aList != nil {
- n.updateNodeType(adt.ListKind, aList)
+ if aList, id := n.addLists(ctx); aList != nil {
+ n.updateNodeType(adt.ListKind, aList, id)
} else {
break
}
}
if n.aStruct != nil {
- n.updateNodeType(adt.StructKind, n.aStruct)
+ n.updateNodeType(adt.StructKind, n.aStruct, n.aStructID)
}
switch err := n.getErr(); {
@@ -524,15 +524,32 @@
if v != nil && adt.IsConcrete(v) {
if n.lowerBound != nil {
if b := ctx.Validate(n.lowerBound, v); b != nil {
+ // TODO(errors): make Validate return boolean and generate
+ // optimized conflict message. Also track and inject IDs
+ // to determine origin location.s
+ if e, _ := b.Err.(*adt.ValueError); e != nil {
+ e.AddPosition(n.lowerBound)
+ e.AddPosition(v)
+ }
n.addBottom(b)
}
}
if n.upperBound != nil {
if b := ctx.Validate(n.upperBound, v); b != nil {
+ // TODO(errors): make Validate return boolean and generate
+ // optimized conflict message. Also track and inject IDs
+ // to determine origin location.s
+ if e, _ := b.Err.(*adt.ValueError); e != nil {
+ e.AddPosition(n.upperBound)
+ e.AddPosition(v)
+ }
n.addBottom(b)
}
}
for _, v := range n.checks {
+ // TODO(errors): make Validate return boolean and generate
+ // optimized conflict message. Also track and inject IDs
+ // to determine origin location.s
if b := ctx.Validate(v, n.node); b != nil {
n.addBottom(b)
}
@@ -727,11 +744,13 @@
arcMap map[arcKey]bool
// Current value (may be under construction)
- scalar adt.Value // TODO: use Value in node.
+ scalar adt.Value // TODO: use Value in node.
+ scalarID adt.ID
// Concrete conjuncts
kind adt.Kind
kindExpr adt.Expr // expr that adjust last value (for error reporting)
+ kindID adt.ID // for error tracing
lowerBound *adt.BoundValue // > or >=
upperBound *adt.BoundValue // < or <=
checks []adt.Validator // BuiltinValidator, other bound values.
@@ -745,6 +764,7 @@
// TODO: remove this and annotate directly in acceptor.
needClose bool
aStruct adt.Expr
+ aStructID adt.ID
hasTop bool
// Expression conjuncts
@@ -770,7 +790,50 @@
return a
}
-func (n *nodeContext) updateNodeType(k adt.Kind, v adt.Expr) bool {
+// TODO(errors): return a dedicated ConflictError that can track original
+// positions on demand.
+//
+// Fully detailed origin tracking could be created on the back of CloseIDs
+// tracked in acceptor.Canopy. To make this fully functional, the following
+// still needs to be implemented:
+//
+// - We need to know the ID for every value involved in the conflict.
+// (There may be multiple if the result comes from a simplification).
+// - CloseIDs should be forked not only for definitions, but all references.
+// - Upon forking a CloseID, it should keep track of its enclosing CloseID
+// as well (probably as a pointer to the origin so that the reference
+// persists after compaction).
+//
+// The modifications to the CloseID algorithm may also benefit the computation
+// of `{ #A } == #A`, which currently is not done.
+//
+func (n *nodeContext) addConflict(
+ v1, v2 adt.Node,
+ k1, k2 adt.Kind,
+ id1, id2 adt.ID) {
+
+ ctx := n.ctx
+
+ var err *adt.ValueError
+ if k1 == k2 {
+ err = ctx.NewPosf(token.NoPos,
+ "conflicting values %s and %s", ctx.Str(v1), ctx.Str(v2))
+ } else {
+ err = ctx.NewPosf(token.NoPos,
+ "conflicting values %s and %s (mismatched types %s and %s)",
+ ctx.Str(v1), ctx.Str(v2), k1, k2)
+ }
+
+ ci := closedInfo(n.node)
+ err.AddPosition(v1)
+ err.AddPosition(v2)
+ err.AddPosition(ci.node(id1).Src)
+ err.AddPosition(ci.node(id2).Src)
+
+ n.addErr(err)
+}
+
+func (n *nodeContext) updateNodeType(k adt.Kind, v adt.Expr, id adt.ID) bool {
ctx := n.ctx
kind := n.kind & k
@@ -781,9 +844,7 @@
case kind == adt.BottomKind:
if n.kindExpr != nil {
- n.addErr(ctx.Newf(
- "conflicting values %s and %s (mismatched types %s and %s)",
- ctx.Str(n.kindExpr), ctx.Str(v), n.kind, k))
+ n.addConflict(n.kindExpr, v, n.kind, k, n.kindID, id)
} else {
n.addErr(ctx.Newf(
"conflicting value %s (mismatched types %s and %s)",
@@ -1190,6 +1251,7 @@
if x, ok := v.(*adt.Vertex); ok {
if m, ok := x.Value.(*adt.StructMarker); ok {
n.aStruct = x
+ n.aStructID = id
if m.NeedClose {
ci := closedInfo(n.node)
ci.isClosed = true // TODO: remove
@@ -1248,7 +1310,7 @@
return
}
- if !n.updateNodeType(v.Kind(), v) {
+ if !n.updateNodeType(v.Kind(), v, id) {
return
}
@@ -1280,7 +1342,13 @@
case adt.LessThanOp, adt.LessEqualOp:
if y := n.upperBound; y != nil {
n.upperBound = nil
- n.addValueConjunct(env, adt.SimplifyBounds(ctx, n.kind, x, y), id)
+ v := adt.SimplifyBounds(ctx, n.kind, x, y)
+ if err := valueError(v); err != nil {
+ err.AddPosition(v)
+ err.AddPosition(n.upperBound)
+ err.AddPosition(closedInfo(n.node).node(id).Src)
+ }
+ n.addValueConjunct(env, v, id)
return
}
n.upperBound = x
@@ -1288,7 +1356,13 @@
case adt.GreaterThanOp, adt.GreaterEqualOp:
if y := n.lowerBound; y != nil {
n.lowerBound = nil
- n.addValueConjunct(env, adt.SimplifyBounds(ctx, n.kind, x, y), id)
+ v := adt.SimplifyBounds(ctx, n.kind, x, y)
+ if err := valueError(v); err != nil {
+ err.AddPosition(v)
+ err.AddPosition(n.lowerBound)
+ err.AddPosition(closedInfo(n.node).node(id).Src)
+ }
+ n.addValueConjunct(env, v, id)
return
}
n.lowerBound = x
@@ -1332,7 +1406,7 @@
case adt.Value: // *NullLit, *BoolLit, *NumLit, *StringLit, *BytesLit
if y := n.scalar; y != nil {
if b, ok := adt.BinOp(ctx, adt.EqualOp, x, y).(*adt.Bool); !ok || !b.B {
- n.addErr(ctx.Newf("incompatible values %s and %s", ctx.Str(x), ctx.Str(y)))
+ n.addConflict(x, y, x.Kind(), y.Kind(), n.scalarID, id)
}
// TODO: do we need to explicitly add again?
// n.scalar = nil
@@ -1340,6 +1414,7 @@
break
}
n.scalar = x
+ n.scalarID = id
default:
panic(fmt.Sprintf("unknown value type %T", x))
@@ -1347,6 +1422,11 @@
if n.lowerBound != nil && n.upperBound != nil {
if u := adt.SimplifyBounds(ctx, n.kind, n.lowerBound, n.upperBound); u != nil {
+ if err := valueError(u); err != nil {
+ err.AddPosition(n.lowerBound)
+ err.AddPosition(n.upperBound)
+ err.AddPosition(closedInfo(n.node).node(id).Src)
+ }
n.lowerBound = nil
n.upperBound = nil
n.addValueConjunct(env, u, id)
@@ -1354,6 +1434,21 @@
}
}
+func valueError(v adt.Value) *adt.ValueError {
+ if v == nil {
+ return nil
+ }
+ b, _ := v.(*adt.Bottom)
+ if b == nil {
+ return nil
+ }
+ err, _ := b.Err.(*adt.ValueError)
+ if err == nil {
+ return nil
+ }
+ return err
+}
+
// addStruct collates the declarations of a struct.
//
// addStruct fulfills two additional pivotal functions:
@@ -1406,11 +1501,13 @@
case *adt.OptionalField:
if x.Label.IsString() {
n.aStruct = s
+ n.aStructID = closeID
}
opt.AddOptional(ctx, x)
case *adt.DynamicField:
n.aStruct = s
+ n.aStructID = closeID
hasOther = x
n.dynamicFields = append(n.dynamicFields, envDynamic{childEnv, x, closeID})
opt.AddDynamic(ctx, childEnv, x)
@@ -1438,11 +1535,13 @@
case *adt.BulkOptionalField:
n.aStruct = s
+ n.aStructID = closeID
hasBulk = x
opt.AddBulk(ctx, x)
case *adt.Ellipsis:
n.aStruct = s
+ n.aStructID = closeID
hasBulk = x
opt.AddEllipsis(ctx, x)
@@ -1457,6 +1556,7 @@
if !hasEmbed {
n.aStruct = s
+ n.aStructID = closeID
}
// Apply existing fields
@@ -1472,6 +1572,7 @@
case *adt.Field:
if x.Label.IsString() {
n.aStruct = s
+ n.aStructID = closeID
}
n.insertField(x.Label, adt.MakeConjunct(childEnv, x, closeID))
}
@@ -1626,9 +1727,9 @@
//
// TODO(embeddedScalars): for embedded scalars, there should be another pass
// of evaluation expressions after expanding lists.
-func (n *nodeContext) addLists(c *adt.OpContext) (oneOfTheLists adt.Expr) {
+func (n *nodeContext) addLists(c *adt.OpContext) (oneOfTheLists adt.Expr, anID adt.ID) {
if len(n.lists) == 0 && len(n.vLists) == 0 {
- return nil
+ return nil, 0
}
isOpen := true
@@ -1681,6 +1782,7 @@
outer:
for i, l := range n.lists {
oneOfTheLists = l.list
+ anID = l.id
n.updateCyclicStatus(l.env)
@@ -1806,7 +1908,7 @@
n.lists = n.lists[:0]
n.vLists = n.vLists[:0]
- return oneOfTheLists
+ return oneOfTheLists, anID
}
func (n *nodeContext) invalidListLength(na, nb int, a, b adt.Expr) {
diff --git a/internal/core/validate/validate_test.go b/internal/core/validate/validate_test.go
index a47afbd..496ad3b 100644
--- a/internal/core/validate/validate_test.go
+++ b/internal/core/validate/validate_test.go
@@ -65,20 +65,20 @@
in: `
1 & 2
`,
- out: "eval\nincompatible values 2 and 1",
+ out: "eval\nconflicting values 2 and 1:\n test:2:3\n test:2:7",
}, {
desc: "evaluation error in field",
in: `
x: 1 & 2
`,
- out: "eval\nx: incompatible values 2 and 1",
+ out: "eval\nx: conflicting values 2 and 1:\n test:2:6\n test:2:10",
}, {
desc: "first error",
in: `
x: 1 & 2
y: 2 & 4
`,
- out: "eval\nx: incompatible values 2 and 1",
+ out: "eval\nx: conflicting values 2 and 1:\n test:2:6\n test:2:10",
}, {
desc: "all errors",
cfg: &Config{AllErrors: true},
@@ -87,8 +87,12 @@
y: 2 & 4
`,
out: `eval
-x: incompatible values 2 and 1
-y: incompatible values 4 and 2`,
+x: conflicting values 2 and 1:
+ test:2:6
+ test:2:10
+y: conflicting values 4 and 2:
+ test:3:6
+ test:3:10`,
}, {
desc: "incomplete",
cfg: &Config{Concrete: true},
@@ -142,7 +146,7 @@
y: string
x: 1 & 2
`,
- out: "eval\nx: incompatible values 2 and 1",
+ out: "eval\nx: conflicting values 2 and 1:\n test:3:6\n test:3:10",
}, {
desc: "consider defaults for concreteness",
cfg: &Config{Concrete: true},