internal/core/adt: add path location for majority of errors
Change-Id: I8a7b19851af88ecd09ae9032d659ade3570b73e3
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/6650
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
diff --git a/internal/core/adt/binop.go b/internal/core/adt/binop.go
index 2fb3205..7edd191 100644
--- a/internal/core/adt/binop.go
+++ b/internal/core/adt/binop.go
@@ -19,7 +19,6 @@
"math/big"
"strings"
- "cuelang.org/go/cue/errors"
"github.com/cockroachdb/apd/v2"
)
@@ -42,13 +41,13 @@
if left.Concreteness() > Concrete {
return &Bottom{
Code: IncompleteError,
- Err: errors.Newf(c.pos(), msg, c.Str(left), op),
+ Err: c.Newf(msg, c.Str(left), op),
}
}
if right.Concreteness() > Concrete {
return &Bottom{
Code: IncompleteError,
- Err: errors.Newf(c.pos(), msg, c.Str(right), op),
+ Err: c.Newf(msg, c.Str(right), op),
}
}
diff --git a/internal/core/adt/context.go b/internal/core/adt/context.go
index 19955ec..f54fd72 100644
--- a/internal/core/adt/context.go
+++ b/internal/core/adt/context.go
@@ -93,6 +93,7 @@
}
ctx := &OpContext{
config: *cfg,
+ vertex: v,
}
if v != nil {
ctx.e = &Environment{Up: nil, Vertex: v}
@@ -110,6 +111,11 @@
src ast.Node
errs *Bottom
+ // vertex is used to determine the path location in case of error. Turning
+ // this into a stack could also allow determining the cyclic path for
+ // structural cycle errors.
+ vertex *Vertex
+
// TODO: remove use of tentative. Should be possible if incomplete
// handling is done better.
tentative int // set during comprehension evaluation
@@ -219,7 +225,7 @@
}
}
- err := errors.Newf(pos, msg, args...)
+ err := c.NewPosf(pos, msg, args...)
c.addErr(code, err)
}
@@ -243,13 +249,15 @@
// NewErrf creates a *Bottom value and returns it. The returned uses the
// current source as the point of origin of the error.
func (c *OpContext) NewErrf(format string, args ...interface{}) *Bottom {
- err := errors.Newf(c.pos(), format, args...)
+ // TODO: consider renaming ot NewBottomf: this is now confusing as we also
+ // have Newf.
+ err := c.Newf(format, args...)
return &Bottom{Src: c.src, Err: err, Code: EvalError}
}
// AddErrf records an error in OpContext. It returns errors collected so far.
func (c *OpContext) AddErrf(format string, args ...interface{}) *Bottom {
- return c.AddErr(errors.Newf(c.pos(), format, args...))
+ return c.AddErr(c.Newf(format, args...))
}
func (c *OpContext) validate(v Value) *Bottom {
@@ -295,6 +303,19 @@
return err
}
+// PushArc signals c that arc v is currently being processed for the purpose
+// of error reporting. PopArc should be called with the returned value once
+// processing of v is completed.
+func (c *OpContext) PushArc(v *Vertex) (saved *Vertex) {
+ c.vertex, saved = v, c.vertex
+ return saved
+}
+
+// PopArc signals completion of processing the current arc.
+func (c *OpContext) PopArc(saved *Vertex) {
+ c.vertex = saved
+}
+
// Resolve finds a node in the tree.
//
// Should only be used to insert Conjuncts. TODO: perhaps only return Conjuncts
@@ -315,7 +336,15 @@
// Validate calls validates value for the given validator.
func (c *OpContext) Validate(check Validator, value Value) *Bottom {
- return check.validate(c, value)
+ // TODO: use a position stack to push both values.
+ saved := c.src
+ c.src = check.Source()
+
+ err := check.validate(c, value)
+
+ c.src = saved
+
+ return err
}
// Yield evaluates a Yielder and calls f for each result.
@@ -404,7 +433,7 @@
// TODO ENSURE THIS DOESN"T HAPPEN>
val = &Bottom{
Code: IncompleteError,
- Err: errors.Newf(token.NoPos, "UNANTICIPATED ERROR"),
+ Err: c.Newf("UNANTICIPATED ERROR"),
}
}
diff --git a/internal/core/adt/errors.go b/internal/core/adt/errors.go
index ecfe359..ec73924 100644
--- a/internal/core/adt/errors.go
+++ b/internal/core/adt/errors.go
@@ -34,6 +34,7 @@
import (
"cuelang.org/go/cue/ast"
"cuelang.org/go/cue/errors"
+ "cuelang.org/go/cue/token"
)
// ErrorCode indicates the type of error. The type of error may influence
@@ -199,3 +200,53 @@
Code: a.Code,
}
}
+
+// A valueError is returned as a result of evaluating a value.
+type valueError struct {
+ r Runtime
+ v *Vertex
+ pos []token.Pos
+ errors.Message
+}
+
+func (c *OpContext) errNode() *Vertex {
+ return c.vertex
+}
+
+func (c *OpContext) Newf(format string, args ...interface{}) *valueError {
+ return c.NewPosf(c.pos(), format, args...)
+}
+
+func (c *OpContext) NewPosf(pos token.Pos, format string, args ...interface{}) *valueError {
+ return &valueError{
+ r: c.Runtime,
+ v: c.errNode(),
+ // TODO: leave ni if it can be derived from the source and save an
+ // allocation.
+ pos: []token.Pos{pos},
+ Message: errors.NewMessage(format, args),
+ }
+}
+
+func (e *valueError) Error() string {
+ return errors.String(e)
+}
+
+func (e *valueError) Position() token.Pos {
+ if len(e.pos) == 0 {
+ // TODO: retrieve from source
+ return token.NoPos
+ }
+ return e.pos[0]
+}
+
+func (e *valueError) InputPositions() []token.Pos {
+ return e.pos
+}
+
+func (e *valueError) Path() (a []string) {
+ for _, f := range appendPath(nil, e.v) {
+ a = append(a, f.SelectorString(e.r))
+ }
+ return a
+}
diff --git a/internal/core/adt/simplify.go b/internal/core/adt/simplify.go
index aa3eb58..cc35653 100644
--- a/internal/core/adt/simplify.go
+++ b/internal/core/adt/simplify.go
@@ -151,7 +151,7 @@
fallthrough
case d.Negative:
- return ctx.NewErrf("bounds %v %v", ctx.Str(x), ctx.Str(y))
+ return ctx.NewErrf("incompatible bounds %v and %v", ctx.Str(x), ctx.Str(y))
}
case x.Op == NotEqualOp:
diff --git a/internal/core/convert/go.go b/internal/core/convert/go.go
index f84ff9f..36cf236 100644
--- a/internal/core/convert/go.go
+++ b/internal/core/convert/go.go
@@ -305,7 +305,7 @@
case errors.Error:
errs = x
default:
- errs = errors.Newf(token.NoPos, "%s", x.Error())
+ errs = ctx.Newf("%s", x.Error())
}
return &adt.Bottom{Err: errs}
case bool:
diff --git a/internal/core/eval/closed.go b/internal/core/eval/closed.go
index b2201fd..80f6ed8 100644
--- a/internal/core/eval/closed.go
+++ b/internal/core/eval/closed.go
@@ -42,8 +42,6 @@
// in eval.go.
import (
- "cuelang.org/go/cue/errors"
- "cuelang.org/go/cue/token"
"cuelang.org/go/internal/core/adt"
)
@@ -320,9 +318,7 @@
filter := f.IsString() || f == adt.InvalidLabel
if filter && !n.verifyArcRecursive(ctx, n.tree, f) {
label := f.SelectorString(ctx)
- return &adt.Bottom{
- Err: errors.Newf(token.NoPos, "field `%s` not allowed", label),
- }
+ return ctx.NewErrf("field `%s` not allowed", label)
}
return nil
}
diff --git a/internal/core/eval/disjunct.go b/internal/core/eval/disjunct.go
index db5df3c..973afcc 100644
--- a/internal/core/eval/disjunct.go
+++ b/internal/core/eval/disjunct.go
@@ -17,8 +17,6 @@
import (
"sort"
- "cuelang.org/go/cue/errors"
- "cuelang.org/go/cue/token"
"cuelang.org/go/internal/core/adt"
)
@@ -214,7 +212,7 @@
// really we should keep track of the errors and return a more
// accurate result here.
Code: adt.IncompleteError,
- Err: errors.Newf(token.NoPos, "empty disjunction"),
+ Err: n.ctx.Newf("empty disjunction"),
}
n.node.AddErr(n.ctx, b)
}
diff --git a/internal/core/eval/eval.go b/internal/core/eval/eval.go
index 591e179..a6444c7 100644
--- a/internal/core/eval/eval.go
+++ b/internal/core/eval/eval.go
@@ -325,6 +325,8 @@
}
saved := *v
+ defer c.PopArc(c.PushArc(v))
+
e.stats.UnifyCount++
for i := 0; ; i++ {
e.stats.DisjunctCount++
@@ -474,10 +476,8 @@
markStruct := false
if n.isStruct {
if kind != 0 && kind&adt.StructKind == 0 {
- n.node.Value = &adt.Bottom{
- Err: errors.Newf(token.NoPos,
- "conflicting values struct and %s", n.kind),
- }
+ n.node.Value = n.ctx.NewErrf(
+ "conflicting values struct and %s", n.kind)
}
markStruct = true
} else if len(n.node.Structs) > 0 {
@@ -497,9 +497,10 @@
if src := v.Source(); src != nil {
p = src.Pos()
}
- n.addErr(errors.Newf(p,
- // TODO(err): position of all value types.
- "conflicting types",
+ n.addErr(ctx.NewPosf(p,
+ // TODO(errors): position of all value types.
+ "conflicting types %v and %v",
+ v.Kind(), n.kind,
))
}
if n.lowerBound != nil {
@@ -533,9 +534,9 @@
case v.Kind() == adt.ListKind:
for _, a := range n.node.Arcs {
if a.Label.Typ() == adt.StringLabel {
- n.addErr(errors.Newf(token.NoPos,
- // TODO(err): add positions for list and arc definitions.
- "list may not have regular fields"))
+ n.addErr(ctx.Newf("list may not have regular fields"))
+ // TODO(errors): add positions for list and arc definitions.
+
}
}
@@ -543,7 +544,7 @@
// for _, a := range n.node.Arcs {
// if a.Label.IsRegular() {
// n.addErr(errors.Newf(token.NoPos,
- // // TODO(err): add positions of non-struct values and arcs.
+ // // TODO(errors): add positions of non-struct values and arcs.
// "cannot combine scalar values with arcs"))
// }
// }
@@ -594,9 +595,8 @@
if accept := n.nodeShared.accept; accept != nil {
if !accept.Accept(n.ctx, a.Label) {
label := a.Label.SelectorString(ctx)
- n.node.Value = &adt.Bottom{
- Err: errors.Newf(token.NoPos, "field `%s` not allowed by Acceptor", label),
- }
+ n.node.Value = ctx.NewErrf(
+ "field `%s` not allowed by Acceptor", label)
}
} else if err := m.verifyArcAllowed(n.ctx, a.Label); err != nil {
n.node.Value = err
@@ -619,7 +619,7 @@
if cyclic {
n.node.Value = adt.CombineErrors(nil, n.node.Value, &adt.Bottom{
Code: adt.StructuralCycleError,
- Err: errors.Newf(token.NoPos, "structural cycle"),
+ Err: ctx.Newf("structural cycle"),
Value: n.node.Value,
// TODO: probably, this should have the referenced arc.
})
@@ -1190,7 +1190,7 @@
kind := n.kind & v.Kind()
if kind == adt.BottomKind {
// TODO: how to get other conflicting values?
- n.addErr(errors.Newf(token.NoPos,
+ n.addErr(ctx.Newf(
"invalid value %s (mismatched types %s and %s)",
ctx.Str(v), v.Kind(), n.kind))
return
@@ -1245,7 +1245,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(errors.Newf(ctx.Pos(), "incompatible values %s and %s", ctx.Str(x), ctx.Str(y)))
+ n.addErr(ctx.Newf("incompatible values %s and %s", ctx.Str(x), ctx.Str(y)))
}
// TODO: do we need to explicitly add again?
// n.scalar = nil
@@ -1372,7 +1372,7 @@
}
if hasBulk != nil && hasOther != nil {
- n.addErr(errors.Newf(token.NoPos, "cannot mix bulk optional fields with dynamic fields, embeddings, or comprehensions within the same struct"))
+ n.addErr(ctx.Newf("cannot mix bulk optional fields with dynamic fields, embeddings, or comprehensions within the same struct"))
}
// Apply existing fields
@@ -1421,7 +1421,7 @@
// processed. We could instead detect such insertion and feed it to the
// ForClause to generate another entry or have the for clause be recomputed.
// This seems to be too complicated and lead to iffy edge cases.
-// TODO(error): detect when a field is added to a struct that is already used
+// TODO(errors): detect when a field is added to a struct that is already used
// in a for clause.
func (n *nodeContext) expandOne() (done bool) {
if n.done() {
@@ -1548,7 +1548,7 @@
for _, a := range n.node.Arcs {
if t := a.Label.Typ(); t == adt.StringLabel {
- n.addErr(errors.Newf(token.NoPos, "conflicting types list and struct"))
+ n.addErr(c.Newf("conflicting types list and struct"))
}
}
@@ -1616,8 +1616,7 @@
case *adt.Ellipsis:
if j != len(l.list.Elems)-1 {
- n.addErr(errors.Newf(token.NoPos,
- "ellipsis must be last element in list"))
+ n.addErr(c.Newf("ellipsis must be last element in list"))
}
n.lists[i].elipsis = x
@@ -1709,6 +1708,5 @@
}
func (n *nodeContext) invalidListLength(na, nb int, a, b adt.Expr) {
- n.addErr(errors.Newf(n.ctx.Pos(),
- "incompatible list lengths (%d and %d)", na, nb))
+ n.addErr(n.ctx.Newf("incompatible list lengths (%d and %d)", na, nb))
}
diff --git a/internal/core/eval/eval_test.go b/internal/core/eval/eval_test.go
index 96197b1..68eb20f 100644
--- a/internal/core/eval/eval_test.go
+++ b/internal/core/eval/eval_test.go
@@ -62,7 +62,7 @@
t.Log(e.Stats())
- if b := validate.Validate(r, v, &validate.Config{
+ if b := validate.Validate(ctx, v, &validate.Config{
AllErrors: true,
}); b != nil {
fmt.Fprintln(t, "Errors:")
diff --git a/internal/core/export/testdata/adt.txtar b/internal/core/export/testdata/adt.txtar
index aaabe3f..49e7fbc 100644
--- a/internal/core/export/testdata/adt.txtar
+++ b/internal/core/export/testdata/adt.txtar
@@ -191,8 +191,8 @@
[x]
-- out/value --
== Simplified
-_|_ // undefined field 2
+_|_ // e3: undefined field 2
== Raw
-_|_ // undefined field 2
+_|_ // e3: undefined field 2
== All
-_|_ // undefined field 2
+_|_ // e3: undefined field 2
diff --git a/internal/core/validate/validate.go b/internal/core/validate/validate.go
index 2f20f82..ecf85fb 100644
--- a/internal/core/validate/validate.go
+++ b/internal/core/validate/validate.go
@@ -12,13 +12,11 @@
// See the License for the specific language governing permissions and
// limitations under the License.
+// Package validate collects errors from an evaluated Vertex.
package validate
import (
- "cuelang.org/go/cue/errors"
- "cuelang.org/go/cue/token"
"cuelang.org/go/internal/core/adt"
- "cuelang.org/go/internal/core/debug"
)
type Config struct {
@@ -36,20 +34,20 @@
// Validate checks that a value has certain properties. The value must have
// been evaluated.
-func Validate(r adt.Runtime, v *adt.Vertex, cfg *Config) *adt.Bottom {
+func Validate(ctx *adt.OpContext, v *adt.Vertex, cfg *Config) *adt.Bottom {
if cfg == nil {
cfg = &Config{}
}
- x := validator{Config: *cfg, runtime: r}
+ x := validator{Config: *cfg, ctx: ctx}
x.validate(v)
return x.err
}
type validator struct {
Config
+ ctx *adt.OpContext
err *adt.Bottom
inDefinition int
- runtime adt.Runtime
}
func (v *validator) add(b *adt.Bottom) {
@@ -63,6 +61,8 @@
}
func (v *validator) validate(x *adt.Vertex) {
+ defer v.ctx.PopArc(v.ctx.PushArc(x))
+
if b, _ := x.Value.(*adt.Bottom); b != nil {
switch b.Code {
case adt.CycleError:
@@ -83,15 +83,10 @@
}
} else if v.Concrete && v.inDefinition == 0 && !adt.IsConcrete(x) {
- p := token.NoPos
- if src := x.Value.Source(); src != nil {
- p = src.Pos()
- }
// TODO: use ValueError to get full path.
v.add(&adt.Bottom{
Code: adt.IncompleteError,
- Err: errors.Newf(p, "incomplete value %v",
- debug.NodeString(v.runtime, x.Value, nil)),
+ Err: v.ctx.Newf("incomplete value %v", v.ctx.Str(x.Value)),
})
}
diff --git a/internal/core/validate/validate_test.go b/internal/core/validate/validate_test.go
index ab70286..4813ef5 100644
--- a/internal/core/validate/validate_test.go
+++ b/internal/core/validate/validate_test.go
@@ -42,7 +42,7 @@
#foo: { use: string }
`,
lookup: "#foo",
- out: "incomplete\nincomplete value string",
+ out: "incomplete\n#foo.use: incomplete value string",
}, {
desc: "definitions not considered for completeness",
cfg: &Config{Concrete: true},
@@ -71,14 +71,14 @@
in: `
x: 1 & 2
`,
- out: "eval\nincompatible values 2 and 1",
+ out: "eval\nx: incompatible values 2 and 1",
}, {
desc: "first error",
in: `
x: 1 & 2
y: 2 & 4
`,
- out: "eval\nincompatible values 2 and 1",
+ out: "eval\nx: incompatible values 2 and 1",
}, {
desc: "all errors",
cfg: &Config{AllErrors: true},
@@ -87,8 +87,8 @@
y: 2 & 4
`,
out: `eval
-incompatible values 2 and 1
-incompatible values 4 and 2`,
+x: incompatible values 2 and 1
+y: incompatible values 4 and 2`,
}, {
desc: "incomplete",
cfg: &Config{Concrete: true},
@@ -96,7 +96,7 @@
y: 2 + x
x: string
`,
- out: "incomplete\nnon-concrete value string in operand to +:\n test:2:6",
+ out: "incomplete\ny: non-concrete value string in operand to +:\n test:2:6",
}, {
desc: "allowed incomplete cycle",
in: `
@@ -142,7 +142,7 @@
y: string
x: 1 & 2
`,
- out: "eval\nincompatible values 2 and 1",
+ out: "eval\nx: incompatible values 2 and 1",
}}
r := runtime.New()
@@ -163,7 +163,7 @@
v = v.Lookup(adt.MakeIdentLabel(r, tc.lookup))
}
- b := Validate(r, v, tc.cfg)
+ b := Validate(ctx, v, tc.cfg)
w := &strings.Builder{}
if b != nil {
