cue: improved error message
One of many steps to go.
The most notatble improvement here is that there is more than one location for an "empty disjunction" message.
Issue #129, Issue #52.
Change-Id: I8ff10e9a0be0eea685b9134c7804460249454a09
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/3780
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
diff --git a/cmd/cue/cmd/testdata/script/eval_expr.txt b/cmd/cue/cmd/testdata/script/eval_expr.txt
index 6475b28..4d7b08e 100644
--- a/cmd/cue/cmd/testdata/script/eval_expr.txt
+++ b/cmd/cue/cmd/testdata/script/eval_expr.txt
@@ -9,6 +9,7 @@
-- expect-stderr --
invalid non-ground value string (must be concrete int|string):
./partial.cue:7:9
+ ./partial.cue:8:7
-- partial.cue --
package partial
diff --git a/cmd/cue/cmd/testdata/script/vet_concrete.txt b/cmd/cue/cmd/testdata/script/vet_concrete.txt
index 6d557da..2aa0108 100644
--- a/cmd/cue/cmd/testdata/script/vet_concrete.txt
+++ b/cmd/cue/cmd/testdata/script/vet_concrete.txt
@@ -5,6 +5,7 @@
./partial.cue:4:6
b.idx: invalid non-ground value string (must be concrete int|string):
./partial.cue:7:9
+ ./partial.cue:8:7
b.str: incomplete value (string):
./partial.cue:8:7
-- partial.cue --
diff --git a/cmd/cue/cmd/testdata/script/vet_expr.txt b/cmd/cue/cmd/testdata/script/vet_expr.txt
index 5381795..dabbe47 100644
--- a/cmd/cue/cmd/testdata/script/vet_expr.txt
+++ b/cmd/cue/cmd/testdata/script/vet_expr.txt
@@ -8,7 +8,7 @@
./data.yaml:13:11
./vet.cue:3:11
field "skip" not allowed in closed struct:
- ./vet.cue:1:9
+ ./data.yaml:20:7
-- vet.cue --
File :: {
translations <_>: {
diff --git a/cue/binop.go b/cue/binop.go
index 5a3ca19..278e251 100644
--- a/cue/binop.go
+++ b/cue/binop.go
@@ -684,7 +684,7 @@
if a.optional {
continue
}
- return ctx.mkErr(src, y, "field %q not allowed in closed struct",
+ return ctx.mkErr(a.v, a.v, "field %q not allowed in closed struct",
ctx.labelStr(a.feature))
}
cp := ctx.copy(a.v)
diff --git a/cue/builtin.go b/cue/builtin.go
index d7fea05..4ce6c57 100644
--- a/cue/builtin.go
+++ b/cue/builtin.go
@@ -200,7 +200,7 @@
for iter.Next() {
d = append(d, dValue{iter.Value().path.v, false})
}
- c.ret = &disjunction{baseValue{c.src}, d, false}
+ c.ret = &disjunction{baseValue{c.src}, d, nil, false}
if len(d) == 0 {
c.ret = errors.New("empty list in call to or")
}
diff --git a/cue/debug.go b/cue/debug.go
index 1eb5bff..3742770 100644
--- a/cue/debug.go
+++ b/cue/debug.go
@@ -476,11 +476,20 @@
write("_|_")
if x.value != nil || x.format != "" {
write("(")
- if x.value != nil && p.showNodeRef {
- p.str(x.value)
- p.write(":")
+ errs := x.sub
+ if errs == nil {
+ errs = []*bottom{x}
}
- write(x.msg())
+ for i, x := range errs {
+ if i > 0 {
+ p.write(";")
+ }
+ if x.value != nil && p.showNodeRef {
+ p.str(x.value)
+ p.write(":")
+ }
+ write(x.msg())
+ }
write(")")
}
case *top:
diff --git a/cue/errors.go b/cue/errors.go
index 98af40e..f9c05ab 100644
--- a/cue/errors.go
+++ b/cue/errors.go
@@ -50,14 +50,26 @@
return e.path
}
-func (v Value) toErr(b *bottom) errors.Error {
- if b.err != nil {
- return b.err
+func (v Value) appendErr(err errors.Error, b *bottom) errors.Error {
+ switch {
+ case len(b.sub) > 0:
+ for _, b := range b.sub {
+ err = v.appendErr(err, b)
+ }
+ fallthrough
+ case b.err != nil:
+ err = errors.Append(err, b.err)
+ default:
+ err = errors.Append(err, &valueError{
+ v: v,
+ err: b,
+ })
}
- return &valueError{
- v: v,
- err: b,
- }
+ return err
+}
+
+func (v Value) toErr(b *bottom) (err errors.Error) {
+ return v.appendErr(nil, b)
}
var _ errors.Error = &valueError{}
@@ -77,7 +89,7 @@
}
func (e *valueError) InputPositions() []token.Pos {
- return e.err.Positions()
+ return e.err.Positions(e.v.ctx())
}
func (e *valueError) Msg() (string, []interface{}) {
@@ -139,33 +151,33 @@
args []interface{}
err errors.Error // pass-through from higher-level API
+ sub []*bottom // sub errors
value value
wrapped *bottom
}
func (x *bottom) kind() kind { return bottomKind }
-func (x *bottom) Positions() []token.Pos {
+func (x *bottom) Positions(ctx *context) []token.Pos {
if x.index != nil { // TODO: remove check?
- return appendPositions(nil, x.pos)
+ return appendPositions(ctx, nil, x.pos)
}
return nil
}
-func appendPositions(pos []token.Pos, src source) []token.Pos {
+func appendPositions(ctx *context, pos []token.Pos, src source) []token.Pos {
if src != nil {
- if b, ok := src.(*binaryExpr); ok {
- if _, isUnify := b.op.unifyType(); isUnify {
- pos = appendPositions(pos, b.left)
- pos = appendPositions(pos, b.right)
- }
- }
if p := src.Pos(); p != token.NoPos {
- return append(pos, src.Pos())
+ pos = append(pos, src.Pos())
}
if c := src.computed(); c != nil {
- pos = appendPositions(pos, c.x)
- pos = appendPositions(pos, c.y)
+ pos = appendPositions(ctx, pos, c.x)
+ pos = appendPositions(ctx, pos, c.y)
+ }
+ switch x := src.(type) {
+ case evaluated:
+ case value:
+ pos = appendPositions(ctx, pos, x.evalPartial(ctx))
}
}
return pos
@@ -218,6 +230,8 @@
e.code = x
case *bottom:
e.wrapped = x
+ case []*bottom:
+ e.sub = x
case errors.Error:
e.err = x
case value:
diff --git a/cue/eval.go b/cue/eval.go
index 5950ef9..ba20f4a 100644
--- a/cue/eval.go
+++ b/cue/eval.go
@@ -355,7 +355,12 @@
if len(ctx.evalStack) > 1 {
ctx.inSum++
}
- dn := &disjunction{x.baseValue, make([]dValue, 0, len(x.values)), x.hasDefaults}
+ dn := &disjunction{
+ x.baseValue,
+ make([]dValue, 0, len(x.values)),
+ make([]*bottom, 0, len(x.errors)),
+ x.hasDefaults,
+ }
changed := false
for _, v := range x.values {
n := v.val.evalPartial(ctx)
@@ -413,7 +418,7 @@
return values[0].val.evalPartial(ctx)
}
- x = &disjunction{x.baseValue, values, true}
+ x = &disjunction{x.baseValue, values, x.errors, true}
return x.normalize(ctx, x).val
}
diff --git a/cue/export.go b/cue/export.go
index 656b8ff..0604457 100644
--- a/cue/export.go
+++ b/cue/export.go
@@ -616,7 +616,18 @@
case *bottom:
err := &ast.BottomLit{}
if x.format != "" {
- comment := &ast.Comment{Text: "// " + x.msg()}
+ msg := x.msg()
+ if len(x.sub) > 0 {
+ buf := strings.Builder{}
+ for i, b := range x.sub {
+ if i > 0 {
+ buf.WriteString("; ")
+ buf.WriteString(b.msg())
+ }
+ }
+ msg = buf.String()
+ }
+ comment := &ast.Comment{Text: "// " + msg}
err.AddComment(&ast.CommentGroup{
Line: true,
Position: 2,
diff --git a/cue/export_test.go b/cue/export_test.go
index eed802c..3bf8181 100644
--- a/cue/export_test.go
+++ b/cue/export_test.go
@@ -858,6 +858,20 @@
})
})
}`),
+ }, {
+ eval: true,
+ in: `
+ a?: 1
+ b?: 2
+ b?: 2
+ c?: 3
+ c: 3`,
+ out: unindent(`
+ {
+ a?: 1
+ b?: 2
+ c: 3
+ }`),
}}
for _, tc := range testCases {
t.Run("", func(t *testing.T) {
diff --git a/cue/go.go b/cue/go.go
index b61026d..9c18208 100644
--- a/cue/go.go
+++ b/cue/go.go
@@ -668,6 +668,7 @@
values: []dValue{
{val: &nullLit{}, marked: nullIsDefault},
{val: e}},
+ errors: nil,
hasDefaults: nullIsDefault,
}
}
diff --git a/cue/resolve_test.go b/cue/resolve_test.go
index 689a75b..b46ac22 100644
--- a/cue/resolve_test.go
+++ b/cue/resolve_test.go
@@ -1270,11 +1270,11 @@
`,
out: `<0>{` +
`S :: <1>{<>: <2>(_: string)-><3>C{a: int}, }, ` +
- `a: <4>{<>: <5>(_: string)-><6>C{a: int}, v: _|_(<7>{<>: <5>(_: string)-><6>C{a: int}, v: <8>{b: int}}:field "b" not allowed in closed struct)}, ` +
- `b: <9>{<>: <10>(_: string)->(<11>C{a: int} | <12>C{b: int}), w: _|_(<13>{<>: <10>(_: string)->(<11>C{a: int} | <12>C{b: int}), w: <14>{c: int}}:empty disjunction: field "c" not allowed in closed struct)}, ` +
- `Q :: <15>{<>: <16>(_: string)->(<17>C{a: int} | <18>C{b: int}), }, ` +
- `c: <19>{<>: <20>(_: string)->[<21>C{a: int},<22>C{b: int}], w: [_|_((<23>{d: int} & close(<24>C{a: int})):field "d" not allowed in closed struct),<25>C{b: int}]}, ` +
- `R :: <26>{<>: <27>(_: string)->[<28>C{a: int},<29>C{b: int}], }}`,
+ `a: <4>{<>: <5>(_: string)-><6>C{a: int}, v: _|_(int:field "b" not allowed in closed struct)}, ` +
+ `b: <7>{<>: <8>(_: string)->(<9>C{a: int} | <10>C{b: int}), w: _|_(int:empty disjunction: field "c" not allowed in closed struct)}, ` +
+ `Q :: <11>{<>: <12>(_: string)->(<13>C{a: int} | <14>C{b: int}), }, ` +
+ `c: <15>{<>: <16>(_: string)->[<17>C{a: int},<18>C{b: int}], w: [_|_(int:field "d" not allowed in closed struct),<19>C{b: int}]}, ` +
+ `R :: <20>{<>: <21>(_: string)->[<22>C{a: int},<23>C{b: int}], }}`,
}, {
desc: "definitions with disjunctions",
in: `
@@ -1297,8 +1297,8 @@
out: `<0>{` +
`Foo :: (<1>C{field: int, a: 1} | <2>C{field: int, b: 2}), ` +
`foo: <3>C{field: int, a: 1}, ` +
- `bar: _|_((<4>.Foo & <5>{c: 2}):empty disjunction: field "c" not allowed in closed struct), ` +
- `baz: <6>C{field: int, b: 2}}`,
+ `bar: _|_(2:empty disjunction: field "c" not allowed in closed struct), ` +
+ `baz: <4>C{field: int, b: 2}}`,
}, {
desc: "definitions with disjunctions recurisive",
in: `
@@ -1787,7 +1787,7 @@
`a3: <3>{a: _|_((=~"oo" & "bar"):invalid value "bar" (does not match =~"oo")), b: =~"oo", c: =~"fo"}, ` +
`o1: <4>{a: string, b: string, c: "bar"}, ` +
`o2: <5>{a: "foo", b: string, c: "bar"}, ` +
- `o3: <6>{a: _|_((or ([<7>.b,<7>.c]) & "foo"):empty disjunction: conflicting values "baz" and "foo"), b: "baz", c: "bar"}}`,
+ `o3: <6>{a: _|_(("baz" & "foo"):empty disjunction: conflicting values "baz" and "foo";("bar" & "foo"):empty disjunction: conflicting values "bar" and "foo"), b: "baz", c: "bar"}}`,
}, {
desc: "self-reference cycles conflicts with strings",
in: `
@@ -2396,7 +2396,7 @@
x: {a:1}|{a:2}
y: x & {a:3}
`,
- out: `<0>{x: (<1>{a: 1} | <2>{a: 2}), y: _|_((<3>.x & <4>{a: 3}):empty disjunction: {a: (1 & 3)})}`,
+ out: `<0>{x: (<1>{a: 1} | <2>{a: 2}), y: _|_((1 & 3):empty disjunction: conflicting values 1 and 3;(2 & 3):empty disjunction: conflicting values 2 and 3)}`,
}, {
desc: "cannot resolve references that would be ambiguous",
in: `
diff --git a/cue/rewrite.go b/cue/rewrite.go
index 3a2b477..3f81daf 100644
--- a/cue/rewrite.go
+++ b/cue/rewrite.go
@@ -222,7 +222,7 @@
if !changed {
return x
}
- return &disjunction{x.baseValue, values, x.hasDefaults}
+ return &disjunction{x.baseValue, values, x.errors, x.hasDefaults}
}
func (x *listComprehension) rewrite(ctx *context, fn rewriteFunc) value {
diff --git a/cue/value.go b/cue/value.go
index 17ef829..495f4d5 100644
--- a/cue/value.go
+++ b/cue/value.go
@@ -1317,6 +1317,11 @@
values []dValue
+ // errors is used to keep track of all errors that occurred in
+ // a disjunction for better error reporting down the road.
+ // TODO: consider storing the errors in values.
+ errors []*bottom
+
hasDefaults bool // also true if it had elminated defaults.
// bind is the node that a successful disjunction will bind to. This
@@ -1347,6 +1352,9 @@
// add add a value to the disjunction. It is assumed not to be a disjunction.
func (x *disjunction) add(ctx *context, v value, marked bool) {
x.values = append(x.values, dValue{v, marked})
+ if b, ok := v.(*bottom); ok {
+ x.errors = append(x.errors, b)
+ }
}
// normalize removes redundant element from unification.
@@ -1370,11 +1378,12 @@
outer:
for i, v := range x.values {
// TODO: this is pre-evaluation is quite aggressive. Verify whether
- // this does not trigger structural cycles. If so, this can check for
+ // this does not trigger structural cycles (it does). If so, this can check for
// bottom and the validation can be delayed to as late as picking
// defaults. The drawback of this approach is that printed intermediate
// results will not look great.
if err := validate(ctx, v.val); err != nil {
+ x.errors = append(x.errors, err)
if v.marked {
markedErr = err
}
@@ -1419,7 +1428,7 @@
// TODO: use format instead of debugStr.
err = ctx.mkErr(src, ctx.str(err))
}
- return mVal{ctx.mkErr(src, "empty disjunction: %v", err), false}
+ return mVal{x.computeError(ctx, src), false}
case 1:
v := x.values[0]
return mVal{v.val.(evaluated), v.marked}
@@ -1429,6 +1438,45 @@
return mVal{x, false}
}
+func (x *disjunction) computeError(ctx *context, src source) evaluated {
+ var errors []*bottom
+
+ // Ensure every position is visited at least once.
+ // This prevents discriminators fields from showing up too much. A special
+ // "all errors" flag could be used to expand all errors.
+ visited := map[token.Pos]bool{}
+
+ for _, b := range x.errors {
+ positions := b.Positions(ctx)
+ if len(positions) == 0 {
+ positions = append(positions, token.NoPos)
+ }
+ // Include the error if at least one of its positions wasn't covered
+ // before.
+ done := true
+ for _, p := range positions {
+ if !visited[p] {
+ done = false
+ }
+ visited[p] = true
+ }
+ if !done {
+ b := *b
+ b.format = "empty disjunction: " + b.format
+ errors = append(errors, &b)
+ }
+ }
+ switch len(errors) {
+ case 0:
+ // Should never happen.
+ return ctx.mkErr(src, errors, "empty disjunction")
+ case 1:
+ return ctx.mkErr(src, errors, "empty disjunction: %v", errors[0])
+ default:
+ return ctx.mkErr(src, errors, "empty disjunction: %v (and %d other errors)", errors[0], len(errors)-1)
+ }
+}
+
type listComprehension struct {
baseValue
clauses yielder
diff --git a/doc/tutorial/basics/2_types/80_lists.txt b/doc/tutorial/basics/2_types/80_lists.txt
index 652f118..6f0e137 100644
--- a/doc/tutorial/basics/2_types/80_lists.txt
+++ b/doc/tutorial/basics/2_types/80_lists.txt
@@ -40,4 +40,4 @@
IP: [uint8, uint8, uint8, uint8]
PrivateIP: [10, uint8, uint8, uint8] | [192, 168, uint8, uint8] | [172, >=16 & <=32 & uint8, uint8, uint8]
myIP: [10, 2, 3, 4]
-yourIP: _|_ // empty disjunction: [((10 & (int & >=0 & int & <=255)) & 11),((int & >=0 & int & <=255) & 1),((int & >=0 & int & <=255) & 2),((int & >=0 & int & <=255) & 3)]
+yourIP: _|_ // ; empty disjunction: conflicting values 192 and 11; empty disjunction: conflicting values 172 and 11
