cue/errors: better vetting of error formats

Currently there's no way for `go vet` to know that the arguments passed
to `errors.NewMessage` are intended for use by `fmt.Printf`.

This CL uses a never-called invocation of `fmt.Sprintf` to do that.
It also renames `NewMessage` to `NewMessagef` (deprecating the
original function) because `go vet` does not recognize printf-like
functions unless they have variadic arguments.

Also fix the various erroneous calls which were exposed by this change.
This includes changing `%s` to `%v` whereever the error argument transformation
code in `adt.OpContext.NewPosf` changes the type. This should not
affect behavior because `%v` acts the same as `%s` for `string`.

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: I0f4e8751db862ef822c4b5b16c8084d7a0cf66c6
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/551766
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Marcel van Lohuizen <mpvl@gmail.com>
Unity-Result: CUEcueckoo <cueckoo@cuelang.org>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>

cue/errors/errors.go

@@ -74,13 +74,23 @@ type Message struct {
 	args   []interface{}
 }
 
-// NewMessage creates an error message for human consumption. The arguments
+// NewMessagef creates an error message for human consumption. The arguments
 // are for later consumption, allowing the message to be localized at a later
 // time. The passed argument list should not be modified.
-func NewMessage(format string, args []interface{}) Message {
+func NewMessagef(format string, args ...interface{}) Message {
+	if false {
+		// Let go vet know that we're expecting printf-like arguments.
+		_ = fmt.Sprintf(format, args...)
+	}
 	return Message{format: format, args: args}
 }
 
+// NewMessage creates an error message for human consumption.
+// Deprecated: Use NewMessagef instead.
+func NewMessage(format string, args []interface{}) Message {
+	return NewMessagef(format, args...)
+}
+
 // Msg returns a printf-style format string and its arguments for human
 // consumption.
 func (m *Message) Msg() (format string, args []interface{}) {
@@ -162,7 +172,7 @@ func Path(err error) []string {
 func Newf(p token.Pos, format string, args ...interface{}) Error {
 	return &posError{
 		pos:     p,
-		Message: NewMessage(format, args),
+		Message: NewMessagef(format, args...),
 	}
 }
 
@@ -171,7 +181,7 @@ func Newf(p token.Pos, format string, args ...interface{}) Error {
 func Wrapf(err error, p token.Pos, format string, args ...interface{}) Error {
 	pErr := &posError{
 		pos:     p,
-		Message: NewMessage(format, args),
+		Message: NewMessagef(format, args...),
 	}
 	return Wrap(pErr, err)
 }

cue/load/import.go

@@ -120,8 +120,7 @@ func (l *loader) importPkg(pos token.Pos, p *build.Instance) []*build.Instance {
 	if !found {
 		return retErr(
 			&PackageError{
-				Message: errors.NewMessage("cannot find package %q",
-					[]interface{}{p.DisplayPath}),
+				Message: errors.NewMessagef("cannot find package %q", p.DisplayPath),
 			})
 	}
 

cue/load/loader.go

@@ -68,7 +68,7 @@ func (l *loader) abs(filename string) string {
 func (l *loader) errPkgf(importPos []token.Pos, format string, args ...interface{}) *PackageError {
 	err := &PackageError{
 		ImportStack: l.stk.Copy(),
-		Message:     errors.NewMessage(format, args),
+		Message:     errors.NewMessagef(format, args...),
 	}
 	err.fillPos(l.cfg.Dir, importPos)
 	return err

encoding/jsonschema/constraints.go

@@ -240,7 +240,7 @@ var constraints = []*constraint{
 
 	p1("examples", func(n cue.Value, s *state) {
 		if n.Kind() != cue.ListKind {
-			s.errf(n, `value of "examples" must be an array, found %v`, n.Kind)
+			s.errf(n, `value of "examples" must be an array, found %v`, n.Kind())
 		}
 		// TODO: implement examples properly.
 		// for _, n := range s.listItems("examples", n, true) {
@@ -488,7 +488,7 @@ var constraints = []*constraint{
 
 	p2("required", func(n cue.Value, s *state) {
 		if n.Kind() != cue.ListKind {
-			s.errf(n, `value of "required" must be list of strings, found %v`, n.Kind)
+			s.errf(n, `value of "required" must be list of strings, found %v`, n.Kind())
 			return
 		}
 
@@ -573,7 +573,7 @@ var constraints = []*constraint{
 	p2("patternProperties", func(n cue.Value, s *state) {
 		s.usedTypes |= cue.StructKind
 		if n.Kind() != cue.StructKind {
-			s.errf(n, `value of "patternProperties" must be an an object, found %v`, n.Kind)
+			s.errf(n, `value of "patternProperties" must be an an object, found %v`, n.Kind())
 		}
 		obj := s.object(n)
 		existing := excludeFields(s.obj.Elts)

encoding/openapi/build.go

@@ -230,7 +230,7 @@ func (c *buildContext) isInternal(sel cue.Selector) bool {
 
 func (b *builder) failf(v cue.Value, format string, args ...interface{}) {
 	panic(&openapiError{
-		errors.NewMessage(format, args),
+		errors.NewMessagef(format, args...),
 		cue.MakePath(b.ctx.path...),
 		v.Pos(),
 	})

internal/core/adt/composite.go

@@ -837,10 +837,10 @@ func (v *Vertex) GetArc(c *OpContext, f Feature, t ArcType) (arc *Vertex, isNew
 		// TODO(errors): add positions.
 		if f.IsInt() {
 			c.addErrf(EvalError, token.NoPos,
-				"element at index %s not allowed by earlier comprehension or reference cycle", f)
+				"element at index %v not allowed by earlier comprehension or reference cycle", f)
 		} else {
 			c.addErrf(EvalError, token.NoPos,
-				"field %s not allowed by earlier comprehension or reference cycle", f)
+				"field %v not allowed by earlier comprehension or reference cycle", f)
 		}
 	}
 	arc = &Vertex{Parent: v, Label: f, ArcType: t}

internal/core/adt/context.go

@@ -575,7 +575,7 @@ func (c *OpContext) getDefault(v Value) (result Value, ok bool) {
 
 	if d.NumDefaults != 1 {
 		c.addErrf(IncompleteError, c.pos(),
-			"unresolved disjunction %s (type %s)", d, d.Kind())
+			"unresolved disjunction %v (type %s)", d, d.Kind())
 		return nil, false
 	}
 	return c.getDefault(d.Values[0])
@@ -805,7 +805,7 @@ func (c *OpContext) lookup(x *Vertex, pos token.Pos, l Feature, state vertexStat
 	switch x.BaseValue.(type) {
 	case *StructMarker:
 		if l.Typ() == IntLabel {
-			c.addErrf(0, pos, "invalid struct selector %s (type int)", l)
+			c.addErrf(0, pos, "invalid struct selector %v (type int)", l)
 			return nil
 		}
 
@@ -814,17 +814,17 @@ func (c *OpContext) lookup(x *Vertex, pos token.Pos, l Feature, state vertexStat
 		case l.Typ() == IntLabel:
 			switch {
 			case l.Index() < 0:
-				c.addErrf(0, pos, "invalid list index %s (index must be non-negative)", l)
+				c.addErrf(0, pos, "invalid list index %v (index must be non-negative)", l)
 				return nil
 			case l.Index() > len(x.Arcs):
-				c.addErrf(0, pos, "invalid list index %s (out of bounds)", l)
+				c.addErrf(0, pos, "invalid list index %v (out of bounds)", l)
 				return nil
 			}
 
 		case l.IsDef(), l.IsHidden(), l.IsLet():
 
 		default:
-			c.addErrf(0, pos, "invalid list index %s (type string)", l)
+			c.addErrf(0, pos, "invalid list index %v (type string)", l)
 			return nil
 		}
 
@@ -843,7 +843,7 @@ func (c *OpContext) lookup(x *Vertex, pos token.Pos, l Feature, state vertexStat
 			// return nil
 		} else if !l.IsDef() && !l.IsHidden() && !l.IsLet() {
 			c.addErrf(0, pos,
-				"invalid selector %s for value of type %s", l, kind)
+				"invalid selector %v for value of type %s", l, kind)
 			return nil
 		}
 	}
@@ -1121,7 +1121,7 @@ func (c *OpContext) uint64(v Value, as string) uint64 {
 	}
 	if !x.X.Coeff.IsUint64() {
 		// TODO: improve message
-		c.AddErrf("cannot convert number %s to uint64", x.X)
+		c.AddErrf("cannot convert number %s to uint64", &x.X)
 		return 0
 	}
 	return x.X.Coeff.Uint64()

internal/core/adt/errors.go

@@ -301,7 +301,7 @@ func (c *OpContext) NewPosf(p token.Pos, format string, args ...interface{}) *Va
 		v:       c.errNode(),
 		pos:     p,
 		auxpos:  a,
-		Message: errors.NewMessage(format, args),
+		Message: errors.NewMessagef(format, args...),
 	}
 }
 

internal/core/adt/expr.go

@@ -1215,7 +1215,7 @@ func (x *UnaryExpr) evaluate(c *OpContext, state vertexStatus) Value {
 			"operand %s of '%s' not concrete (was %s)", x.X, op, k)
 		return nil
 	}
-	return c.NewErrf("invalid operation %s (%s %s)", x, op, k)
+	return c.NewErrf("invalid operation %v (%s %s)", x, op, k)
 }
 
 // BinaryExpr is a binary expression.
@@ -1566,15 +1566,15 @@ func (x *Builtin) call(c *OpContext, p token.Pos, validate bool, args []Value) E
 	fun := x // right now always x.
 	if len(args) > len(x.Params) {
 		c.addErrf(0, p,
-			"too many arguments in call to %s (have %d, want %d)",
+			"too many arguments in call to %v (have %d, want %d)",
 			fun, len(args), len(x.Params))
 		return nil
 	}
 	for i := len(args); i < len(x.Params); i++ {
 		v := x.Params[i].Default()
 		if v == nil {
 			c.addErrf(0, p,
-				"not enough arguments in call to %s (have %d, want %d)",
+				"not enough arguments in call to %v (have %d, want %d)",
 				fun, len(args), len(x.Params))
 			return nil
 		}
@@ -1594,7 +1594,7 @@ func (x *Builtin) call(c *OpContext, p token.Pos, validate bool, args []Value) E
 				code = b.Code
 			}
 			c.addErrf(code, pos(a),
-				"cannot use %s (type %s) as %s in argument %d to %s",
+				"cannot use %s (type %s) as %s in argument %d to %v",
 				a, k, x.Params[i].Kind(), i+1, fun)
 			return nil
 		}
@@ -1609,7 +1609,7 @@ func (x *Builtin) call(c *OpContext, p token.Pos, validate bool, args []Value) E
 			n.Finalize(c)
 			if _, ok := n.BaseValue.(*Bottom); ok {
 				c.addErrf(0, pos(a),
-					"cannot use %s as %s in argument %d to %s",
+					"cannot use %s as %s in argument %d to %v",
 					a, v, i+1, fun)
 				return nil
 			}

internal/core/adt/feature.go

@@ -258,10 +258,10 @@ func labelFromValue(c *OpContext, src Expr, v Value) Feature {
 			case nil, *Num, *UnaryExpr:
 				// If the value is a constant, we know it is always an error.
 				// UnaryExpr is an approximation for a constant value here.
-				c.AddErrf("invalid index %s (index must be non-negative)", x)
+				c.AddErrf("invalid index %v (index must be non-negative)", x)
 			default:
 				// Use a different message is it is the result of evaluation.
-				c.AddErrf("index %s out of range [%s]", src, x)
+				c.AddErrf("index %v out of range [%v]", src, x)
 			}
 			return InvalidLabel
 		}

internal/core/compile/compile.go

@@ -133,7 +133,7 @@ func (c *compiler) errf(n ast.Node, format string, args ...interface{}) *adt.Bot
 	err := &compilerError{
 		n:       n,
 		path:    c.path(),
-		Message: errors.NewMessage(format, args),
+		Message: errors.NewMessagef(format, args...),
 	}
 	c.errs = errors.Append(c.errs, err)
 	return &adt.Bottom{Err: err}

internal/core/runtime/errors.go

@@ -37,7 +37,7 @@ func (n *nodeError) Error() string {
 func nodeErrorf(n ast.Node, format string, args ...interface{}) *nodeError {
 	return &nodeError{
 		n:       n,
-		Message: errors.NewMessage(format, args),
+		Message: errors.NewMessagef(format, args...),
 	}
 }
 

internal/core/subsume/vertex.go

@@ -55,7 +55,7 @@ func (s *subsumer) vertices(x, y *adt.Vertex) bool {
 
 	case *adt.ListMarker:
 		if !y.IsList() {
-			s.errf("list does not subsume %s (type %s)", y, y.Kind())
+			s.errf("list does not subsume %v (type %s)", y, y.Kind())
 			return false
 		}
 		if !s.listVertices(x, y) {
@@ -145,7 +145,7 @@ func (s *subsumer) vertices(x, y *adt.Vertex) bool {
 		if b == nil {
 			// y.f is optional
 			if !aOpt {
-				s.errf("required field is optional in subsumed value: %s", f)
+				s.errf("required field is optional in subsumed value: %v", f)
 				return false
 			}
 
@@ -170,7 +170,7 @@ func (s *subsumer) vertices(x, y *adt.Vertex) bool {
 		s.gt = a
 		s.lt = y
 
-		s.errf("field %s not present in %s", f, y)
+		s.errf("field %v not present in %v", f, y)
 		return false
 	}
 
@@ -211,7 +211,7 @@ outer:
 			if s.Profile.IgnoreClosedness {
 				continue
 			}
-			s.errf("field not allowed in closed struct: %s", f)
+			s.errf("field not allowed in closed struct: %v", f)
 			return false
 		}
 

internal/task/task.go

@@ -84,7 +84,7 @@ func (c *Context) addErr(v cue.Value, wrap error, format string, args ...interfa
 	err := &taskError{
 		task:    c.Obj,
 		v:       v,
-		Message: errors.NewMessage(format, args),
+		Message: errors.NewMessagef(format, args...),
 	}
 	c.Err = errors.Append(c.Err, errors.Wrap(err, wrap))
 }

pkg/internal/context.go

@@ -295,14 +295,14 @@ func (c *CallCtxt) DecimalList(i int) (a []*apd.Decimal) {
 		default:
 			if k := w.Kind(); k&adt.NumKind == 0 {
 				err := c.ctx.NewErrf(
-					"invalid list element %d in argument %d to call: cannot use value %s (%s) as number",
+					"invalid list element %d in argument %d to call: cannot use value %v (%s) as number",
 					j, i, w, k)
 				c.Err = &callError{err}
 				return a
 			}
 
 			err := c.ctx.NewErrf(
-				"non-concrete value %s for element %d of number list argument %d",
+				"non-concrete value %v for element %d of number list argument %d",
 				w, j, i)
 			err.Code = adt.IncompleteError
 			c.Err = &callError{err}
@@ -333,14 +333,14 @@ func (c *CallCtxt) StringList(i int) (a []string) {
 		default:
 			if k := w.Kind(); k&adt.StringKind == 0 {
 				err := c.ctx.NewErrf(
-					"invalid list element %d in argument %d to call: cannot use value %s (%s) as string",
+					"invalid list element %d in argument %d to call: cannot use value %v (%s) as string",
 					j, i, w, k)
 				c.Err = &callError{err}
 				return a
 			}
 
 			err := c.ctx.NewErrf(
-				"non-concrete value %s for element %d of string list argument %d",
+				"non-concrete value %v for element %d of string list argument %d",
 				w, j, i)
 			err.Code = adt.IncompleteError
 			c.Err = &callError{err}