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>

internal/filedesc/desc.go

@@ -355,18 +355,18 @@ type (
 		L2 *ExtensionL2 // protected by fileDesc.once
 	}
 	ExtensionL1 struct {
-		Number   pref.FieldNumber
+		Number      pref.FieldNumber
-		Extendee pref.MessageDescriptor
+		Extendee    pref.MessageDescriptor
-		Kind     pref.Kind
+		Cardinality pref.Cardinality
+		Kind        pref.Kind
 	}
 	ExtensionL2 struct {
-		Options     func() pref.ProtoMessage
+		Options  func() pref.ProtoMessage
-		Cardinality pref.Cardinality
+		JSONName jsonName
-		JSONName    jsonName
+		IsPacked bool // promoted from google.protobuf.FieldOptions
-		IsPacked    bool // promoted from google.protobuf.FieldOptions
+		Default  defaultValue
-		Default     defaultValue
+		Enum     pref.EnumDescriptor
-		Enum        pref.EnumDescriptor
+		Message  pref.MessageDescriptor
-		Message     pref.MessageDescriptor
 	}
 )
 
@@ -377,7 +377,7 @@ func (xd *Extension) Options() pref.ProtoMessage {
 	return descopts.Field
 }
 func (xd *Extension) Number() pref.FieldNumber                   { return xd.L1.Number }
-func (xd *Extension) Cardinality() pref.Cardinality              { return xd.lazyInit().Cardinality }
+func (xd *Extension) Cardinality() pref.Cardinality              { return xd.L1.Cardinality }
 func (xd *Extension) Kind() pref.Kind                            { return xd.L1.Kind }
 func (xd *Extension) HasJSONName() bool                          { return xd.lazyInit().JSONName.has }
 func (xd *Extension) JSONName() string                           { return xd.lazyInit().JSONName.get(xd) }

internal/filedesc/desc_init.go

@@ -379,6 +379,8 @@ func (xd *Extension) unmarshalSeed(b []byte, sb *strs.Builder, pf *File, pd pref
 			switch num {
 			case fieldnum.FieldDescriptorProto_Number:
 				xd.L1.Number = pref.FieldNumber(v)
+			case fieldnum.FieldDescriptorProto_Label:
+				xd.L1.Cardinality = pref.Cardinality(v)
 			case fieldnum.FieldDescriptorProto_Type:
 				xd.L1.Kind = pref.Kind(v)
 			}

internal/filedesc/desc_lazy.go

@@ -537,13 +537,6 @@ func (xd *Extension) unmarshalFull(b []byte, sb *strs.Builder) {
 		num, typ, n := wire.ConsumeTag(b)
 		b = b[n:]
 		switch typ {
-		case wire.VarintType:
-			v, m := wire.ConsumeVarint(b)
-			b = b[m:]
-			switch num {
-			case fieldnum.FieldDescriptorProto_Label:
-				xd.L2.Cardinality = pref.Cardinality(v)
-			}
 		case wire.BytesType:
 			v, m := wire.ConsumeBytes(b)
 			b = b[m:]

internal/filetype/build.go

@@ -225,6 +225,9 @@ func (tb Builder) Build() (out Out) {
 		default:
 			goType = goTypeForPBKind[fbOut.Extensions[i].L1.Kind]
 		}
+		if fbOut.Extensions[i].IsList() {
+			goType = reflect.SliceOf(goType)
+		}
 
 		pimpl.InitExtensionInfo(&tb.ExtensionInfos[i], &fbOut.Extensions[i], goType)
 

internal/impl/api_export.go

@@ -140,5 +140,9 @@ func (Export) MessageStringOf(m pref.ProtoMessage) string {
 
 // 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
 }

internal/impl/extension.go

@@ -26,21 +26,17 @@ type ExtensionInfo struct {
 	// initialized. This is the starting state for an ExtensionInfo
 	// in legacy generated code.
 	//
-	// extensionInfoDescInit: The desc and tdesc fields have been
+	// extensionInfoDescInit: The desc field is set, but other unexported fields
-	// set, but the descriptor is not otherwise initialized. Legacy
+	// may not be initialized. Legacy exported fields may or may not be set.
-	// exported fields may or may not be set. This is the starting state
+	// This is the starting state for an ExtensionInfo in newly generated code.
-	// for an ExtensionInfo in new generated code. Calling the Descriptor
-	// method will not trigger lazy initialization, although any other
-	// method will.
 	//
 	// 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 @@ const (
 )
 
 func InitExtensionInfo(xi *ExtensionInfo, xd pref.ExtensionDescriptor, goType reflect.Type) {
-	if xi.desc != nil {
-		return
-	}
-	xi.desc = xd
 	xi.goType = goType
-
+	xi.desc = extensionTypeDescriptor{xd, xi}
-	xi.tdesc.ExtensionDescriptor = xi.desc
-	xi.tdesc.xi = xi
 	xi.init = extensionInfoDescInit
 }
 
@@ -125,14 +115,14 @@ func (xi *ExtensionInfo) GoType() reflect.Type {
 	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 @@ func (xi *ExtensionInfo) lazyInitSlow() {
 	}
 	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 {

internal/impl/legacy_extension.go

@@ -6,7 +6,6 @@ package impl
 
 import (
 	"reflect"
-	"sync"
 
 	"google.golang.org/protobuf/internal/encoding/messageset"
 	ptag "google.golang.org/protobuf/internal/encoding/tag"
@@ -16,36 +15,6 @@ import (
 	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 @@ func (xi *ExtensionInfo) initFromLegacy() {
 	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 @@ func (xi *ExtensionInfo) initFromLegacy() {
 	if isOptional {
 		tt = tt.Elem()
 	}
-	xi.desc = xd
 	xi.goType = tt
+	xi.desc = extensionTypeDescriptor{xd, xi}
 }

internal/impl/legacy_test.go

@@ -143,52 +143,52 @@ var (
 		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 TestLegacyExtensions(t *testing.T) {
 	}
 }
 
-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 @@ func TestExtensionConvert(t *testing.T) {
 			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 @@ var concurrentFD = func() []byte {
 	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

reflect/protodesc/desc_init.go

@@ -168,7 +168,7 @@ func (r descsByName) initExtensionDeclarations(xds []*descriptorpb.FieldDescript
 			x.L2.IsPacked = opts.GetPacked()
 		}
 		x.L1.Number = protoreflect.FieldNumber(xd.GetNumber())
-		x.L2.Cardinality = protoreflect.Cardinality(xd.GetLabel())
+		x.L1.Cardinality = protoreflect.Cardinality(xd.GetLabel())
 		if xd.Type != nil {
 			x.L1.Kind = protoreflect.Kind(xd.GetType())
 		}