From 812d9137dfe746dc679874e16e3d9f2287313733 Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Wed, 12 Sep 2018 11:26:15 -0700 Subject: [PATCH] reflect/protoreflect: add Has and Clear to KnownFields and Map Change the API to use explicit Has/Clear methods instead of relying on the "invalid" form of Value to represent nullability. There are several reasons for this change: * Much of the ecosystem around protobufs do not care about nullability alone. For example, for the encoder to determine whether to emit a field: it needs to first check if a field is nulled, and if not, it still needs to go through a series of type-assertion to check whether the value is the zero value. It is much easier to be able to just call Has. * It enables representing the default value as part of the value API, rather than only as part of the descriptor API. * The C++ API also uses explicit has and clear methods. However, we differ from them by defining Has for proto3 scalars (while C++ panics instead). For internal consistency, we also use a Has/Clear API for Maps. Change-Id: I30eda482c959d3e1454d72d9fc761c761ace27a6 Reviewed-on: https://go-review.googlesource.com/134998 Reviewed-by: Damien Neil --- reflect/protoreflect/type.go | 6 +-- reflect/protoreflect/value.go | 77 ++++++++++++++++------------- reflect/protoreflect/value_test.go | 5 +- reflect/protoreflect/value_union.go | 22 +++------ reflect/prototype/protofile_desc.go | 2 +- reflect/prototype/protofile_type.go | 2 +- 6 files changed, 56 insertions(+), 58 deletions(-) diff --git a/reflect/protoreflect/type.go b/reflect/protoreflect/type.go index 223532df..a729c3b7 100644 --- a/reflect/protoreflect/type.go +++ b/reflect/protoreflect/type.go @@ -141,11 +141,11 @@ type DescriptorOptions interface { // Get returns the ith field. It panics if out of bounds. // The FieldDescriptor is guaranteed to be non-nil, while the Value - // may be Null if the proto file did not specify this option. + // may be invalid if the proto file did not specify this option. Get(i int) (FieldDescriptor, Value) // ByName looks a field up by full name and - // returns (nil, Null) if not found. + // returns (nil, Value{}) if not found. // // As a special-case, non-extension fields in the options type can be // directly accessed by the field name alone (e.g., "map_entry" may be used @@ -153,7 +153,7 @@ type DescriptorOptions interface { ByName(s FullName) (FieldDescriptor, Value) // ByNumber looks a field up by the field number and - // returns (nil, Null) if not found. + // returns (nil, Value{}) if not found. ByNumber(n FieldNumber) (FieldDescriptor, Value) doNotImplement diff --git a/reflect/protoreflect/value.go b/reflect/protoreflect/value.go index 77448765..e6db47d2 100644 --- a/reflect/protoreflect/value.go +++ b/reflect/protoreflect/value.go @@ -20,7 +20,7 @@ type Enum interface { Number() EnumNumber } -// Message is a reflection interface to a concrete message value, +// Message is a reflective interface for a concrete message value, // which provides type information and getters/setters for individual fields. // // Concrete types may implement interfaces defined in proto/protoiface, @@ -51,22 +51,12 @@ type Message interface { // underlying repeated element type is determined by the FieldDescriptor.Kind. // See Value for a list of Go types associated with each Kind. // -// Some fields have the property of nullability where it is possible to -// distinguish between the zero value of a field and whether the field was -// explicitly populated with the zero value. Only scalars in proto2, -// members of a oneof field, and singular messages are nullable. -// In the presence of unset fields, KnownFields.Get does not return defaults; -// use the corresponding FieldDescriptor.DefaultValue for that information. -// // Field extensions are handled as known fields once the extension type has been // registered with KnownFields.ExtensionTypes. // -// List, Len, Get, Range, and ExtensionTypes are safe for concurrent access. +// List, Len, Has, Get, Range, and ExtensionTypes are safe for concurrent use. type KnownFields interface { - // List returns a new, unordered list of all fields that are populated. - // A nullable field is populated only if explicitly set. - // A scalar field in proto3 is populated if it contains a non-zero value. - // A repeated field is populated only if it is non-empty. + // List returns a new, unordered list of all populated fields. List() []FieldNumber // Len reports the number of fields that are populated. @@ -74,22 +64,26 @@ type KnownFields interface { // Invariant: f.Len() == len(f.List()) Len() int - // TODO: Should Get return FieldDescriptor.Default if unpopulated instead of - // returning the Null variable? If so, we loose the ability to represent - // nullability in Get and Set calls and also need to add Has and Clear. + // Has reports whether a field is populated. + // + // Some fields have the property of nullability where it is possible to + // distinguish between the default value of a field and whether the field + // was explicitly populated with the default value. Only scalars in proto2, + // member fields of a oneof, and singular messages are nullable. + // + // A nullable field is populated only if explicitly set. + // A scalar field in proto3 is populated if it contains a non-zero value. + // A repeated field is populated only if it is non-empty. + Has(FieldNumber) bool - // Get retrieves the value for field with the given field number. - // It returns Null for non-existent or nulled fields. + // Get retrieves the value for a field with the given field number. + // It returns the default value for unpopulated fields. + // It returns an invalid value for unknown fields. Get(FieldNumber) Value - // TODO: Document memory aliasing behavior when a field is cleared? - // For example, if Mutable is called later, can it reuse memory? - // Set stores the value for a field with the given field number. // Setting a field belonging to a oneof implicitly clears any other field // that may be currently set by the same oneof. - // Null may be used to explicitly clear a field containing a proto2 scalar, - // a member of oneof, or a singular message. // // When setting a composite type, it is unspecified whether the set // value aliases the source's memory in any way. @@ -98,6 +92,15 @@ type KnownFields interface { // in MessageDescriptor.Fields or an extension field in ExtensionTypes. Set(FieldNumber, Value) + // TODO: Document memory aliasing behavior when a field is cleared? + // For example, if Mutable is called later, can it reuse memory? + + // Clear clears the field such that a subsequent call to Has reports false. + // + // It panics if the field number does not correspond with a known field + // in MessageDescriptor.Fields or an extension field in ExtensionTypes. + Clear(FieldNumber) + // Mutable returns a reference value for a field with a given field number. // If the field is unset, Mutable implicitly initializes the field with // a zero value instance of the Go type for that field. @@ -105,7 +108,7 @@ type KnownFields interface { // The returned Mutable reference is never nil, and is only valid until the // next Set or Mutable call. // - // It panics if FieldNumber does not correspond with a known field + // It panics if the field number does not correspond with a known field // in MessageDescriptor.Fields or an extension field in ExtensionTypes. Mutable(FieldNumber) Mutable @@ -124,7 +127,7 @@ type KnownFields interface { // However, the relative ordering of fields with different field numbers // is undefined. // -// List, Len, Get, and Range are safe for concurrent access. +// List, Len, Get, and Range are safe for concurrent use. type UnknownFields interface { // List returns a new, unordered list of all fields that are set. List() []FieldNumber @@ -184,7 +187,7 @@ func (b RawFields) IsValid() bool { // ExtensionFieldTypes are the extension field types that this message instance // has been extended with. // -// List, Len, Get, and Range are safe for concurrent access. +// List, Len, Get, and Range are safe for concurrent use. type ExtensionFieldTypes interface { // List returns a new, unordered list of known extension field types. List() []ExtensionType @@ -219,12 +222,12 @@ type ExtensionFieldTypes interface { Range(f func(ExtensionType) bool) } -// Vector is an ordered list. Every element is always considered populated -// (i.e., Get never provides and Set never accepts Null). +// Vector is an ordered list. Every element is considered populated +// (i.e., Get never provides and Set never accepts invalid Values). // The element Value type is determined by the associated FieldDescriptor.Kind // and cannot be a Map or Vector. // -// Len and Get are safe for concurrent access. +// Len and Get are safe for concurrent use. type Vector interface { // Len reports the number of entries in the Vector. // Get, Set, Mutable, and Truncate panic with out of bound indexes. @@ -237,16 +240,12 @@ type Vector interface { // // When setting a composite type, it is unspecified whether the set // value aliases the source's memory in any way. - // - // It panics if the value is Null. Set(int, Value) // Append appends the provided value to the end of the vector. // // When appending a composite type, it is unspecified whether the appended // value aliases the source's memory in any way. - // - // It panics if the value is Null. Append(Value) // Mutable returns a Mutable reference for the element with a given index. @@ -274,7 +273,7 @@ type Vector interface { // is considered populated. The entry Value type is determined by the associated // FieldDescripto.Kind and cannot be a Map or Vector. // -// List, Len, Get, and Range are safe for concurrent access. +// List, Len, Has, Get, and Range are safe for concurrent use. type Map interface { // List returns an unordered list of keys for all entries in the map. List() []MapKey @@ -284,7 +283,11 @@ type Map interface { // Invariant: f.Len() == len(f.List()) Len() int + // Has reports whether an entry with the given key is in the map. + Has(MapKey) bool + // Get retrieves the value for an entry with the given key. + // It returns an invalid value for non-existent entries. Get(MapKey) Value // Set stores the value for an entry with the given key. @@ -292,9 +295,12 @@ type Map interface { // When setting a composite type, it is unspecified whether the set // value aliases the source's memory in any way. // - // It panics if either the key or value are Null. + // It panics if either the key or value are invalid. Set(MapKey, Value) + // Clear clears the entry associated with they given key. + Clear(MapKey) + // Mutable returns a Mutable reference for the element with a given key, // allocating a new entry if necessary. // @@ -303,6 +309,7 @@ type Map interface { Mutable(MapKey) Mutable // Range calls f sequentially for each key and value present in the map. + // The Value provided is guaranteed to be valid. // If f returns false, range stops the iteration. Range(f func(MapKey, Value) bool) diff --git a/reflect/protoreflect/value_test.go b/reflect/protoreflect/value_test.go index 69d99665..2b56f4a0 100644 --- a/reflect/protoreflect/value_test.go +++ b/reflect/protoreflect/value_test.go @@ -20,7 +20,6 @@ func TestValue(t *testing.T) { in Value want interface{} }{ - {in: Null}, {in: Value{}}, {in: ValueOf(nil)}, {in: ValueOf(true), want: true}, @@ -43,8 +42,8 @@ func TestValue(t *testing.T) { t.Errorf("Value(%v).Interface() = %v, want %v", tt.in, got, tt.want) } - if got := tt.in.IsNull(); got != (tt.want == nil) { - t.Errorf("Value(%v).IsNull() = %v, want %v", tt.in, got, tt.want == nil) + if got := tt.in.IsValid(); got != (tt.want != nil) { + t.Errorf("Value(%v).IsValid() = %v, want %v", tt.in, got, tt.want != nil) } switch want := tt.want.(type) { case int32: diff --git a/reflect/protoreflect/value_union.go b/reflect/protoreflect/value_union.go index 351b606e..aa473480 100644 --- a/reflect/protoreflect/value_union.go +++ b/reflect/protoreflect/value_union.go @@ -35,7 +35,7 @@ import ( // // Multiple protobuf Kinds may be represented by a single Go type if the type // can losslessly represent the information for the proto kind. For example, -// Int64Kind, Sint64Kind, and Sfixed64Kind all represent int64, +// Int64Kind, Sint64Kind, and Sfixed64Kind are all represented by int64, // but use different integer encoding methods. // // The Vector or Map types are used if the FieldDescriptor.Cardinality of the @@ -47,14 +47,6 @@ import ( // retrieve an int64 from a string. type Value value -// Null is an unpopulated Value. -// -// Since Value is incomparable, call Value.IsNull instead to test whether -// a Value is empty. -// -// It is equivalent to Value{} or ValueOf(nil). -var Null Value - // The protoreflect API uses a custom Value union type instead of interface{} // to keep the future open for performance optimizations. Using an interface{} // always incurs an allocation for primitives (e.g., int64) since it needs to @@ -110,9 +102,9 @@ func ValueOf(v interface{}) Value { } } -// IsNull reports whether v is empty (has no value). -func (v Value) IsNull() bool { - return v.typ == nilType +// IsValid reports whether v is populated with a value. +func (v Value) IsValid() bool { + return v.typ != nilType } // Interface returns v as an interface{}. @@ -282,9 +274,9 @@ func (v Value) MapKey() MapKey { // converting a Value to a MapKey with an invalid type panics. type MapKey value -// IsNull reports whether v is empty (has no value). -func (k MapKey) IsNull() bool { - return Value(k).IsNull() +// IsValid reports whether k is populated with a value. +func (k MapKey) IsValid() bool { + return Value(k).IsValid() } // Interface returns k as an interface{}. diff --git a/reflect/prototype/protofile_desc.go b/reflect/prototype/protofile_desc.go index 466e53e7..8ab32794 100644 --- a/reflect/prototype/protofile_desc.go +++ b/reflect/prototype/protofile_desc.go @@ -386,5 +386,5 @@ func parseDefault(s string, k protoreflect.Kind) (protoreflect.Value, error) { return protoreflect.ValueOf([]byte(s)), nil } } - return protoreflect.Null, errors.New("invalid default value for %v: %q", k, s) + return protoreflect.Value{}, errors.New("invalid default value for %v: %q", k, s) } diff --git a/reflect/prototype/protofile_type.go b/reflect/prototype/protofile_type.go index f0584035..ef228238 100644 --- a/reflect/prototype/protofile_type.go +++ b/reflect/prototype/protofile_type.go @@ -439,7 +439,7 @@ var ( func (p *defaultValue) lazyInit(t pref.FieldDescriptor, v pref.Value) pref.Value { p.once.Do(func() { p.val = v - if !v.IsNull() { + if v.IsValid() { switch t.Kind() { case pref.EnumKind: // Treat a string value as an identifier referencing some enum