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/impl/legacy_extension.go b/internal/impl/legacy_extension.go
index d54da34..a7a28cf 100644
--- a/internal/impl/legacy_extension.go
+++ b/internal/impl/legacy_extension.go
@@ -59,16 +59,9 @@
}
t := legacyExtensionTypeOf(x.desc)
if x.val == nil {
- if t.Cardinality() == pref.Repeated {
- // TODO: What is the zero value for Lists?
- // TODO: This logic is racy.
- v := t.ValueOf(t.New())
- x.val = t.InterfaceOf(v)
- p.x.Set(n, x)
- return v
- }
+ // NOTE: x.val is never nil for Lists since they are always populated
+ // during ExtensionFieldTypes.Register.
if t.Kind() == pref.MessageKind || t.Kind() == pref.GroupKind {
- // TODO: What is the zero value for Messages?
return pref.Value{}
}
return t.Default()
@@ -91,6 +84,11 @@
if x.desc == nil {
return
}
+ t := legacyExtensionTypeOf(x.desc)
+ if t.Cardinality() == pref.Repeated {
+ t.ValueOf(x.val).List().Truncate(0)
+ return
+ }
x.val = nil
p.x.Set(n, x)
}
@@ -146,6 +144,11 @@
panic("extension descriptor already registered")
}
x.desc = legacyExtensionDescOf(t, p.mi.goType)
+ if t.Cardinality() == pref.Repeated {
+ // If the field is repeated, initialize the entry with an empty list
+ // so that future Get operations can return a mutable and concrete list.
+ x.val = t.InterfaceOf(t.ValueOf(t.New()))
+ }
p.x.Set(t.Number(), x)
}
@@ -154,6 +157,13 @@
return
}
x := p.x.Get(t.Number())
+ if t.Cardinality() == pref.Repeated {
+ // Treat an empty repeated field as unpopulated.
+ v := reflect.ValueOf(x.val)
+ if x.val == nil || v.IsNil() || v.Elem().Len() == 0 {
+ x.val = nil
+ }
+ }
if x.val != nil {
panic("value for extension descriptor still populated")
}