From 1f5b6fe64530cac2061a3d315b7e44966b1a200b Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Wed, 22 Apr 2020 21:58:56 -0700 Subject: [PATCH] all: improve panic messages for better debugability Change-Id: If3e505e715d5ce2c9a81249c868d26226a25b724 Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/232339 Reviewed-by: Damien Neil --- internal/cmd/generate-types/impl.go | 2 +- internal/filedesc/desc.go | 2 +- internal/impl/message_reflect.go | 15 +++-- internal/impl/message_reflect_field.go | 29 +++++----- internal/impl/message_reflect_gen.go | 4 +- internal/impl/pointer_unsafe.go | 6 +- proto/merge.go | 7 ++- proto/reset.go | 8 ++- reflect/protoreflect/value_union.go | 78 ++++++++++++++++++++------ 9 files changed, 106 insertions(+), 45 deletions(-) diff --git a/internal/cmd/generate-types/impl.go b/internal/cmd/generate-types/impl.go index 4b362c60..6068671b 100644 --- a/internal/cmd/generate-types/impl.go +++ b/internal/cmd/generate-types/impl.go @@ -820,7 +820,7 @@ func (m *{{.}}) WhichOneof(od protoreflect.OneofDescriptor) protoreflect.FieldDe if oi := m.messageInfo().oneofs[od.Name()]; oi != nil && oi.oneofDesc == od { return od.Fields().ByNumber(oi.which(m.pointer())) } - panic("invalid oneof descriptor") + panic("invalid oneof descriptor " + string(od.FullName()) + " for message " + string(m.Descriptor().FullName())) } func (m *{{.}}) GetUnknown() protoreflect.RawFields { m.messageInfo().init() diff --git a/internal/filedesc/desc.go b/internal/filedesc/desc.go index 13b7723c..2540befd 100644 --- a/internal/filedesc/desc.go +++ b/internal/filedesc/desc.go @@ -607,7 +607,7 @@ func (dv *defaultValue) get(fd pref.FieldDescriptor) pref.Value { // TODO: Avoid panic if we're running with the race detector // and instead spawn a goroutine that periodically resets // this value back to the original to induce a race. - panic("detected mutation on the default bytes") + panic(fmt.Sprintf("detected mutation on the default bytes for %v", fd.FullName())) } return dv.val } diff --git a/internal/impl/message_reflect.go b/internal/impl/message_reflect.go index 3eb2b139..0f4b8db7 100644 --- a/internal/impl/message_reflect.go +++ b/internal/impl/message_reflect.go @@ -338,24 +338,27 @@ func (mi *MessageInfo) checkField(fd pref.FieldDescriptor) (*fieldInfo, pref.Ext } if fi != nil { if fi.fieldDesc != fd { - panic("mismatching field descriptor") + if got, want := fd.FullName(), fi.fieldDesc.FullName(); got != want { + panic(fmt.Sprintf("mismatching field: got %v, want %v", got, want)) + } + panic(fmt.Sprintf("mismatching field: %v", fd.FullName())) } return fi, nil } if fd.IsExtension() { - if fd.ContainingMessage().FullName() != mi.Desc.FullName() { + if got, want := fd.ContainingMessage().FullName(), mi.Desc.FullName(); got != want { // TODO: Should this be exact containing message descriptor match? - panic("mismatching containing message") + panic(fmt.Sprintf("extension %v has mismatching containing message: got %v, want %v", fd.FullName(), got, want)) } if !mi.Desc.ExtensionRanges().Has(fd.Number()) { - panic("invalid extension field") + panic(fmt.Sprintf("extension %v extends %v outside the extension range", fd.FullName(), mi.Desc.FullName())) } xtd, ok := fd.(pref.ExtensionTypeDescriptor) if !ok { - panic("extension descriptor does not implement ExtensionTypeDescriptor") + panic(fmt.Sprintf("extension %v does not implement protoreflect.ExtensionTypeDescriptor", fd.FullName())) } return nil, xtd.Type() } - panic("invalid field descriptor") + panic(fmt.Sprintf("field %v is invalid", fd.FullName())) } diff --git a/internal/impl/message_reflect_field.go b/internal/impl/message_reflect_field.go index ea6f7554..23124a86 100644 --- a/internal/impl/message_reflect_field.go +++ b/internal/impl/message_reflect_field.go @@ -31,13 +31,13 @@ type fieldInfo struct { func fieldInfoForOneof(fd pref.FieldDescriptor, fs reflect.StructField, x exporter, ot reflect.Type) fieldInfo { ft := fs.Type if ft.Kind() != reflect.Interface { - panic(fmt.Sprintf("invalid type: got %v, want interface kind", ft)) + panic(fmt.Sprintf("field %v has invalid type: got %v, want interface kind", fd.FullName(), ft)) } if ot.Kind() != reflect.Struct { - panic(fmt.Sprintf("invalid type: got %v, want struct kind", ot)) + panic(fmt.Sprintf("field %v has invalid type: got %v, want struct kind", fd.FullName(), ot)) } if !reflect.PtrTo(ot).Implements(ft) { - panic(fmt.Sprintf("invalid type: %v does not implement %v", ot, ft)) + panic(fmt.Sprintf("field %v has invalid type: %v does not implement %v", fd.FullName(), ot, ft)) } conv := NewConverter(ot.Field(0).Type, fd) isMessage := fd.Message() != nil @@ -90,7 +90,7 @@ func fieldInfoForOneof(fd pref.FieldDescriptor, fs reflect.StructField, x export }, mutable: func(p pointer) pref.Value { if !isMessage { - panic("invalid Mutable on field with non-composite type") + panic(fmt.Sprintf("field %v with invalid Mutable call on field with non-composite type", fd.FullName())) } rv := p.Apply(fieldOffset).AsValueOf(fs.Type).Elem() if rv.IsNil() || rv.Elem().Type().Elem() != ot || rv.Elem().IsNil() { @@ -114,7 +114,7 @@ func fieldInfoForOneof(fd pref.FieldDescriptor, fs reflect.StructField, x export func fieldInfoForMap(fd pref.FieldDescriptor, fs reflect.StructField, x exporter) fieldInfo { ft := fs.Type if ft.Kind() != reflect.Map { - panic(fmt.Sprintf("invalid type: got %v, want map kind", ft)) + panic(fmt.Sprintf("field %v has invalid type: got %v, want map kind", fd.FullName(), ft)) } conv := NewConverter(ft, fd) @@ -147,7 +147,7 @@ func fieldInfoForMap(fd pref.FieldDescriptor, fs reflect.StructField, x exporter rv := p.Apply(fieldOffset).AsValueOf(fs.Type).Elem() pv := conv.GoValueOf(v) if pv.IsNil() { - panic(fmt.Sprintf("invalid value: setting map field to read-only value")) + panic(fmt.Sprintf("map field %v cannot be set with read-only value", fd.FullName())) } rv.Set(pv) }, @@ -167,7 +167,7 @@ func fieldInfoForMap(fd pref.FieldDescriptor, fs reflect.StructField, x exporter func fieldInfoForList(fd pref.FieldDescriptor, fs reflect.StructField, x exporter) fieldInfo { ft := fs.Type if ft.Kind() != reflect.Slice { - panic(fmt.Sprintf("invalid type: got %v, want slice kind", ft)) + panic(fmt.Sprintf("field %v has invalid type: got %v, want slice kind", fd.FullName(), ft)) } conv := NewConverter(reflect.PtrTo(ft), fd) @@ -200,7 +200,7 @@ func fieldInfoForList(fd pref.FieldDescriptor, fs reflect.StructField, x exporte rv := p.Apply(fieldOffset).AsValueOf(fs.Type).Elem() pv := conv.GoValueOf(v) if pv.IsNil() { - panic(fmt.Sprintf("invalid value: setting repeated field to read-only value")) + panic(fmt.Sprintf("list field %v cannot be set with read-only value", fd.FullName())) } rv.Set(pv.Elem()) }, @@ -225,7 +225,7 @@ func fieldInfoForScalar(fd pref.FieldDescriptor, fs reflect.StructField, x expor isBytes := ft.Kind() == reflect.Slice && ft.Elem().Kind() == reflect.Uint8 if nullable { if ft.Kind() != reflect.Ptr && ft.Kind() != reflect.Slice { - panic(fmt.Sprintf("invalid type: got %v, want pointer", ft)) + panic(fmt.Sprintf("field %v has invalid type: got %v, want pointer", fd.FullName(), ft)) } if ft.Kind() == reflect.Ptr { ft = ft.Elem() @@ -257,7 +257,7 @@ func fieldInfoForScalar(fd pref.FieldDescriptor, fs reflect.StructField, x expor case reflect.String, reflect.Slice: return rv.Len() > 0 default: - panic(fmt.Sprintf("invalid type: %v", rv.Type())) // should never happen + panic(fmt.Sprintf("field %v has invalid type: %v", fd.FullName(), rv.Type())) // should never happen } }, clear: func(p pointer) { @@ -314,7 +314,7 @@ func fieldInfoForWeakMessage(fd pref.FieldDescriptor, weakOffset offset) fieldIn messageName := fd.Message().FullName() messageType, _ = preg.GlobalTypes.FindMessageByName(messageName) if messageType == nil { - panic(fmt.Sprintf("weak message %v is not linked in", messageName)) + panic(fmt.Sprintf("weak message %v for field %v is not linked in", messageName, fd.FullName())) } }) } @@ -347,7 +347,10 @@ func fieldInfoForWeakMessage(fd pref.FieldDescriptor, weakOffset offset) fieldIn lazyInit() m := v.Message() if m.Descriptor() != messageType.Descriptor() { - panic("mismatching message descriptor") + if got, want := m.Descriptor().FullName(), messageType.Descriptor().FullName(); got != want { + panic(fmt.Sprintf("field %v has mismatching message descriptor: got %v, want %v", fd.FullName(), got, want)) + } + panic(fmt.Sprintf("field %v has mismatching message descriptor: %v", fd.FullName(), m.Descriptor().FullName())) } p.Apply(weakOffset).WeakFields().set(num, m.Interface()) }, @@ -402,7 +405,7 @@ func fieldInfoForMessage(fd pref.FieldDescriptor, fs reflect.StructField, x expo rv := p.Apply(fieldOffset).AsValueOf(fs.Type).Elem() rv.Set(conv.GoValueOf(v)) if rv.IsNil() { - panic("invalid nil pointer") + panic(fmt.Sprintf("field %v has invalid nil pointer", fd.FullName())) } }, mutable: func(p pointer) pref.Value { diff --git a/internal/impl/message_reflect_gen.go b/internal/impl/message_reflect_gen.go index 7d65f5ca..741d6e5b 100644 --- a/internal/impl/message_reflect_gen.go +++ b/internal/impl/message_reflect_gen.go @@ -114,7 +114,7 @@ func (m *messageState) WhichOneof(od protoreflect.OneofDescriptor) protoreflect. if oi := m.messageInfo().oneofs[od.Name()]; oi != nil && oi.oneofDesc == od { return od.Fields().ByNumber(oi.which(m.pointer())) } - panic("invalid oneof descriptor") + panic("invalid oneof descriptor " + string(od.FullName()) + " for message " + string(m.Descriptor().FullName())) } func (m *messageState) GetUnknown() protoreflect.RawFields { m.messageInfo().init() @@ -234,7 +234,7 @@ func (m *messageReflectWrapper) WhichOneof(od protoreflect.OneofDescriptor) prot if oi := m.messageInfo().oneofs[od.Name()]; oi != nil && oi.oneofDesc == od { return od.Fields().ByNumber(oi.which(m.pointer())) } - panic("invalid oneof descriptor") + panic("invalid oneof descriptor " + string(od.FullName()) + " for message " + string(m.Descriptor().FullName())) } func (m *messageReflectWrapper) GetUnknown() protoreflect.RawFields { m.messageInfo().init() diff --git a/internal/impl/pointer_unsafe.go b/internal/impl/pointer_unsafe.go index 27416433..088aa85d 100644 --- a/internal/impl/pointer_unsafe.go +++ b/internal/impl/pointer_unsafe.go @@ -148,7 +148,11 @@ func (ms *messageState) pointer() pointer { return pointer{p: unsafe.Pointer(ms)} } func (ms *messageState) messageInfo() *MessageInfo { - return ms.LoadMessageInfo() + mi := ms.LoadMessageInfo() + if mi == nil { + panic("invalid nil message info; this suggests memory corruption due to a race or shallow copy on the message struct") + } + return mi } func (ms *messageState) LoadMessageInfo() *MessageInfo { return (*MessageInfo)(atomic.LoadPointer((*unsafe.Pointer)(unsafe.Pointer(&ms.atomicMessageInfo)))) diff --git a/proto/merge.go b/proto/merge.go index 27ab1a68..d761ab33 100644 --- a/proto/merge.go +++ b/proto/merge.go @@ -5,6 +5,8 @@ package proto import ( + "fmt" + "google.golang.org/protobuf/reflect/protoreflect" "google.golang.org/protobuf/runtime/protoiface" ) @@ -26,6 +28,9 @@ func Merge(dst, src Message) { dstMsg, srcMsg := dst.ProtoReflect(), src.ProtoReflect() if dstMsg.Descriptor() != srcMsg.Descriptor() { + if got, want := dstMsg.Descriptor().FullName(), srcMsg.Descriptor().FullName(); got != want { + panic(fmt.Sprintf("descriptor mismatch: %v != %v", got, want)) + } panic("descriptor mismatch") } mergeOptions{}.mergeMessage(dstMsg, srcMsg) @@ -72,7 +77,7 @@ func (o mergeOptions) mergeMessage(dst, src protoreflect.Message) { } if !dst.IsValid() { - panic("cannot merge into invalid destination message") + panic(fmt.Sprintf("cannot merge into invalid %v message", dst.Descriptor().FullName())) } src.Range(func(fd protoreflect.FieldDescriptor, v protoreflect.Value) bool { diff --git a/proto/reset.go b/proto/reset.go index 22048bc1..3d7f8943 100644 --- a/proto/reset.go +++ b/proto/reset.go @@ -4,7 +4,11 @@ package proto -import "google.golang.org/protobuf/reflect/protoreflect" +import ( + "fmt" + + "google.golang.org/protobuf/reflect/protoreflect" +) // Reset clears every field in the message. // The resulting message shares no observable memory with its previous state @@ -19,7 +23,7 @@ func Reset(m Message) { func resetMessage(m protoreflect.Message) { if !m.IsValid() { - panic("cannot reset invalid message") + panic(fmt.Sprintf("cannot reset invalid %v message", m.Descriptor().FullName())) } // Clear all known fields. diff --git a/reflect/protoreflect/value_union.go b/reflect/protoreflect/value_union.go index d695a4d6..f334f71b 100644 --- a/reflect/protoreflect/value_union.go +++ b/reflect/protoreflect/value_union.go @@ -7,7 +7,6 @@ package protoreflect import ( "fmt" "math" - "reflect" ) // Value is a union where only one Go type may be set at a time. @@ -87,7 +86,7 @@ func ValueOf(v interface{}) Value { case Message, List, Map: return valueOfIface(v) default: - panic(fmt.Sprintf("invalid type: %v", reflect.TypeOf(v))) + panic(fmt.Sprintf("invalid type: %T", v)) } } @@ -197,13 +196,55 @@ func (v Value) Interface() interface{} { } } +func (v Value) typeName() string { + switch v.typ { + case nilType: + return "nil" + case boolType: + return "bool" + case int32Type: + return "int32" + case int64Type: + return "int64" + case uint32Type: + return "uint32" + case uint64Type: + return "uint64" + case float32Type: + return "float32" + case float64Type: + return "float64" + case stringType: + return "string" + case bytesType: + return "bytes" + case enumType: + return "enum" + default: + switch v := v.getIface().(type) { + case Message: + return "message" + case List: + return "list" + case Map: + return "map" + default: + return fmt.Sprintf("", v) + } + } +} + +func (v Value) panicMessage(what string) string { + return fmt.Sprintf("type mismatch: cannot convert %v to %s", v.typeName(), what) +} + // Bool returns v as a bool and panics if the type is not a bool. func (v Value) Bool() bool { switch v.typ { case boolType: return v.num > 0 default: - panic("proto: value type mismatch") + panic(v.panicMessage("bool")) } } @@ -213,7 +254,7 @@ func (v Value) Int() int64 { case int32Type, int64Type: return int64(v.num) default: - panic("proto: value type mismatch") + panic(v.panicMessage("int")) } } @@ -223,7 +264,7 @@ func (v Value) Uint() uint64 { case uint32Type, uint64Type: return uint64(v.num) default: - panic("proto: value type mismatch") + panic(v.panicMessage("uint")) } } @@ -233,7 +274,7 @@ func (v Value) Float() float64 { case float32Type, float64Type: return math.Float64frombits(uint64(v.num)) default: - panic("proto: value type mismatch") + panic(v.panicMessage("float")) } } @@ -254,7 +295,7 @@ func (v Value) Bytes() []byte { case bytesType: return v.getBytes() default: - panic("proto: value type mismatch") + panic(v.panicMessage("bytes")) } } @@ -264,37 +305,37 @@ func (v Value) Enum() EnumNumber { case enumType: return EnumNumber(v.num) default: - panic("proto: value type mismatch") + panic(v.panicMessage("enum")) } } // Message returns v as a Message and panics if the type is not a Message. func (v Value) Message() Message { - switch v := v.getIface().(type) { + switch vi := v.getIface().(type) { case Message: - return v + return vi default: - panic("proto: value type mismatch") + panic(v.panicMessage("message")) } } // List returns v as a List and panics if the type is not a List. func (v Value) List() List { - switch v := v.getIface().(type) { + switch vi := v.getIface().(type) { case List: - return v + return vi default: - panic("proto: value type mismatch") + panic(v.panicMessage("list")) } } // Map returns v as a Map and panics if the type is not a Map. func (v Value) Map() Map { - switch v := v.getIface().(type) { + switch vi := v.getIface().(type) { case Map: - return v + return vi default: - panic("proto: value type mismatch") + panic(v.panicMessage("map")) } } @@ -303,8 +344,9 @@ func (v Value) MapKey() MapKey { switch v.typ { case boolType, int32Type, int64Type, uint32Type, uint64Type, stringType: return MapKey(v) + default: + panic(v.panicMessage("map key")) } - panic("proto: invalid map key type") } // MapKey is used to index maps, where the Go type of the MapKey must match