reflect/protoreflect: clarify Get semantics on unpopulated fields
Clearly specify that Get on an unpopulated field:
* returns the default value for scalars
* returns a mutable (but empty) List for repeated fields
* returns a mutable (but empty) Map for map fields
* returns an invalid value for message fields
The difference in semantics between List+Maps and Messages is because
protobuf semantics provide no distinction between an unpopulated and empty list
or map. On the other hand, there is a semantic difference between an unpopulated
message and an empty message.
Default values for scalars is trivial to implement with FieldDescriptor.Default.
A mutable, but empty List and Map is easy to implement for known fields since
known fields are generated as a slice or map field in a struct.
Since struct fields are addressable, the implementation can just return a
reference to the slice or map.
Repeated, extension fields are a little more tricky since extension fields
are implemented under the hood as a map[FieldNumber]Extension.
Rather than allocating an empty list in KnownFields.Get upon first retrieval
(which presents a race), delegate the work to ExtensionFieldTypes.Register,
which must occur before any Get operation. Register is not a concurrent-safe
operation, so that is an excellent time to initilize empty lists.
The implementation of extensions will need to be careful that Clear on a repeated
field simply truncates it zero instead of deleting the object.
For unpopulated messages, we return an invalid value, instead of the prior
behavior of returning a typed nil-pointer to the Go type for the message.
The approach is problematic because it assumes that
1) all messages are always implemented on a pointer reciever
2) a typed nil-pointer is an appropriate "read-only, but empty" message
These assumptions are not true of all message types (e.g., dynamic messages).
Change-Id: Ie96e6744c890308d9de738b6cf01d3b19e7e7c6a
Reviewed-on: https://go-review.googlesource.com/c/150319
Reviewed-by: Damien Neil <dneil@google.com>
diff --git a/internal/value/list.go b/internal/value/list.go
index 2eec007..ba82b30 100644
--- a/internal/value/list.go
+++ b/internal/value/list.go
@@ -35,19 +35,13 @@
}
func (ls listReflect) Mutable(i int) pref.Mutable {
// Mutable is only valid for messages and panics for other kinds.
- rv := ls.v.Index(i)
- if rv.IsNil() {
- // TODO: Is checking for nil proper behavior for custom messages?
- pv := pref.ValueOf(ls.conv.MessageType.New().ProtoReflect())
- rv.Set(ls.conv.GoValueOf(pv))
- }
- return rv.Interface().(pref.Message)
+ return ls.conv.PBValueOf(ls.v.Index(i)).Message()
}
func (ls listReflect) MutableAppend() pref.Mutable {
// MutableAppend is only valid for messages and panics for other kinds.
pv := pref.ValueOf(ls.conv.MessageType.New().ProtoReflect())
ls.v.Set(reflect.Append(ls.v, ls.conv.GoValueOf(pv)))
- return ls.v.Index(ls.Len() - 1).Interface().(pref.Message)
+ return pv.Message()
}
func (ls listReflect) Truncate(i int) {
ls.v.Set(ls.v.Slice(0, i))
diff --git a/internal/value/map.go b/internal/value/map.go
index d2ef659..db9656e 100644
--- a/internal/value/map.go
+++ b/internal/value/map.go
@@ -57,13 +57,12 @@
}
rk := ms.keyConv.GoValueOf(k.Value())
rv := ms.v.MapIndex(rk)
- if !rv.IsValid() || rv.IsNil() {
- // TODO: Is checking for nil proper behavior for custom messages?
+ if !rv.IsValid() {
pv := pref.ValueOf(ms.valConv.MessageType.New().ProtoReflect())
rv = ms.valConv.GoValueOf(pv)
ms.v.SetMapIndex(rk, rv)
}
- return rv.Interface().(pref.Message)
+ return ms.valConv.PBValueOf(rv).Message()
}
func (ms mapReflect) Range(f func(pref.MapKey, pref.Value) bool) {
for _, k := range ms.v.MapKeys() {