internal: simplify ExtensionInfo initialization
This CL:
* Make the meaning of impl/ExtensionInfo.goType consistent. Before,
it was sometimes a T and other times a []T depending on the current
state of initialization. Change it so that it is the constructor's
responsibility to pass in a []T if it is repeated.
* Make internal/filetype responsible for constructing a []T for
repeated extension fields.
* Makes filedesc/Extension.Cardinality one of the eagerly initialized
pieces of information since it is useful to internal/filetype.
* Unify ExtensionInfo.desc and ExtensionInfo.tdesc.ExtensionField,
which held the same information.
* Remove the internal implementation for impl.X.ExtensionDescFromType
since we are dropping support for this from v1.
Change-Id: Ie95c4de66cd674c1d886da4f63b133b7d763c7ef
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/195777
Reviewed-by: Damien Neil <dneil@google.com>
diff --git a/internal/impl/api_export.go b/internal/impl/api_export.go
index 3cb2ada..95e7846 100644
--- a/internal/impl/api_export.go
+++ b/internal/impl/api_export.go
@@ -140,5 +140,9 @@
// ExtensionDescFromType returns the legacy protoV1.ExtensionDesc for t.
func (Export) ExtensionDescFromType(t pref.ExtensionType) *ExtensionInfo {
- return legacyExtensionDescFromType(t)
+ // TODO: Delete this function when v1 directly does this assertion.
+ if xt, ok := t.(*ExtensionInfo); ok {
+ return xt
+ }
+ return nil
}
diff --git a/internal/impl/extension.go b/internal/impl/extension.go
index 40074fc..a895763 100644
--- a/internal/impl/extension.go
+++ b/internal/impl/extension.go
@@ -26,21 +26,17 @@
// initialized. This is the starting state for an ExtensionInfo
// in legacy generated code.
//
- // extensionInfoDescInit: The desc and tdesc fields have been
- // set, but the descriptor is not otherwise initialized. Legacy
- // exported fields may or may not be set. This is the starting state
- // for an ExtensionInfo in new generated code. Calling the Descriptor
- // method will not trigger lazy initialization, although any other
- // method will.
+ // extensionInfoDescInit: The desc field is set, but other unexported fields
+ // may not be initialized. Legacy exported fields may or may not be set.
+ // This is the starting state for an ExtensionInfo in newly generated code.
//
// extensionInfoFullInit: The ExtensionInfo is fully initialized.
// This state is only entered after lazy initialization is complete.
init uint32
mu sync.Mutex
- desc pref.ExtensionDescriptor
- tdesc extensionTypeDescriptor
goType reflect.Type
+ desc extensionTypeDescriptor
conv Converter
// ExtendedType is a typed nil-pointer to the parent message type that
@@ -91,14 +87,8 @@
)
func InitExtensionInfo(xi *ExtensionInfo, xd pref.ExtensionDescriptor, goType reflect.Type) {
- if xi.desc != nil {
- return
- }
- xi.desc = xd
xi.goType = goType
-
- xi.tdesc.ExtensionDescriptor = xi.desc
- xi.tdesc.xi = xi
+ xi.desc = extensionTypeDescriptor{xd, xi}
xi.init = extensionInfoDescInit
}
@@ -125,14 +115,14 @@
return xi.goType
}
func (xi *ExtensionInfo) TypeDescriptor() pref.ExtensionTypeDescriptor {
- if atomic.LoadUint32(&xi.init) == extensionInfoUninitialized {
+ if atomic.LoadUint32(&xi.init) < extensionInfoDescInit {
xi.lazyInitSlow()
}
- return &xi.tdesc
+ return &xi.desc
}
func (xi *ExtensionInfo) lazyInit() Converter {
- if atomic.LoadUint32(&xi.init) != extensionInfoFullInit {
+ if atomic.LoadUint32(&xi.init) < extensionInfoFullInit {
xi.lazyInitSlow()
}
return xi.conv
@@ -147,19 +137,13 @@
}
defer atomic.StoreUint32(&xi.init, extensionInfoFullInit)
- if xi.desc == nil {
+ if xi.desc.ExtensionDescriptor == nil {
xi.initFromLegacy()
- } else if xi.desc.Cardinality() == pref.Repeated {
- // Cardinality is initialized lazily, so we defer consulting it until here.
- xi.goType = reflect.SliceOf(xi.goType)
}
- xi.conv = NewConverter(xi.goType, xi.desc)
- xi.tdesc.ExtensionDescriptor = xi.desc
- xi.tdesc.xi = xi
-
if xi.ExtensionType == nil {
xi.initToLegacy()
}
+ xi.conv = NewConverter(xi.goType, xi.desc)
}
type extensionTypeDescriptor struct {
diff --git a/internal/impl/legacy_extension.go b/internal/impl/legacy_extension.go
index bfa6e1b..0979ec0 100644
--- a/internal/impl/legacy_extension.go
+++ b/internal/impl/legacy_extension.go
@@ -6,7 +6,6 @@
import (
"reflect"
- "sync"
"google.golang.org/protobuf/internal/encoding/messageset"
ptag "google.golang.org/protobuf/internal/encoding/tag"
@@ -16,36 +15,6 @@
piface "google.golang.org/protobuf/runtime/protoiface"
)
-var legacyExtensionInfoCache sync.Map // map[protoreflect.ExtensionType]*ExtensionInfo
-
-// legacyExtensionDescFromType converts a protoreflect.ExtensionType to an
-// ExtensionInfo. The returned ExtensionInfo must not be mutated.
-func legacyExtensionDescFromType(xt pref.ExtensionType) *ExtensionInfo {
- // Fast-path: check whether this is an ExtensionInfo.
- if xt, ok := xt.(*ExtensionInfo); ok {
- return xt
- }
-
- // Fast-path: check the cache for whether this ExtensionType has already
- // been converted to an ExtensionInfo.
- if d, ok := legacyExtensionInfoCache.Load(xt); ok {
- return d.(*ExtensionInfo)
- }
-
- tt := xt.GoType()
- if xt.TypeDescriptor().Cardinality() == pref.Repeated {
- tt = tt.Elem().Elem()
- }
- xi := &ExtensionInfo{}
- InitExtensionInfo(xi, xt.TypeDescriptor().Descriptor(), tt)
- xi.lazyInit() // populate legacy fields
-
- if xi, ok := legacyExtensionInfoCache.LoadOrStore(xt, xi); ok {
- return xi.(*ExtensionInfo)
- }
- return xi
-}
-
func (xi *ExtensionInfo) initToLegacy() {
xd := xi.desc
var parent piface.MessageV1
@@ -134,7 +103,7 @@
xd.L0.ParentFile = filedesc.SurrogateProto2
xd.L0.FullName = pref.FullName(xi.Name)
xd.L1.Number = pref.FieldNumber(xi.Field)
- xd.L2.Cardinality = fd.L1.Cardinality
+ xd.L1.Cardinality = fd.L1.Cardinality
xd.L1.Kind = fd.L1.Kind
xd.L2.IsPacked = fd.L1.IsPacked
xd.L2.Default = fd.L1.Default
@@ -151,6 +120,6 @@
if isOptional {
tt = tt.Elem()
}
- xi.desc = xd
xi.goType = tt
+ xi.desc = extensionTypeDescriptor{xd, xi}
}
diff --git a/internal/impl/legacy_test.go b/internal/impl/legacy_test.go
index 6a11f40..46d8c36 100644
--- a/internal/impl/legacy_test.go
+++ b/internal/impl/legacy_test.go
@@ -143,52 +143,52 @@
mustMakeExtensionType(
`package:"fizz.buzz" dependency:"legacy.proto"`,
`name:"repeated_bool" number:10010 label:LABEL_REPEATED type:TYPE_BOOL extendee:".LegacyTestMessage"`,
- reflect.TypeOf(false), depReg,
+ reflect.TypeOf([]bool(nil)), depReg,
),
mustMakeExtensionType(
`package:"fizz.buzz" dependency:"legacy.proto"`,
`name:"repeated_int32" number:10011 label:LABEL_REPEATED type:TYPE_INT32 extendee:".LegacyTestMessage"`,
- reflect.TypeOf(int32(0)), depReg,
+ reflect.TypeOf([]int32(nil)), depReg,
),
mustMakeExtensionType(
`package:"fizz.buzz" dependency:"legacy.proto"`,
`name:"repeated_uint32" number:10012 label:LABEL_REPEATED type:TYPE_UINT32 extendee:".LegacyTestMessage"`,
- reflect.TypeOf(uint32(0)), depReg,
+ reflect.TypeOf([]uint32(nil)), depReg,
),
mustMakeExtensionType(
`package:"fizz.buzz" dependency:"legacy.proto"`,
`name:"repeated_float" number:10013 label:LABEL_REPEATED type:TYPE_FLOAT extendee:".LegacyTestMessage"`,
- reflect.TypeOf(float32(0)), depReg,
+ reflect.TypeOf([]float32(nil)), depReg,
),
mustMakeExtensionType(
`package:"fizz.buzz" dependency:"legacy.proto"`,
`name:"repeated_string" number:10014 label:LABEL_REPEATED type:TYPE_STRING extendee:".LegacyTestMessage"`,
- reflect.TypeOf(""), depReg,
+ reflect.TypeOf([]string(nil)), depReg,
),
mustMakeExtensionType(
`package:"fizz.buzz" dependency:"legacy.proto"`,
`name:"repeated_bytes" number:10015 label:LABEL_REPEATED type:TYPE_BYTES extendee:".LegacyTestMessage"`,
- reflect.TypeOf(([]byte)(nil)), depReg,
+ reflect.TypeOf([][]byte(nil)), depReg,
),
mustMakeExtensionType(
`package:"fizz.buzz" dependency:["legacy.proto", "proto2.v1.0.0-20180125-92554152/test.proto"]`,
`name:"repeated_enum_v1" number:10016 label:LABEL_REPEATED type:TYPE_ENUM type_name:".google.golang.org.proto2_20180125.Message.ChildEnum" extendee:".LegacyTestMessage"`,
- reflect.TypeOf(proto2_20180125.Message_ChildEnum(0)), depReg,
+ reflect.TypeOf([]proto2_20180125.Message_ChildEnum(nil)), depReg,
),
mustMakeExtensionType(
`package:"fizz.buzz" dependency:["legacy.proto", "proto2.v1.0.0-20180125-92554152/test.proto"]`,
`name:"repeated_message_v1" number:10017 label:LABEL_REPEATED type:TYPE_MESSAGE type_name:".google.golang.org.proto2_20180125.Message.ChildMessage" extendee:".LegacyTestMessage"`,
- reflect.TypeOf((*proto2_20180125.Message_ChildMessage)(nil)), depReg,
+ reflect.TypeOf([]*proto2_20180125.Message_ChildMessage(nil)), depReg,
),
mustMakeExtensionType(
`package:"fizz.buzz" dependency:["legacy.proto", "enum2.proto"]`,
`name:"repeated_enum_v2" number:10018 label:LABEL_REPEATED type:TYPE_ENUM type_name:".EnumProto2" extendee:".LegacyTestMessage"`,
- reflect.TypeOf(EnumProto2(0)), depReg,
+ reflect.TypeOf([]EnumProto2(nil)), depReg,
),
mustMakeExtensionType(
`package:"fizz.buzz" dependency:["legacy.proto", "enum-messages.proto"]`,
`name:"repeated_message_v2" number:10019 label:LABEL_REPEATED type:TYPE_MESSAGE type_name:".EnumMessages" extendee:".LegacyTestMessage"`,
- reflect.TypeOf((*EnumMessages)(nil)), depReg,
+ reflect.TypeOf([]*EnumMessages(nil)), depReg,
),
}
@@ -457,7 +457,7 @@
}
}
-func TestExtensionConvert(t *testing.T) {
+func TestLegacyExtensionConvert(t *testing.T) {
for i := range extensionTypes {
i := i
t.Run("", func(t *testing.T) {
@@ -466,7 +466,16 @@
wantType := extensionTypes[i]
wantDesc := extensionDescs[i]
gotType := (pref.ExtensionType)(wantDesc)
- gotDesc := pimpl.Export{}.ExtensionDescFromType(wantType)
+ gotDesc := wantType.(*pimpl.ExtensionInfo)
+
+ // Concurrently call accessors to trigger possible races.
+ for _, xt := range []pref.ExtensionType{wantType, wantDesc} {
+ xt := xt
+ go func() { xt.New() }()
+ go func() { xt.Zero() }()
+ go func() { xt.GoType() }()
+ go func() { xt.TypeDescriptor() }()
+ }
// TODO: We need a test package to compare descriptors.
type list interface {
@@ -594,9 +603,9 @@
return pimpl.Export{}.CompressGZIP(b)
}()
-// TestConcurrentInit tests that concurrent wrapping of multiple legacy types
+// TestLegacyConcurrentInit tests that concurrent wrapping of multiple legacy types
// results in the exact same descriptor being created.
-func TestConcurrentInit(t *testing.T) {
+func TestLegacyConcurrentInit(t *testing.T) {
const numParallel = 5
var messageATypes [numParallel]pref.MessageType
var messageBTypes [numParallel]pref.MessageType