Refactor Error handling to prevent PE escapes

By refactoring the code this way, we only deal with errors when we have
errors rather than by default, and also that prevents path element
content to escape to the heap, which significantly decreases the amount
of allocations and improves performance.

Author: Antoine Pelisse

typed/helpers.go

@@ -28,7 +28,7 @@ import (
 
 // ValidationError reports an error about a particular field
 type ValidationError struct {
-	Path         fieldpath.Path
+	Path         string
 	ErrorMessage string
 }
 
@@ -56,58 +56,39 @@ func (errs ValidationErrors) Error() string {
 	return strings.Join(messages, "\n")
 }
 
-// errorFormatter makes it easy to keep a list of validation errors. They
-// should all be packed into a single error object before leaving the package
-// boundary, since it's weird to have functions not return a plain error type.
-type errorFormatter struct {
-	path fieldpath.Path
-}
-
-func (ef *errorFormatter) descend(pe fieldpath.PathElement) {
-	ef.path = append(ef.path, pe)
+// Set the given path to all the validation errors.
+func (errs ValidationErrors) WithPath(p string) ValidationErrors {
+	for i := range errs {
+		errs[i].Path = p
+	}
+	return errs
 }
 
-// parent returns the parent, for the purpose of buffer reuse. It's an error to
-// call this if there is no parent.
-func (ef *errorFormatter) parent() errorFormatter {
-	return errorFormatter{
-		path: ef.path[:len(ef.path)-1],
+// Prefix all errors path with the given pathelement. This is useful
+// when unwinding the stack on errors.
+func (errs ValidationErrors) WithPrefix(prefix string) ValidationErrors {
+	for i := range errs {
+		errs[i].Path = prefix + errs[i].Path
 	}
+	return errs
 }
 
-func (ef errorFormatter) errorf(format string, args ...interface{}) ValidationErrors {
+func errorf(format string, args ...interface{}) ValidationErrors {
 	return ValidationErrors{{
-		Path:         append(fieldpath.Path{}, ef.path...),
 		ErrorMessage: fmt.Sprintf(format, args...),
 	}}
 }
 
-func (ef errorFormatter) error(err error) ValidationErrors {
-	return ValidationErrors{{
-		Path:         append(fieldpath.Path{}, ef.path...),
-		ErrorMessage: err.Error(),
-	}}
-}
-
-func (ef errorFormatter) prefixError(prefix string, err error) ValidationErrors {
-	return ValidationErrors{{
-		Path:         append(fieldpath.Path{}, ef.path...),
-		ErrorMessage: prefix + err.Error(),
-	}}
-}
-
 type atomHandler interface {
 	doScalar(*schema.Scalar) ValidationErrors
 	doList(*schema.List) ValidationErrors
 	doMap(*schema.Map) ValidationErrors
-
-	errorf(msg string, args ...interface{}) ValidationErrors
 }
 
 func resolveSchema(s *schema.Schema, tr schema.TypeRef, v *value.Value, ah atomHandler) ValidationErrors {
 	a, ok := s.Resolve(tr)
 	if !ok {
-		return ah.errorf("schema error: no type found matching: %v", *tr.NamedType)
+		return errorf("schema error: no type found matching: %v", *tr.NamedType)
 	}
 
 	a = deduceAtom(a, v)
@@ -152,32 +133,7 @@ func handleAtom(a schema.Atom, tr schema.TypeRef, ah atomHandler) ValidationErro
 		name = "named type: " + *tr.NamedType
 	}
 
-	return ah.errorf("schema error: invalid atom: %v", name)
-}
-
-func (ef errorFormatter) validateScalar(t *schema.Scalar, v *value.Value, prefix string) (errs ValidationErrors) {
-	if v == nil {
-		return nil
-	}
-	if *v == nil {
-		return nil
-	}
-	switch *t {
-	case schema.Numeric:
-		if !value.IsFloat(*v) && !value.IsInt(*v) {
-			// TODO: should the schema separate int and float?
-			return ef.errorf("%vexpected numeric (int or float), got %T", prefix, *v)
-		}
-	case schema.String:
-		if !value.IsString(*v) {
-			return ef.errorf("%vexpected string, got %#v", prefix, *v)
-		}
-	case schema.Boolean:
-		if !value.IsBool(*v) {
-			return ef.errorf("%vexpected boolean, got %v", prefix, *v)
-		}
-	}
-	return nil
+	return errorf("schema error: invalid atom: %v", name)
 }
 
 // Returns the list, or an error. Reminder: nil is a valid list and might be returned.

typed/merge.go

@@ -25,12 +25,14 @@ import (
 )
 
 type mergingWalker struct {
-	errorFormatter
 	lhs     *value.Value
 	rhs     *value.Value
 	schema  *schema.Schema
 	typeRef schema.TypeRef
 
+	// Current path that we are merging
+	path fieldpath.Path
+
 	// How to merge. Called after schema validation for all leaf fields.
 	rule mergeRule
 
@@ -69,11 +71,11 @@ var (
 func (w *mergingWalker) merge() (errs ValidationErrors) {
 	if w.lhs == nil && w.rhs == nil {
 		// check this condidition here instead of everywhere below.
-		return w.errorf("at least one of lhs and rhs must be provided")
+		return errorf("at least one of lhs and rhs must be provided")
 	}
 	a, ok := w.schema.Resolve(w.typeRef)
 	if !ok {
-		return w.errorf("schema error: no type found matching: %v", *w.typeRef.NamedType)
+		return errorf("schema error: no type found matching: %v", *w.typeRef.NamedType)
 	}
 
 	alhs := deduceAtom(a, w.lhs)
@@ -107,8 +109,8 @@ func (w *mergingWalker) doLeaf() {
 }
 
 func (w *mergingWalker) doScalar(t *schema.Scalar) (errs ValidationErrors) {
-	errs = append(errs, w.validateScalar(t, w.lhs, "lhs: ")...)
-	errs = append(errs, w.validateScalar(t, w.rhs, "rhs: ")...)
+	errs = append(errs, validateScalar(t, w.lhs, "lhs: ")...)
+	errs = append(errs, validateScalar(t, w.rhs, "rhs: ")...)
 	if len(errs) > 0 {
 		return errs
 	}
@@ -132,7 +134,7 @@ func (w *mergingWalker) prepareDescent(pe fieldpath.PathElement, tr schema.TypeR
 	}
 	*w2 = *w
 	w2.typeRef = tr
-	w2.errorFormatter.descend(pe)
+	w2.path = append(w2.path, pe)
 	w2.lhs = nil
 	w2.rhs = nil
 	w2.out = nil
@@ -142,7 +144,7 @@ func (w *mergingWalker) prepareDescent(pe fieldpath.PathElement, tr schema.TypeR
 func (w *mergingWalker) finishDescent(w2 *mergingWalker) {
 	// if the descent caused a realloc, ensure that we reuse the buffer
 	// for the next sibling.
-	w.errorFormatter = w2.errorFormatter.parent()
+	w.path = w2.path[:len(w2.path)-1]
 	*w.spareWalkers = append(*w.spareWalkers, w2)
 }
 
@@ -154,7 +156,7 @@ func (w *mergingWalker) derefMap(prefix string, v *value.Value, dest *map[string
 	}
 	m, err := mapValue(*v)
 	if err != nil {
-		return w.prefixError(prefix, err)
+		return errorf("%v: %v", prefix, err)
 	}
 	*dest = m
 	return nil
@@ -174,14 +176,14 @@ func (w *mergingWalker) visitListItems(t *schema.List, lhs, rhs []interface{}) (
 	for i, child := range rhs {
 		pe, err := listItemToPathElement(t, i, child)
 		if err != nil {
-			errs = append(errs, w.errorf("rhs: element %v: %v", i, err.Error())...)
+			errs = append(errs, errorf("rhs: element %v: %v", i, err.Error())...)
 			// If we can't construct the path element, we can't
 			// even report errors deeper in the schema, so bail on
 			// this element.
 			continue
 		}
 		if _, ok := observedRHS.Get(pe); ok {
-			errs = append(errs, w.errorf("rhs: duplicate entries for key %v", pe.String())...)
+			errs = append(errs, errorf("rhs: duplicate entries for key %v", pe.String())...)
 		}
 		observedRHS.Insert(pe, child)
 		rhsOrder = append(rhsOrder, pe)
@@ -192,14 +194,14 @@ func (w *mergingWalker) visitListItems(t *schema.List, lhs, rhs []interface{}) (
 	for i, child := range lhs {
 		pe, err := listItemToPathElement(t, i, child)
 		if err != nil {
-			errs = append(errs, w.errorf("lhs: element %v: %v", i, err.Error())...)
+			errs = append(errs, errorf("lhs: element %v: %v", i, err.Error())...)
 			// If we can't construct the path element, we can't
 			// even report errors deeper in the schema, so bail on
 			// this element.
 			continue
 		}
 		if observedLHS.Has(pe) {
-			errs = append(errs, w.errorf("lhs: duplicate entries for key %v", pe.String())...)
+			errs = append(errs, errorf("lhs: duplicate entries for key %v", pe.String())...)
 			continue
 		}
 		observedLHS.Insert(pe)
@@ -210,7 +212,7 @@ func (w *mergingWalker) visitListItems(t *schema.List, lhs, rhs []interface{}) (
 			w2.rhs = &rchild
 		}
 		if newErrs := w2.merge(); len(newErrs) > 0 {
-			errs = append(errs, newErrs...)
+			errs = append(errs, newErrs.WithPrefix(pe.String())...)
 		} else if w2.out != nil {
 			out = append(out, *w2.out)
 		}
@@ -225,7 +227,7 @@ func (w *mergingWalker) visitListItems(t *schema.List, lhs, rhs []interface{}) (
 		w2 := w.prepareDescent(pe, t.ElementType)
 		w2.rhs = &value
 		if newErrs := w2.merge(); len(newErrs) > 0 {
-			errs = append(errs, newErrs...)
+			errs = append(errs, newErrs.WithPrefix(pe.String())...)
 		} else if w2.out != nil {
 			out = append(out, *w2.out)
 		}
@@ -248,7 +250,7 @@ func (w *mergingWalker) derefList(prefix string, v *value.Value, dest *[]interfa
 	}
 	l, err := listValue(*v)
 	if err != nil {
-		return w.prefixError(prefix, err)
+		return errorf("%v: %v", prefix, err)
 	}
 	*dest = l
 	return nil
@@ -283,11 +285,12 @@ func (w *mergingWalker) visitMapItem(t *schema.Map, out map[string]interface{},
 	if sf, ok := t.FindField(key); ok {
 		fieldType = sf.Type
 	}
-	w2 := w.prepareDescent(fieldpath.PathElement{FieldName: &key}, fieldType)
+	pe := fieldpath.PathElement{FieldName: &key}
+	w2 := w.prepareDescent(pe, fieldType)
 	w2.lhs = lhs
 	w2.rhs = rhs
 	if newErrs := w2.merge(); len(newErrs) > 0 {
-		errs = append(errs, newErrs...)
+		errs = append(errs, newErrs.WithPrefix(pe.String())...)
 	} else if w2.out != nil {
 		out[key] = *w2.out
 	}

typed/remove.go

@@ -112,5 +112,3 @@ func (w *removingWalker) doMap(t *schema.Map) ValidationErrors {
 	}
 	return nil
 }
-
-func (*removingWalker) errorf(_ string, _ ...interface{}) ValidationErrors { return nil }

typed/tofieldset.go

@@ -158,7 +158,3 @@ func (v *toFieldSetWalker) doMap(t *schema.Map) (errs ValidationErrors) {
 
 	return errs
 }
-
-func (v *toFieldSetWalker) errorf(string, ...interface{}) ValidationErrors {
-	return nil
-}

typed/typed.go

@@ -163,7 +163,7 @@ func (tv TypedValue) NormalizeUnions(new *TypedValue) (*TypedValue, error) {
 			w.out = &v
 		}
 		if err := normalizeUnions(w); err != nil {
-			errs = append(errs, w.error(err)...)
+			errs = append(errs, errorf(err.Error())...)
 		}
 	}
 	out, mergeErrs := merge(&tv, new, func(w *mergingWalker) {}, normalizeFn)
@@ -189,7 +189,7 @@ func (tv TypedValue) NormalizeUnionsApply(new *TypedValue) (*TypedValue, error)
 			w.out = &v
 		}
 		if err := normalizeUnionsApply(w); err != nil {
-			errs = append(errs, w.error(err)...)
+			errs = append(errs, errorf(err.Error())...)
 		}
 	}
 	out, mergeErrs := merge(&tv, new, func(w *mergingWalker) {}, normalizeFn)
@@ -213,12 +213,10 @@ var mwPool = sync.Pool{
 
 func merge(lhs, rhs *TypedValue, rule, postRule mergeRule) (*TypedValue, error) {
 	if lhs.schema != rhs.schema {
-		return nil, errorFormatter{}.
-			errorf("expected objects with types from the same schema")
+		return nil, errorf("expected objects with types from the same schema")
 	}
 	if !lhs.typeRef.Equals(rhs.typeRef) {
-		return nil, errorFormatter{}.
-			errorf("expected objects of the same type, but got %v and %v", lhs.typeRef, rhs.typeRef)
+		return nil, errorf("expected objects of the same type, but got %v and %v", lhs.typeRef, rhs.typeRef)
 	}
 
 	mw := mwPool.Get().(*mergingWalker)