Skip to content

Commit

Permalink
[proto] use the correct parent when resolving features for extensions
Browse files Browse the repository at this point in the history
When I implemented this initially, I thought the parent of an extension is the
extendee. This is incorrect. The parent is the scope in which the extension is
defined. This CL changes the code to use the correct parent. This also allows
us to reduce some complexity in the implementation because we don't need to
wait until the extendee is resolved before we can resolve the features.

Change-Id: I6d7012f7502ef95457ab96f3e8abc4ab763d5bcb
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/579275
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
Auto-Submit: Lasse Folger <lassefolger@google.com>
  • Loading branch information
lfolger authored and gopherbot committed Apr 16, 2024
1 parent 98873a2 commit 671c2db
Show file tree
Hide file tree
Showing 12 changed files with 847 additions and 373 deletions.
28 changes: 14 additions & 14 deletions cmd/protoc-gen-go/testdata/extensions/proto3/ext3.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 10 additions & 9 deletions internal/filedesc/desc.go
Expand Up @@ -422,14 +422,6 @@ type (
Cardinality protoreflect.Cardinality
Kind protoreflect.Kind
EditionFeatures EditionFeatures
// To resolve the EditionFeatures we need to resolve the Extendee which
// happens at the end of the initialization of L1. Thus, we need to buffer
// the unresolved features (which are parsed when starting to initialize
// L1). We cannot move this to L2 because it is required to initialize
// Kind properly. Because some of the options (i.e. packed) affect the
// EditionFeatures we need to unmarshal the full options after resolving
// the Extendee.
rawOptions []byte
}
ExtensionL2 struct {
Options func() protoreflect.ProtoMessage
Expand Down Expand Up @@ -457,7 +449,16 @@ func (xd *Extension) HasPresence() bool { return xd.L1.Cardi
func (xd *Extension) HasOptionalKeyword() bool {
return (xd.L0.ParentFile.L1.Syntax == protoreflect.Proto2 && xd.L1.Cardinality == protoreflect.Optional) || xd.lazyInit().IsProto3Optional
}
func (xd *Extension) IsPacked() bool { return xd.L1.EditionFeatures.IsPacked }
func (xd *Extension) IsPacked() bool {
if xd.L1.Cardinality != protoreflect.Repeated {
return false
}
switch xd.L1.Kind {
case protoreflect.StringKind, protoreflect.BytesKind, protoreflect.MessageKind, protoreflect.GroupKind:
return false
}
return xd.L1.EditionFeatures.IsPacked
}
func (xd *Extension) IsExtension() bool { return true }
func (xd *Extension) IsWeak() bool { return false }
func (xd *Extension) IsList() bool { return xd.Cardinality() == protoreflect.Repeated }
Expand Down
21 changes: 6 additions & 15 deletions internal/filedesc/desc_init.go
Expand Up @@ -34,20 +34,6 @@ func newRawFile(db Builder) *File {
for i := range fd.allExtensions {
xd := &fd.allExtensions[i]
xd.L1.Extendee = fd.resolveMessageDependency(xd.L1.Extendee, listExtTargets, int32(i))

// If the Extendee is not resolved, we cannot resolve the edition
// features of the parent. This should (to my knowledge) only happen
// for v1 messages for which we don't support editions. In v2 the
// Extendee should be resolved at binary start up time.
if _, ok := xd.L1.Extendee.(PlaceholderMessage); !ok {
xd.L1.EditionFeatures = featuresFromParentDesc(xd.L1.Extendee)
}
if xd.L1.rawOptions != nil {
xd.unmarshalOptions(xd.L1.rawOptions)
}
if xd.L1.Kind == protoreflect.MessageKind && xd.L1.EditionFeatures.IsDelimitedEncoded {
xd.L1.Kind = protoreflect.GroupKind
}
}

fd.checkDecls()
Expand Down Expand Up @@ -459,6 +445,7 @@ func (xd *Extension) unmarshalSeed(b []byte, sb *strs.Builder, pf *File, pd prot
xd.L0.ParentFile = pf
xd.L0.Parent = pd
xd.L0.Index = i
xd.L1.EditionFeatures = featuresFromParentDesc(pd)

for len(b) > 0 {
num, typ, n := protowire.ConsumeTag(b)
Expand All @@ -484,13 +471,17 @@ func (xd *Extension) unmarshalSeed(b []byte, sb *strs.Builder, pf *File, pd prot
case genid.FieldDescriptorProto_Extendee_field_number:
xd.L1.Extendee = PlaceholderMessage(makeFullName(sb, v))
case genid.FieldDescriptorProto_Options_field_number:
xd.L1.rawOptions = v
xd.unmarshalOptions(v)
}
default:
m := protowire.ConsumeFieldValue(num, typ, b)
b = b[m:]
}
}

if xd.L1.Kind == protoreflect.MessageKind && xd.L1.EditionFeatures.IsDelimitedEncoded {
xd.L1.Kind = protoreflect.GroupKind
}
}

func (xd *Extension) unmarshalOptions(b []byte) {
Expand Down
28 changes: 14 additions & 14 deletions internal/testprotos/test3/test_extension.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 671c2db

Please sign in to comment.