From 01b51b4f96e6f04345af6153c1b69345f8e075b9 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Fri, 17 Jan 2020 13:40:51 -0800 Subject: [PATCH] proto, internal/errors: add Error sentinel, errors.Wrap Add a sentinel proto.Error error which matches all errors returned by packages in this module. Document that protoregistry.NotFound is an exact sentinel value for performance reasons. Add a Wrap function to the internal/errors package and use it to wrap errors from outside sources (resolvers). Wrapped errors match proto.Error. Fixes golang/protobuf#1021. Change-Id: I45567df3fd6c8dc9a5caafdb55654827f6fb1941 Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/215338 Reviewed-by: Joe Tsai --- internal/encoding/messageset/messageset.go | 8 ++- internal/errors/errors.go | 54 ++++++++++++++-- internal/errors/errors_test.go | 72 ++++++++++++++++------ internal/errors/is_go112.go | 42 +++++++++++++ internal/errors/is_go113.go | 12 ++++ internal/impl/decode.go | 2 +- proto/decode.go | 2 +- proto/messageset.go | 2 +- proto/proto.go | 11 ++++ reflect/protodesc/desc_resolve.go | 2 +- reflect/protoregistry/registry.go | 3 + 11 files changed, 180 insertions(+), 30 deletions(-) create mode 100644 internal/errors/is_go112.go create mode 100644 internal/errors/is_go113.go diff --git a/internal/encoding/messageset/messageset.go b/internal/encoding/messageset/messageset.go index 837e5c49..9d22dd76 100644 --- a/internal/encoding/messageset/messageset.go +++ b/internal/encoding/messageset/messageset.go @@ -61,9 +61,13 @@ func IsMessageSetExtension(fd pref.FieldDescriptor) bool { // In text and JSON formats, the extension name used is the message itself. // The extension field name is derived by appending ExtensionName. func FindMessageSetExtension(r preg.ExtensionTypeResolver, s pref.FullName) (pref.ExtensionType, error) { - xt, err := r.FindExtensionByName(s.Append(ExtensionName)) + name := s.Append(ExtensionName) + xt, err := r.FindExtensionByName(name) if err != nil { - return nil, err + if err == preg.NotFound { + return nil, err + } + return nil, errors.Wrap(err, "%q", name) } if !IsMessageSetExtension(xt.TypeDescriptor()) { return nil, preg.NotFound diff --git a/internal/errors/errors.go b/internal/errors/errors.go index bc495b49..20c17b35 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -6,20 +6,19 @@ package errors import ( + "errors" "fmt" "google.golang.org/protobuf/internal/detrand" ) +// Error is a sentinel matching all errors produced by this package. +var Error = errors.New("protobuf error") + // New formats a string according to the format specifier and arguments and // returns an error that has a "proto" prefix. func New(f string, x ...interface{}) error { - for i := 0; i < len(x); i++ { - if e, ok := x[i].(*prefixError); ok { - x[i] = e.s // avoid "proto: " prefix when chaining - } - } - return &prefixError{s: fmt.Sprintf(f, x...)} + return &prefixError{s: format(f, x...)} } type prefixError struct{ s string } @@ -38,6 +37,49 @@ func (e *prefixError) Error() string { return prefix + e.s } +func (e *prefixError) Unwrap() error { + return Error +} + +// Wrap returns an error that has a "proto" prefix, the formatted string described +// by the format specifier and arguments, and a suffix of err. The error wraps err. +func Wrap(err error, f string, x ...interface{}) error { + return &wrapError{ + s: format(f, x...), + err: err, + } +} + +type wrapError struct { + s string + err error +} + +func (e *wrapError) Error() string { + return format("%v%v: %v", prefix, e.s, e.err) +} + +func (e *wrapError) Unwrap() error { + return e.err +} + +func (e *wrapError) Is(target error) bool { + return target == Error +} + +func format(f string, x ...interface{}) string { + // avoid "proto: " prefix when chaining + for i := 0; i < len(x); i++ { + switch e := x[i].(type) { + case *prefixError: + x[i] = e.s + case *wrapError: + x[i] = format("%v: %v", e.s, e.err) + } + } + return fmt.Sprintf(f, x...) +} + func InvalidUTF8(name string) error { return New("field %v contains invalid UTF-8", name) } diff --git a/internal/errors/errors_test.go b/internal/errors/errors_test.go index b489edd5..804eb96c 100644 --- a/internal/errors/errors_test.go +++ b/internal/errors/errors_test.go @@ -5,27 +5,63 @@ package errors import ( + "errors" "strings" "testing" ) -func TestNewPrefix(t *testing.T) { - e1 := New("abc") - got := e1.Error() - if !strings.HasPrefix(got, "proto:") { - t.Errorf("missing \"proto:\" prefix in %q", got) - } - if !strings.Contains(got, "abc") { - t.Errorf("missing text \"abc\" in %q", got) - } - - e2 := New("%v", e1) - got = e2.Error() - if !strings.HasPrefix(got, "proto:") { - t.Errorf("missing \"proto:\" prefix in %q", got) - } - // Test to make sure prefix is removed from the embedded error. - if strings.Contains(strings.TrimPrefix(got, "proto:"), "proto:") { - t.Errorf("prefix \"proto:\" not elided in embedded error: %q", got) +func TestErrors(t *testing.T) { + var sentinel = New("sentinel") + var foreign = errors.New("foreign") + for _, test := range []struct { + what string + err error + wantText string + is []error + isNot []error + }{{ + what: `New("abc")`, + err: New("abc"), + wantText: "abc", + }, { + what: `New("%v", sentinel)`, + err: New("%v", sentinel), + wantText: "sentinel", + isNot: []error{sentinel}, + }, { + what: `Wrap(sentinel, "%v", "text")`, + err: Wrap(sentinel, "%v", "text"), + wantText: "text: sentinel", + is: []error{sentinel}, + }, { + what: `New("%v", foreign)`, + err: New("%v", foreign), + wantText: "foreign", + isNot: []error{foreign}, + }, { + what: `Wrap(foreign, "%v", "text")`, + err: Wrap(foreign, "%v", "text"), + wantText: "text: foreign", + is: []error{foreign}, + }} { + if got, want := test.err.Error(), prefix; !strings.HasPrefix(got, want) { + t.Errorf("%v.Error() = %q, want prefix %q", test.what, got, want) + } + if got, want := test.err.Error(), prefix+test.wantText; got != want { + t.Errorf("%v.Error() = %q, want %q", test.what, got, want) + } + if got, want := Is(test.err, Error), true; got != want { + t.Errorf("errors.Is(%v, errors.Error) = %v, want %v", test.what, got, want) + } + for _, err := range test.is { + if got, want := Is(test.err, err), true; got != want { + t.Errorf("errors.Is(%v, %v) = %v, want %v", test.what, err, got, want) + } + } + for _, err := range test.isNot { + if got, want := Is(test.err, err), false; got != want { + t.Errorf("errors.Is(%v, %v) = %v, want %v", test.what, err, got, want) + } + } } } diff --git a/internal/errors/is_go112.go b/internal/errors/is_go112.go new file mode 100644 index 00000000..0f5e5850 --- /dev/null +++ b/internal/errors/is_go112.go @@ -0,0 +1,42 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// +build !go1.13 + +package errors + +import "reflect" + +// Is is a copy of Go 1.13's errors.Is for use with older Go versions. +func Is(err, target error) bool { + if target == nil { + return err == target + } + + isComparable := reflect.TypeOf(target).Comparable() + for { + if isComparable && err == target { + return true + } + if x, ok := err.(interface{ Is(error) bool }); ok && x.Is(target) { + return true + } + // TODO: consider supporing target.Is(err). This would allow + // user-definable predicates, but also may allow for coping with sloppy + // APIs, thereby making it easier to get away with them. + if err = unwrap(err); err == nil { + return false + } + } +} + +func unwrap(err error) error { + u, ok := err.(interface { + Unwrap() error + }) + if !ok { + return nil + } + return u.Unwrap() +} diff --git a/internal/errors/is_go113.go b/internal/errors/is_go113.go new file mode 100644 index 00000000..dc05f419 --- /dev/null +++ b/internal/errors/is_go113.go @@ -0,0 +1,12 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// +build go1.13 + +package errors + +import "errors" + +// Is is errors.Is. +func Is(err, target error) bool { return errors.Is(err, target) } diff --git a/internal/impl/decode.go b/internal/impl/decode.go index a2993d4b..6e78faaf 100644 --- a/internal/impl/decode.go +++ b/internal/impl/decode.go @@ -189,7 +189,7 @@ func (mi *MessageInfo) unmarshalExtension(b []byte, num wire.Number, wtyp wire.T if err == preg.NotFound { return out, errUnknown } - return out, err + return out, errors.New("%v: unable to resolve extension %v: %v", mi.Desc.FullName(), num, err) } } xi := getExtensionFieldInfo(xt) diff --git a/proto/decode.go b/proto/decode.go index d8b91877..b712786e 100644 --- a/proto/decode.go +++ b/proto/decode.go @@ -124,7 +124,7 @@ func (o UnmarshalOptions) unmarshalMessageSlow(b []byte, m protoreflect.Message) if fd == nil && md.ExtensionRanges().Has(num) { extType, err := o.Resolver.FindExtensionByNumber(md.FullName(), num) if err != nil && err != protoregistry.NotFound { - return err + return errors.New("%v: unable to resolve extension %v: %v", md.FullName(), num, err) } if extType != nil { fd = extType.TypeDescriptor() diff --git a/proto/messageset.go b/proto/messageset.go index e27e0b7d..f9af2ad6 100644 --- a/proto/messageset.go +++ b/proto/messageset.go @@ -78,7 +78,7 @@ func unmarshalMessageSetField(m protoreflect.Message, num wire.Number, v []byte, return errUnknown } if err != nil { - return err + return errors.New("%v: unable to resolve extension %v: %v", md.FullName(), num, err) } xd := xt.TypeDescriptor() if err := o.unmarshalMessage(v, m.Mutable(xd).Message()); err != nil { diff --git a/proto/proto.go b/proto/proto.go index 983fc715..1fec109f 100644 --- a/proto/proto.go +++ b/proto/proto.go @@ -5,8 +5,19 @@ package proto import ( + "google.golang.org/protobuf/internal/errors" "google.golang.org/protobuf/reflect/protoreflect" ) // Message is the top-level interface that all messages must implement. type Message = protoreflect.ProtoMessage + +// Error matches all errors produced by packages in the protobuf module. +// +// That is, errors.Is(err, Error) reports whether an error is produced +// by this module. +var Error error + +func init() { + Error = errors.Error +} diff --git a/reflect/protodesc/desc_resolve.go b/reflect/protodesc/desc_resolve.go index 1bf4799e..cebb36cd 100644 --- a/reflect/protodesc/desc_resolve.go +++ b/reflect/protodesc/desc_resolve.go @@ -187,7 +187,7 @@ func (r *resolver) findDescriptor(scope protoreflect.FullName, ref partialName) } foundButNotImported = d } else if err != protoregistry.NotFound { - return nil, err + return nil, errors.Wrap(err, "%q", s) } // Continue on at a higher level of scoping. diff --git a/reflect/protoregistry/registry.go b/reflect/protoregistry/registry.go index c77f77a7..8385c751 100644 --- a/reflect/protoregistry/registry.go +++ b/reflect/protoregistry/registry.go @@ -47,6 +47,9 @@ var GlobalFiles *Files = new(Files) var GlobalTypes *Types = new(Types) // NotFound is a sentinel error value to indicate that the type was not found. +// +// Since registry lookup can happen in the critical performance path, resolvers +// must return this exact error value, not an error wrapping it. var NotFound = errors.New("not found") // Files is a registry for looking up or iterating over files and the